linux-kernel - Re: Re: [PATCH] treewide: const qualify ctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <77vb7o2mojeekus2opvh2roukdo3z6goizqalolpzxrzzxrt4h@ppm3utbuzbvy>
Date: Fri, 10 Jan 2025 15:12:40 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Kees Cook <kees@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH] treewide: const qualify ctl_tables where applicable

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@...nel.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?
There a bit more than I expected actually and they are not all in net/.
1. There are some infiniband files that call the register_net_sysctl_sz
2. following sysctl tables modify before registration
   watchdog_hardlockup_sysctl, memory_allocation_profiling_sysctls
   loadpin_sysctl_table, iwcm_ctl_table and ucma_ctl_table.

> 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):
Thx for the patch. I'll take these special cases at a later point. For
now, we are just consting the low hanging fruit.

> 
> 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)
>  {

...

> >  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(-)

-- 

Joel Granados