From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from todd.t-8ch.de (todd.t-8ch.de [159.69.126.157]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2C96720469B for ; Thu, 9 Jan 2025 15:25:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=159.69.126.157 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736436312; cv=none; b=Ibquv0GbLARtHpJJ4PDzl2H/8mPPEUhp3l26pr0N3N8Ktuwdw0NqmJ+r5g4m9aZSortTBI1B5hYHr9KvPeOYDP2Q0pPTaJBXfY2ykgSu1XyllnZz2WTs/lTJ7VX067y+ThAZbEouDA4iX2wzso30XdaCkMFApzeSC2V9II0bq0U= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736436312; c=relaxed/simple; bh=Oh6q0EOdbZzTW3oIxUi+gsEyuZecdZrpr5X2Wc2AocQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BwquSh8vFh/el09DQ/p8AVwRHV6z+Oi5n/BhsTAcjCyHoNjMgiP/TSxgeivCO5WG32o22k000FXHihsadg6lK0S3G6bneQqwti13jV2hXUxusHiq5zB+PL0V+A+Z4UNp9o0alf0LHbHVWFMe135PRlEb1lVUO+8YqprxoOfQcyo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=weissschuh.net; spf=pass smtp.mailfrom=weissschuh.net; dkim=pass (1024-bit key) header.d=weissschuh.net header.i=@weissschuh.net header.b=Qm8SayWE; arc=none smtp.client-ip=159.69.126.157 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=weissschuh.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=weissschuh.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=weissschuh.net header.i=@weissschuh.net header.b="Qm8SayWE" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=weissschuh.net; s=mail; t=1736436307; bh=Oh6q0EOdbZzTW3oIxUi+gsEyuZecdZrpr5X2Wc2AocQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Qm8SayWE/NuZqwgYrwXHCvH9j8rNCfZkCrA36GitovtvKddR61bORJMHG3MrPrbLs G/v2rKGoTFrbCRlvqd+i9fpGnkn+K6pTull7kZaWQUduluF3X9qgiPS6/tEQt8eIHQ XLrdFarb42vp0x2uV6IxitnFgX896cxsSvIr9iOw= Date: Thu, 9 Jan 2025 16:25:06 +0100 From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= To: Joel Granados Cc: Kees Cook , Luis Chamberlain , linux-kernel@vger.kernel.org Subject: Re: [PATCH] treewide: const qualify ctl_tables where applicable Message-ID: <73f3c35b-35f1-433e-bd67-b428c3f2dd6e@t-8ch.de> References: <20250109-jag-ctl_table_const-v1-1-622aea7230cf@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250109-jag-ctl_table_const-v1-1-622aea7230cf@kernel.org> (Drop most recipients as it doesn't affect them) On 2025-01-09 14:16:39+0100, Joel Granados wrote: > Add the const qualifier to all the ctl_tables in the tree except the > ones in ./net dir. The "net" sysctl code is special as it modifies the > arrays before passing it on to the registration function. > > Constifying ctl_table structs will prevent the modification of > proc_handler function pointers as the arrays would reside in .rodata. > This is made possible after commit 78eb4ea25cd5 ("sysctl: treewide: > constify the ctl_table argument of proc_handlers") constified all the > proc_handlers. > > Created this by running an spatch followed by a sed command: > Spatch: > virtual patch > > @ > depends on !(file in "net") > disable optional_qualifier > @ > identifier table_name; > @@ > > + const > struct ctl_table table_name [] = { ... }; > > sed: > sed --in-place \ > -e "s/struct ctl_table .table = &uts_kern/const struct ctl_table *table = \&uts_kern/" \ > kernel/utsname_sysctl.c > > Signed-off-by: Joel Granados > --- > This treewide commit builds upon the work Thomas began a few releases > ago [1], where he laid the groundwork for constifying ctl_tables. We > implement constification throughout the tree, with the exception of the > ctl_tables in the "net" directory. Those are special in that they treat > the ctl_table as non-const but we can take them at a later point. The modifications in net/ are from register_net_sysctl_sz(), correct? Given that before modifying the table the code will already have called WARN() and thereby tainted and most likely panicked the system. If it is fine to crash the system I would argue it is also fine to just fail to register the sysctl. Then the modification would not be necessary anymore. Something like (untested): diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h index 5a2a0df8ad91..5b0fff5fcaf9 100644 --- a/include/net/net_namespace.h +++ b/include/net/net_namespace.h @@ -496,12 +496,12 @@ struct ctl_table; #ifdef CONFIG_SYSCTL int net_sysctl_init(void); struct ctl_table_header *register_net_sysctl_sz(struct net *net, const char *path, - struct ctl_table *table, size_t table_size); + const struct ctl_table *table, size_t table_size); void unregister_net_sysctl_table(struct ctl_table_header *header); #else static inline int net_sysctl_init(void) { return 0; } static inline struct ctl_table_header *register_net_sysctl_sz(struct net *net, - const char *path, struct ctl_table *table, size_t table_size) + const char *path, const struct ctl_table *table, size_t table_size) { return NULL; } diff --git a/net/sysctl_net.c b/net/sysctl_net.c index 19e8048241ba..754a3713eadb 100644 --- a/net/sysctl_net.c +++ b/net/sysctl_net.c @@ -120,10 +120,10 @@ __init int net_sysctl_init(void) * data segment, and rather into the heap where a per-net object was * allocated. */ -static void ensure_safe_net_sysctl(struct net *net, const char *path, - struct ctl_table *table, size_t table_size) +static bool ensure_safe_net_sysctl(struct net *net, const char *path, + const struct ctl_table *table, size_t table_size) { - struct ctl_table *ent; + const struct ctl_table *ent; pr_debug("Registering net sysctl (net %p): %s\n", net, path); ent = table; @@ -155,18 +155,20 @@ static void ensure_safe_net_sysctl(struct net *net, const char *path, WARN(1, "sysctl %s/%s: data points to %s global data: %ps\n", path, ent->procname, where, ent->data); - /* Make it "safe" by dropping writable perms */ - ent->mode &= ~0222; + return false; } + + return true; } struct ctl_table_header *register_net_sysctl_sz(struct net *net, const char *path, - struct ctl_table *table, + const struct ctl_table *table, size_t table_size) { - if (!net_eq(net, &init_net)) - ensure_safe_net_sysctl(net, path, table, table_size); + if (!net_eq(net, &init_net) && + !ensure_safe_net_sysctl(net, path, table, table_size)) + return NULL; return __register_sysctl_table(&net->sysctls, path, table, table_size); } > Upstreaming: > =========== > It is late in the release cycle, but I'm hopeful that we can get this > in for the upcoming merge window and this is why: > 1. We don't use linux-next: As with previous treewide changes similar to > this one [1], we avoid using linux-next in order to avoid unwanted > merge conflicts > 2. This is a non-functional change: which lowers the probability of > unforeseen errors or regressions. > 3. It will have at least 2 weeks to be tested/reviewed: The PULL should > be sent at the end of the merge window, giving it at least 2 weeks. > And if there are more release candidates after rc6, there will be > more time. > > Testing: > ======== > 1. Currently being tested in 0-day > 2. sysctl self-tests/kunit-tests > > Reduced To/Cc: > ============== > b4 originally gave me 200 ppl that this should go out to (which seems a > bit overkill from my point of view). So I left the mailing lists and > reduced the To: the ppl previously involved in the effort and sysctl > maintainers. Please tell me if I missed someone important to the > constification effort. > > Comments are greatly appreciated. > Specially interested in any specifics from @Thomas and @kees Looks good to me, not much (surprising) going on. > Best > > [1] https://lore.kernel.org/20240724210014.mc6nima6cekgiukx@joelS2.panther.com > > -- > --- > arch/arm/kernel/isa.c | 2 +- > arch/arm64/kernel/fpsimd.c | 4 ++-- > arch/arm64/kernel/process.c | 2 +- > arch/powerpc/kernel/idle.c | 2 +- > arch/powerpc/platforms/pseries/mobility.c | 2 +- > arch/riscv/kernel/process.c | 2 +- > arch/riscv/kernel/vector.c | 2 +- > arch/s390/appldata/appldata_base.c | 2 +- > arch/s390/kernel/debug.c | 2 +- > arch/s390/kernel/hiperdispatch.c | 2 +- > arch/s390/kernel/topology.c | 2 +- > arch/s390/mm/cmm.c | 2 +- > arch/s390/mm/pgalloc.c | 2 +- > arch/x86/entry/vdso/vdso32-setup.c | 2 +- > arch/x86/kernel/cpu/bus_lock.c | 2 +- > arch/x86/kernel/itmt.c | 2 +- > crypto/fips.c | 2 +- > drivers/base/firmware_loader/fallback_table.c | 2 +- > drivers/cdrom/cdrom.c | 2 +- > drivers/char/hpet.c | 2 +- > drivers/char/ipmi/ipmi_poweroff.c | 2 +- > drivers/char/random.c | 2 +- > drivers/gpu/drm/i915/i915_perf.c | 2 +- > drivers/gpu/drm/xe/xe_observation.c | 2 +- > drivers/hv/hv_common.c | 2 +- > drivers/infiniband/core/iwcm.c | 2 +- > drivers/infiniband/core/ucma.c | 2 +- > drivers/macintosh/mac_hid.c | 2 +- > drivers/md/md.c | 2 +- > drivers/misc/sgi-xp/xpc_main.c | 4 ++-- > drivers/perf/arm_pmuv3.c | 2 +- > drivers/perf/riscv_pmu_sbi.c | 2 +- > drivers/scsi/scsi_sysctl.c | 2 +- > drivers/scsi/sg.c | 2 +- > drivers/tty/tty_io.c | 2 +- > drivers/xen/balloon.c | 2 +- > fs/aio.c | 2 +- > fs/cachefiles/error_inject.c | 2 +- > fs/coda/sysctl.c | 2 +- > fs/coredump.c | 2 +- > fs/dcache.c | 2 +- > fs/devpts/inode.c | 2 +- > fs/eventpoll.c | 2 +- > fs/exec.c | 2 +- > fs/file_table.c | 2 +- > fs/fuse/sysctl.c | 2 +- > fs/inode.c | 2 +- > fs/lockd/svc.c | 2 +- > fs/locks.c | 2 +- > fs/namei.c | 2 +- > fs/namespace.c | 2 +- > fs/nfs/nfs4sysctl.c | 2 +- > fs/nfs/sysctl.c | 2 +- > fs/notify/dnotify/dnotify.c | 2 +- > fs/notify/fanotify/fanotify_user.c | 2 +- > fs/notify/inotify/inotify_user.c | 2 +- > fs/ocfs2/stackglue.c | 2 +- > fs/pipe.c | 2 +- > fs/quota/dquot.c | 2 +- > fs/sysctls.c | 2 +- > fs/userfaultfd.c | 2 +- > fs/verity/init.c | 2 +- > fs/xfs/xfs_sysctl.c | 2 +- > init/do_mounts_initrd.c | 2 +- > io_uring/io_uring.c | 2 +- > ipc/ipc_sysctl.c | 2 +- > ipc/mq_sysctl.c | 2 +- > kernel/acct.c | 2 +- > kernel/bpf/syscall.c | 2 +- > kernel/delayacct.c | 2 +- > kernel/exit.c | 2 +- > kernel/hung_task.c | 2 +- > kernel/kexec_core.c | 2 +- > kernel/kprobes.c | 2 +- > kernel/latencytop.c | 2 +- > kernel/locking/lockdep.c | 2 +- > kernel/panic.c | 2 +- > kernel/pid_namespace.c | 2 +- > kernel/pid_sysctl.h | 2 +- > kernel/printk/sysctl.c | 2 +- > kernel/reboot.c | 2 +- > kernel/sched/autogroup.c | 2 +- > kernel/sched/core.c | 2 +- > kernel/sched/deadline.c | 2 +- > kernel/sched/fair.c | 2 +- > kernel/sched/rt.c | 2 +- > kernel/sched/topology.c | 2 +- > kernel/seccomp.c | 2 +- > kernel/signal.c | 2 +- > kernel/stackleak.c | 2 +- > kernel/sysctl-test.c | 6 +++--- > kernel/sysctl.c | 4 ++-- > kernel/time/timer.c | 2 +- > kernel/trace/ftrace.c | 2 +- > kernel/trace/trace_events_user.c | 2 +- > kernel/umh.c | 2 +- > kernel/utsname_sysctl.c | 4 ++-- > kernel/watchdog.c | 4 ++-- > lib/alloc_tag.c | 2 +- > lib/test_sysctl.c | 6 +++--- > mm/compaction.c | 2 +- > mm/hugetlb.c | 2 +- > mm/hugetlb_vmemmap.c | 2 +- > mm/memory-failure.c | 2 +- > mm/oom_kill.c | 2 +- > mm/page-writeback.c | 2 +- > mm/page_alloc.c | 2 +- > security/apparmor/lsm.c | 2 +- > security/keys/sysctl.c | 2 +- > security/loadpin/loadpin.c | 2 +- > security/yama/yama_lsm.c | 2 +- > 111 files changed, 120 insertions(+), 120 deletions(-)