Restrict permissions for zfs/dbgmsg and other kstats, print real pointers

[shartse]

Motivation and Context

This is addressing this issue: #8467

There are several places where we use zfs_dbgmsg and %p to print pointers. In the Linux kernel, these values obfuscated to prevent information leaks (which makes sense, as /proc/spl/kstat/zfs/dbgmsg is world readable). However, this means the pointers aren't very useful for debugging crash dumps since you can't find the location of the struct they refer to.

Description

I found that kstat proc creation always uses the same permission mode (0644). I abstracted out the mode so that different kstats can be created with different permissions. Upon suggestion, I reduced the permissions of dbufs and pool/reads along with dbgmsg to 0600 but left all other kstats with the original mode.

Then, I looked for occurrences of %p in calls to zfs_dbgmsg and replaced them with %px so that the actual pointer is printed.

Finally, as proposed by @pcd1193182 in https://github.com/delphix/zfs/pull/40, I changed spl_dumpstack to use px as well.

How Has This Been Tested?

Loaded changes on a VM and verififed that the kstat permissions changed as expected.

Before

> ls -l  /proc/spl/kstat/zfs/
-rw-r--r-- 1 root root 0 Mar 29 22:17 abdstats
-rw-r--r-- 1 root root 0 Mar 29 22:17 arcstats
-rw-r--r-- 1 root root 0 Mar 29 22:17 dbgmsg
-rw-r--r-- 1 root root 0 Mar 29 22:17 dbufs
-rw-r--r-- 1 root root 0 Mar 29 22:17 dbufstats
...
> ls -l /proc/spl/kstat/zfs/rpool/
-rw-r--r-- 1 root root 0 Mar 29 22:18 dmu_tx_assign
...
-rw-r--r-- 1 root root 0 Mar 29 22:18 reads
...

After

> ls -l  /proc/spl/kstat/zfs/
-rw-r--r-- 1 root root 0 Mar 29 22:16 abdstats
-rw-r--r-- 1 root root 0 Mar 29 22:16 arcstats
-rw------- 1 root root 0 Mar 29 22:16 dbgmsg
-rw------- 1 root root 0 Mar 29 22:16 dbufs
-rw-r--r-- 1 root root 0 Mar 29 22:16 dbufstats
...
> ls -l /proc/spl/kstat/zfs/rpool/
total 0
-rw-r--r-- 1 root root 0 Mar 29 22:19 dmu_tx_assign
...
-rw------- 1 root root 0 Mar 29 22:19 reads
...

Inspected /proc/spl/kstat/zfs/dbgmsg and saw that the logged pointers from metaslab_condense now look legitimate.

Before
metaslab_condense(): condensing: txg 352980, msp[49] 000000003159e1df, vdev id 0, spa rpool, smp size 17056, segents 703, forcing condense=FALSE

After (requires sudo to access)
metaslab_condense(): condensing: txg 84170, msp[51] ffff9619a0271800, vdev id 0, spa rpool, smp size 16520, segments 421, forcing condense=FALSE

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[behlendorf]

That make sense, I'd just suggest we restrict these two additional kstats while we're here.

/proc/spl/kstat/zfs/dbufs
/proc/spl/kstat/zfs/pool/reads

[dweeezil]

I think this type of protection has been needed for quite some time. There are likely other kstats which we ought to similarly protect lest they be used to perform DOS attacks. I'd suggest restricting taskq-all as well as possibly others. Unfortunately, many of the other kstats, when repeatedly read in a tight loop, could cause various forms of DOS. For example, reading even arcstats at a high rate would defeat out attempt to reduce the use of atomic instructions.

[shartse]

@behlendorf and @dweeezil - it looks like some of the kstats you mentioned are initialized directly through procfs_list_install but others go through kstat_install which calls procfs_list_install with a default 0644. Do you think it makes sense to add another argument to kstat_install, forcing people to pick a permissions mode when they create their kstat? Or do we not want to change that interface and instead special case certain kstats by name within kstat_install?

[shartse]

Or perhaps I should add mode to the kstat struct itself? I like they idea of developers having to think a little more about what the permissions really should be when adding kstats.

[behlendorf]

@shartse historically we've avoided changing any of the interfaces we're emulating from Illumos. Primarily to reduce code differences and avoid conflicts when porting. Since we only have a few proc entries we want to target, and they don't have Illumos counterparts, I'd suggest we just special case them for now.

[behlendorf]

@shartse I agree the special casing approach makes sense for this this. Everything looks good except we had one test failure with dbufstat_001_pos due to a permission problem which needs to be investigated.

Test: /usr/share/zfs/zfs-tests/tests/functional/cli_user/misc/dbufstat_001_pos (run as buildbot) [00:00] [FAIL]
00:16:29.80 ASSERTION: dbufstat generates output and doesn't return an error code
00:16:29.82 Cannot open /proc/spl/kstat/zfs/dbufs for reading
00:16:29.82 ERROR: eval dbufstat  > /dev/null exited 1

[shartse]

Cool - I added sudo to the test invocations dbufstat, which seems reasonable to me as this work restricts the visibility of the dbuf kstat. I'll take a quick look to see if other kstats changed here are used in the tests in a way that wouldn't fail as obviously.

[codecov]

Codecov Report

Merging #8476 into master will decrease coverage by 0.08%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #8476      +/-   ##
==========================================
- Coverage   78.81%   78.73%   -0.09%     
==========================================
  Files         381      381              
  Lines      117512   117513       +1     
==========================================
- Hits        92622    92522     -100     
- Misses      24890    24991     +101

Flag	Coverage Δ
#kernel	79.24% <83.33%> (-0.11%)	⬇️
#user	67.27% <50%> (-0.25%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af65079...d22197b. Read the comment docs.

[problame]

(Late) follow-up question about this change: was there any particular reason why the dprintf calls weren't changed to use %px as well? And: does the kernel have an exception for cases where the format string is in a panic message?

$ git grep  -P '%p[^x]' a887d653b32aaba3fe04c7b25ff0091b9ea9c64e module/zfs
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/arc.c:              cmn_err(CE_PANIC, "invalid arc state 0x%p",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/arc.c:                                      panic("bad overwrite, hdr=%p exists=%p",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/arc.c:                                      panic("bad nopwrite, hdr=%p exists=%p",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dbuf.c:     dprintf_dbuf(db, "db=%p\n", db);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dbuf.c:     dprintf_dbuf_bp(db, db->db_blkptr, "blkptr=%p", db->db_blkptr);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dbuf.c:     dprintf_dbuf_bp(db, db->db_blkptr, "blkptr=%p", db->db_blkptr);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dnode.c:    dprintf("os=%p obj=%llu txg=%llu blocksize=%d ibs=%d dn_slots=%d\n",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dnode.c:    dprintf_dnode(dn, "dn=%p dnp=%p used=%llu delta=%lld\n",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dnode_sync.c:       dprintf("os=%p obj=%llu, increase to %d\n", dn->dn_objset,
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dnode_sync.c:       dprintf("ds=%p obj=%llx num=%d\n", ds, dn->dn_object, num);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dnode_sync.c:                                           "child=%p i=%d off=%d num=%d\n",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dnode_sync.c:                                           "child=%p i=%d off=%d num=%d\n",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dsl_dataset.c:      dprintf_bp(bp, "ds=%p", ds);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dsl_deadlist.c:             zfs_panic_recover("blkptr at %p has invalid BLK_BIRTH %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dsl_dir.c:          panic("invalid p=%p", (void *)p);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dsl_dir.c:          dprintf("next=%p (%s) tail=%p\n", next, next?next:"", tailp);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/dsl_scan.c:  *     "visiting ds=%p/%llu zb=%llx/%llx/%llx/%llx bp=%p",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/range_tree.c:               panic("segment already in tree; rs=%p", (void *)rs);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/refcount.c: panic("No such hold %p on refcount %llx", holder,
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/rrwlock.c:          panic("thread %p terminating with rrw lock %p held",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/txg.c:      dprintf("pool %p\n", dp);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/txg.c:      dprintf("pool %p\n", dp);
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/txg.c:                      dprintf("waiting; tx_synced=%llu waiting=%llu dp=%p\n",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/txg.c:                  "tx_synced=%llu waiting=%llu dp=%p\n",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:              zfs_panic_recover("blkptr at %p has invalid TYPE %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:              zfs_panic_recover("blkptr at %p has invalid CHECKSUM %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:              zfs_panic_recover("blkptr at %p has invalid COMPRESS %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:              zfs_panic_recover("blkptr at %p has invalid LSIZE %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:              zfs_panic_recover("blkptr at %p has invalid PSIZE %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:                      zfs_panic_recover("blkptr at %p has invalid ETYPE %llu",
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:                      zfs_panic_recover("blkptr at %p DVA %u has invalid "
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:                      zfs_panic_recover("blkptr at %p DVA %u has invalid "
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:                      zfs_panic_recover("blkptr at %p DVA %u has hole "
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zio.c:                      zfs_panic_recover("blkptr at %p DVA %u has invalid "
a887d653b32aaba3fe04c7b25ff0091b9ea9c64e:module/zfs/zpl_xattr.c:                cmn_err(CE_WARN, "ZFS: inode %p has xattr \"%s\""

[behlendorf]

I believe changing the dprintf was simply overlooked (or left for another time). We could change them since they're protected by the dbgmsg permissions, but It'd probably be a good idea to go through and audit all these calls and see if printing the pointer value is actually helpful at all. It seems like in some cases it should just be removed. Of course, anything the might go the console (zfs_panic_recover, cmd_err, panic) we'll need to leave hashed.