Re: [PATCH] treewide: const qualify ctl_tables where applicable

[Kees Cook <kees@kernel.org>]

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 <joel.granados@kernel.org>
> > ---
> > 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