From a887d653b32aaba3fe04c7b25ff0091b9ea9c64e Mon Sep 17 00:00:00 2001 From: Sara Hartse Date: Thu, 4 Apr 2019 18:57:06 -0700 Subject: [PATCH] Restrict kstats and print real pointers 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 means the pointers aren't very useful for debugging crash dumps. We decided to restrict the permissions of dbgmsg (and some other kstats while we were at it) and print pointers with %px in zfs_dbgmsg as well as spl_dumpstack Reviewed-by: Brian Behlendorf Reviewed-by: John Gallagher Signed-off-by: sara hartse Closes #8467 Closes #8476 --- cmd/ztest/ztest.c | 2 +- include/spl/sys/debug.h | 2 +- include/spl/sys/kstat.h | 2 +- include/spl/sys/procfs_list.h | 1 + include/sys/zfs_context.h | 1 + lib/libzpool/kernel.c | 1 + module/spl/spl-kstat.c | 14 +++++++++++--- module/spl/spl-procfs-list.c | 3 ++- module/zfs/arc.c | 2 +- module/zfs/metaslab.c | 2 +- module/zfs/range_tree.c | 2 +- module/zfs/spa_stats.c | 3 +++ module/zfs/space_map.c | 2 +- module/zfs/vdev_removal.c | 2 +- module/zfs/zfs_debug.c | 1 + module/zfs/zil.c | 2 +- module/zfs/zio.c | 4 ++-- .../functional/cli_user/misc/dbufstat_001_pos.ksh | 4 ++-- 18 files changed, 33 insertions(+), 17 deletions(-) diff --git a/cmd/ztest/ztest.c b/cmd/ztest/ztest.c index a1ab56bd2f5..9c2cf950183 100644 --- a/cmd/ztest/ztest.c +++ b/cmd/ztest/ztest.c @@ -6468,7 +6468,7 @@ ztest_initialize(ztest_ds_t *zd, uint64_t id) char *path = strdup(rand_vd->vdev_path); boolean_t active = rand_vd->vdev_initialize_thread != NULL; - zfs_dbgmsg("vd %p, guid %llu", rand_vd, guid); + zfs_dbgmsg("vd %px, guid %llu", rand_vd, guid); spa_config_exit(spa, SCL_VDEV, FTAG); uint64_t cmd = ztest_random(POOL_INITIALIZE_FUNCS); diff --git a/include/spl/sys/debug.h b/include/spl/sys/debug.h index d9336c7d158..b17d77d280a 100644 --- a/include/spl/sys/debug.h +++ b/include/spl/sys/debug.h @@ -102,7 +102,7 @@ void spl_dumpstack(void); if (!(_verify3_left OP _verify3_right)) \ spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ - "failed (%p " #OP " %p)\n", \ + "failed (%px" #OP " %px)\n", \ (void *) (_verify3_left), \ (void *) (_verify3_right)); \ } while (0) diff --git a/include/spl/sys/kstat.h b/include/spl/sys/kstat.h index 53274d8f59c..3ce47424887 100644 --- a/include/spl/sys/kstat.h +++ b/include/spl/sys/kstat.h @@ -196,7 +196,7 @@ extern kstat_t *__kstat_create(const char *ks_module, int ks_instance, extern void kstat_proc_entry_init(kstat_proc_entry_t *kpep, const char *module, const char *name); extern void kstat_proc_entry_delete(kstat_proc_entry_t *kpep); -extern void kstat_proc_entry_install(kstat_proc_entry_t *kpep, +extern void kstat_proc_entry_install(kstat_proc_entry_t *kpep, mode_t mode, const struct file_operations *file_ops, void *data); extern void __kstat_install(kstat_t *ksp); diff --git a/include/spl/sys/procfs_list.h b/include/spl/sys/procfs_list.h index cbcb4bcffff..eb1519c0ad6 100644 --- a/include/spl/sys/procfs_list.h +++ b/include/spl/sys/procfs_list.h @@ -58,6 +58,7 @@ typedef struct procfs_list_node { void procfs_list_install(const char *module, const char *name, + mode_t mode, procfs_list_t *procfs_list, int (*show)(struct seq_file *f, void *p), int (*show_header)(struct seq_file *f), diff --git a/include/sys/zfs_context.h b/include/sys/zfs_context.h index 87ddde30a3b..e3fa2e61bdc 100644 --- a/include/sys/zfs_context.h +++ b/include/sys/zfs_context.h @@ -375,6 +375,7 @@ typedef struct procfs_list_node { void procfs_list_install(const char *module, const char *name, + mode_t mode, procfs_list_t *procfs_list, int (*show)(struct seq_file *f, void *p), int (*show_header)(struct seq_file *f), diff --git a/lib/libzpool/kernel.c b/lib/libzpool/kernel.c index 926f4f4f40b..f22ad56b533 100644 --- a/lib/libzpool/kernel.c +++ b/lib/libzpool/kernel.c @@ -437,6 +437,7 @@ seq_printf(struct seq_file *m, const char *fmt, ...) void procfs_list_install(const char *module, const char *name, + mode_t mode, procfs_list_t *procfs_list, int (*show)(struct seq_file *f, void *p), int (*show_header)(struct seq_file *f), diff --git a/module/spl/spl-kstat.c b/module/spl/spl-kstat.c index 7207a35e04b..feff31e6c75 100644 --- a/module/spl/spl-kstat.c +++ b/module/spl/spl-kstat.c @@ -659,7 +659,7 @@ kstat_detect_collision(kstat_proc_entry_t *kpep) * kstat. */ void -kstat_proc_entry_install(kstat_proc_entry_t *kpep, +kstat_proc_entry_install(kstat_proc_entry_t *kpep, mode_t mode, const struct file_operations *file_ops, void *data) { kstat_module_t *module; @@ -693,7 +693,7 @@ kstat_proc_entry_install(kstat_proc_entry_t *kpep, list_add_tail(&kpep->kpe_list, &module->ksm_kstat_list); kpep->kpe_owner = module; - kpep->kpe_proc = proc_create_data(kpep->kpe_name, 0644, + kpep->kpe_proc = proc_create_data(kpep->kpe_name, mode, module->ksm_proc, file_ops, data); if (kpep->kpe_proc == NULL) { list_del_init(&kpep->kpe_list); @@ -710,7 +710,15 @@ void __kstat_install(kstat_t *ksp) { ASSERT(ksp); - kstat_proc_entry_install(&ksp->ks_proc, &proc_kstat_operations, ksp); + mode_t mode; + /* Specify permission modes for different kstats */ + if (strncmp(ksp->ks_proc.kpe_name, "dbufs", KSTAT_STRLEN) == 0) { + mode = 0600; + } else { + mode = 0644; + } + kstat_proc_entry_install( + &ksp->ks_proc, mode, &proc_kstat_operations, ksp); } EXPORT_SYMBOL(__kstat_install); diff --git a/module/spl/spl-procfs-list.c b/module/spl/spl-procfs-list.c index 4902e0a56c6..f6a00da5c91 100644 --- a/module/spl/spl-procfs-list.c +++ b/module/spl/spl-procfs-list.c @@ -201,6 +201,7 @@ static struct file_operations procfs_list_operations = { void procfs_list_install(const char *module, const char *name, + mode_t mode, procfs_list_t *procfs_list, int (*show)(struct seq_file *f, void *p), int (*show_header)(struct seq_file *f), @@ -218,7 +219,7 @@ procfs_list_install(const char *module, procfs_list->pl_node_offset = procfs_list_node_off; kstat_proc_entry_init(&procfs_list->pl_kstat_entry, module, name); - kstat_proc_entry_install(&procfs_list->pl_kstat_entry, + kstat_proc_entry_install(&procfs_list->pl_kstat_entry, mode, &procfs_list_operations, procfs_list); } EXPORT_SYMBOL(procfs_list_install); diff --git a/module/zfs/arc.c b/module/zfs/arc.c index 50d0125df81..c724878948d 100644 --- a/module/zfs/arc.c +++ b/module/zfs/arc.c @@ -2266,7 +2266,7 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, const zbookmark_phys_t *zb, */ if (error != 0) { zfs_dbgmsg( - "hdr %p, compress %d, psize %d, lsize %d", + "hdr %px, compress %d, psize %d, lsize %d", hdr, arc_hdr_get_compress(hdr), HDR_GET_PSIZE(hdr), HDR_GET_LSIZE(hdr)); if (hash_lock != NULL) diff --git a/module/zfs/metaslab.c b/module/zfs/metaslab.c index 06d8383f09c..ec89810b48a 100644 --- a/module/zfs/metaslab.c +++ b/module/zfs/metaslab.c @@ -2643,7 +2643,7 @@ metaslab_condense(metaslab_t *msp, uint64_t txg, dmu_tx_t *tx) ASSERT(msp->ms_loaded); - zfs_dbgmsg("condensing: txg %llu, msp[%llu] %p, vdev id %llu, " + zfs_dbgmsg("condensing: txg %llu, msp[%llu] %px, vdev id %llu, " "spa %s, smp size %llu, segments %lu, forcing condense=%s", txg, msp->ms_id, msp, msp->ms_group->mg_vd->vdev_id, msp->ms_group->mg_vd->vdev_spa->spa_name, diff --git a/module/zfs/range_tree.c b/module/zfs/range_tree.c index 1a31c112925..391533b3f49 100644 --- a/module/zfs/range_tree.c +++ b/module/zfs/range_tree.c @@ -118,7 +118,7 @@ range_tree_stat_verify(range_tree_t *rt) for (i = 0; i < RANGE_TREE_HISTOGRAM_SIZE; i++) { if (hist[i] != rt->rt_histogram[i]) { - zfs_dbgmsg("i=%d, hist=%p, hist=%llu, rt_hist=%llu", + zfs_dbgmsg("i=%d, hist=%px, hist=%llu, rt_hist=%llu", i, hist, hist[i], rt->rt_histogram[i]); } VERIFY3U(hist[i], ==, rt->rt_histogram[i]); diff --git a/module/zfs/spa_stats.c b/module/zfs/spa_stats.c index 3b51250c683..6895428f4fa 100644 --- a/module/zfs/spa_stats.c +++ b/module/zfs/spa_stats.c @@ -131,6 +131,7 @@ spa_read_history_init(spa_t *spa) shl->procfs_list.pl_private = shl; procfs_list_install(module, "reads", + 0600, &shl->procfs_list, spa_read_history_show, spa_read_history_show_header, @@ -301,6 +302,7 @@ spa_txg_history_init(spa_t *spa) shl->procfs_list.pl_private = shl; procfs_list_install(module, "txgs", + 0644, &shl->procfs_list, spa_txg_history_show, spa_txg_history_show_header, @@ -706,6 +708,7 @@ spa_mmp_history_init(spa_t *spa) shl->procfs_list.pl_private = shl; procfs_list_install(module, "multihost", + 0644, &shl->procfs_list, spa_mmp_history_show, spa_mmp_history_show_header, diff --git a/module/zfs/space_map.c b/module/zfs/space_map.c index 5cf3feaae10..d9cd8767e09 100644 --- a/module/zfs/space_map.c +++ b/module/zfs/space_map.c @@ -848,7 +848,7 @@ space_map_truncate(space_map_t *sm, int blocksize, dmu_tx_t *tx) doi.doi_bonus_size != sizeof (space_map_phys_t)) || doi.doi_data_block_size != blocksize || doi.doi_metadata_block_size != 1 << space_map_ibs) { - zfs_dbgmsg("txg %llu, spa %s, sm %p, reallocating " + zfs_dbgmsg("txg %llu, spa %s, sm %px, reallocating " "object[%llu]: old bonus %u, old blocksz %u", dmu_tx_get_txg(tx), spa_name(spa), sm, sm->sm_object, doi.doi_bonus_size, doi.doi_data_block_size); diff --git a/module/zfs/vdev_removal.c b/module/zfs/vdev_removal.c index 99d67b7be85..f2d18d9257b 100644 --- a/module/zfs/vdev_removal.c +++ b/module/zfs/vdev_removal.c @@ -340,7 +340,7 @@ vdev_remove_initiate_sync(void *arg, dmu_tx_t *tx) */ vdev_config_dirty(vd); - zfs_dbgmsg("starting removal thread for vdev %llu (%p) in txg %llu " + zfs_dbgmsg("starting removal thread for vdev %llu (%px) in txg %llu " "im_obj=%llu", vd->vdev_id, vd, dmu_tx_get_txg(tx), vic->vic_mapping_object); diff --git a/module/zfs/zfs_debug.c b/module/zfs/zfs_debug.c index 62c59e02092..538533d27d2 100644 --- a/module/zfs/zfs_debug.c +++ b/module/zfs/zfs_debug.c @@ -94,6 +94,7 @@ zfs_dbgmsg_init(void) { procfs_list_install("zfs", "dbgmsg", + 0600, &zfs_dbgmsgs, zfs_dbgmsg_show, zfs_dbgmsg_show_header, diff --git a/module/zfs/zil.c b/module/zfs/zil.c index e5de8b5e4c2..a70fe1a629e 100644 --- a/module/zfs/zil.c +++ b/module/zfs/zil.c @@ -3232,7 +3232,7 @@ zil_close(zilog_t *zilog) txg_wait_synced(zilog->zl_dmu_pool, txg); if (zilog_is_dirty(zilog)) - zfs_dbgmsg("zil (%p) is dirty, txg %llu", zilog, txg); + zfs_dbgmsg("zil (%px) is dirty, txg %llu", zilog, txg); if (txg < spa_freeze_txg(zilog->zl_spa)) VERIFY(!zilog_is_dirty(zilog)); diff --git a/module/zfs/zio.c b/module/zfs/zio.c index 1915de41729..016ac07eabd 100644 --- a/module/zfs/zio.c +++ b/module/zfs/zio.c @@ -1893,7 +1893,7 @@ zio_deadman_impl(zio_t *pio, int ziodepth) uint64_t delta = gethrtime() - pio->io_timestamp; uint64_t failmode = spa_get_deadman_failmode(pio->io_spa); - zfs_dbgmsg("slow zio[%d]: zio=%p timestamp=%llu " + zfs_dbgmsg("slow zio[%d]: zio=%px timestamp=%llu " "delta=%llu queued=%llu io=%llu " "path=%s last=%llu " "type=%d priority=%d flags=0x%x " @@ -3444,7 +3444,7 @@ zio_dva_allocate(zio_t *zio) } if (error != 0) { - zfs_dbgmsg("%s: metaslab allocation failure: zio %p, " + zfs_dbgmsg("%s: metaslab allocation failure: zio %px, " "size %llu, error %d", spa_name(spa), zio, zio->io_size, error); if (error == ENOSPC && zio->io_size > SPA_MINBLOCKSIZE) diff --git a/tests/zfs-tests/tests/functional/cli_user/misc/dbufstat_001_pos.ksh b/tests/zfs-tests/tests/functional/cli_user/misc/dbufstat_001_pos.ksh index 95f0598c618..0e187015f8d 100755 --- a/tests/zfs-tests/tests/functional/cli_user/misc/dbufstat_001_pos.ksh +++ b/tests/zfs-tests/tests/functional/cli_user/misc/dbufstat_001_pos.ksh @@ -33,11 +33,11 @@ log_assert "dbufstat generates output and doesn't return an error code" typeset -i i=0 while [[ $i -lt ${#args[*]} ]]; do - log_must eval "dbufstat ${args[i]} > /dev/null" + log_must eval "sudo dbufstat ${args[i]} > /dev/null" ((i = i + 1)) done # A simple test of dbufstat filter functionality -log_must eval "dbufstat -F object=10,dbc=1,pool=$TESTPOOL > /dev/null" +log_must eval "sudo dbufstat -F object=10,dbc=1,pool=$TESTPOOL > /dev/null" log_pass "dbufstat generates output and doesn't return an error code"