From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 617AD18595F for ; Thu, 9 Jan 2025 16:55:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736441732; cv=none; b=VhEF1Bf8u4sQklcfEpC7F9pDJML5tn+zd2JLw3QQGgUl61/h0gz68MrB6JqMZBA0YlIbjiqa/IdTNhWyl5nP9hQFH71PVTOYDneA1gQi2SdBr0awCzskTzF6hyLgerFFe0aH4LVgZtYbChiQeXP04yloeYh3qbkkxTBPXhNYHQc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736441732; c=relaxed/simple; bh=2GuQOQMKunsf75baLVoKYxBlCnbOu7EVFA004pOsbrM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=KRWHdPO3MPnHJLW7kOzmEM5NoWusF1KEyt0/zrLZrQ0yGXqMwXGNlpmwzTERCAnptMaFmJAQSEVtNoYdq0Xw/Lu3qQ33nesXRvuWGVhJTf24oJuAMhFvxA2MKZi6zDHsSB6l4uoytWez0kYFkja3lgl9F825R2v2JrQgfymXaa0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=cYmCYcQQ; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cYmCYcQQ" Received: by smtp.kernel.org (Postfix) with ESMTPSA id DBF5CC4CED2; Thu, 9 Jan 2025 16:55:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736441730; bh=2GuQOQMKunsf75baLVoKYxBlCnbOu7EVFA004pOsbrM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=cYmCYcQQSK9Y/Ox9JaJWCggv0kzpkgXCbvwj9F15OC311+iwC2Uzji/ghS93ATFaL aHInDf6qUZ0djxWg23zC9x4pyVH5mQgr1n6em8gyvJ91WX4KAlxT0FNXM7HBrR9WZs XkL5OXHFSCSNKPE8xA4ocGXRtHa/trmYtMp1XhZFVduEEChkfJPiQCUO/xYM1QB4XK hnqUP71uCt5eO2J89NAYeOXgkK1MEVq8jugnZ1hmdVe+EN97MF3mPJBSkro60wkqHg k4vFMENxx+jpmRvfCneaZgY+p3fJgdnGljglbSw3NkKhbfT/XkFZPik6FNgD4j+Tyl EA5wARXRCV5qw== Date: Thu, 9 Jan 2025 08:55:27 -0800 From: Kees Cook To: Thomas =?iso-8859-1?Q?Wei=DFschuh?= Cc: Joel Granados , Luis Chamberlain , linux-kernel@vger.kernel.org Subject: Re: [PATCH] treewide: const qualify ctl_tables where applicable Message-ID: <202501090854.40D19AD31@keescook> References: <20250109-jag-ctl_table_const-v1-1-622aea7230cf@kernel.org> <73f3c35b-35f1-433e-bd67-b428c3f2dd6e@t-8ch.de> 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=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <73f3c35b-35f1-433e-bd67-b428c3f2dd6e@t-8ch.de> On Thu, Jan 09, 2025 at 04:25:06PM +0100, Thomas Weißschuh wrote: > (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; > } Yeah, I like this solution. Perhaps update the WARN to say the sysctl is being ignored? -- Kees Cook