Re: [PATCH RFC 0/7] sysctl: constify sysctl ctl_tables

[Joel Granados]

Hey Thomas

In general I would like to see more clarity with the motivation and I
would also expect some system testing. My comments inline:

On Sat, Nov 25, 2023 at 01:52:49PM +0100, Thomas Weißschuh wrote:
> Problem description:
> 
> The kernel contains a lot of struct ctl_table throught the tree.
> These are very often 'static' definitions.
> It would be good to mark these tables const to avoid accidental or
> malicious modifications.
It is unclear to me what you mean here with accidental or malicious
modifications. Do you have a specific attack vector in mind? Do you
have an example of how this could happen maliciously? With
accidental, do you mean in proc/sysctl.c? Can you expand more on the
accidental part?

What happens with the code that modifies these outside the sysctl core?
Like for example in sysctl_route_net_init where the table is modified
depending on the net->user_ns? Would these non-const ctl_table pointers
be ok? would they be handled differently?

> Unfortunately the tables can not be made const because the core
> registration functions expect mutable tables.
> 
> This is for two reasons:
> 
> 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify
>    the table. This should be fixable by only modifying the header
>    instead of the table itself.
> 2) The table is passed to the handler function as a non-const pointer.
> 
> This series is an aproach on fixing reason 2).
So number 2 will be sent in another set?

> 
> Full process:
> 
> * Introduce field proc_handler_new for const handlers (this series)
> * Migrate all core handlers to proc_handler_new (this series, partial)
>   This can hopefully be done in a big switch, as it only involves
>   functions and structures owned by the core sysctl code.
> * Migrate all other sysctl handlers to proc_handler_new.
> * Drop the old proc_handler_field.
> * Fix the sysctl core to not modify the tables anymore.
> * Adapt public sysctl APIs to take "const struct ctl_table *".
> * Teach checkpatch.pl to warn on non-const "struct ctl_table"
>   definitions.
> * Migrate definitions of "struct ctl_table" to "const" where applicable.
>  
> 
> Notes:
> 
> Just casting the function pointers around would trigger
> CFI (control flow integrity) warnings.
> 
> The name of the new handler "proc_handler_new" is a bit too long messing
> up the alignment of the table definitions.
> Maybe "proc_handler2" or "proc_handler_c" for (const) would be better.
indeed the name does not say much. "_new" looses its meaning quite fast
:)

In my experience these tree wide modifications are quite tricky. Have you
run any tests to see that everything is as it was? sysctl selftests and
0-day come to mind.

Best
> 
> ---
> Thomas Weißschuh (7):
>       sysctl: add helper sysctl_run_handler
>       bpf: cgroup: call proc handler through helper
>       sysctl: add proc_handler_new to struct ctl_table
>       net: sysctl: add new sysctl table handler to debug message
>       treewide: sysctl: migrate proc_dostring to proc_handler_new
>       treewide: sysctl: migrate proc_dobool to proc_handler_new
>       treewide: sysctl: migrate proc_dointvec to proc_handler_new
> 
>  arch/arm/kernel/isa.c                   |  6 +--
>  arch/csky/abiv1/alignment.c             |  8 ++--
>  arch/powerpc/kernel/idle.c              |  2 +-
>  arch/riscv/kernel/vector.c              |  2 +-
>  arch/s390/kernel/debug.c                |  2 +-
>  crypto/fips.c                           |  6 +--
>  drivers/char/hpet.c                     |  2 +-
>  drivers/char/random.c                   |  4 +-
>  drivers/infiniband/core/iwcm.c          |  2 +-
>  drivers/infiniband/core/ucma.c          |  2 +-
>  drivers/macintosh/mac_hid.c             |  4 +-
>  drivers/md/md.c                         |  4 +-
>  drivers/scsi/sg.c                       |  2 +-
>  drivers/tty/tty_io.c                    |  4 +-
>  fs/coda/sysctl.c                        |  6 +--
>  fs/coredump.c                           |  6 +--
>  fs/devpts/inode.c                       |  2 +-
>  fs/lockd/svc.c                          |  4 +-
>  fs/locks.c                              |  4 +-
>  fs/nfs/nfs4sysctl.c                     |  2 +-
>  fs/nfs/sysctl.c                         |  2 +-
>  fs/notify/dnotify/dnotify.c             |  2 +-
>  fs/ntfs/sysctl.c                        |  2 +-
>  fs/ocfs2/stackglue.c                    |  2 +-
>  fs/proc/proc_sysctl.c                   | 16 ++++---
>  fs/quota/dquot.c                        |  2 +-
>  include/linux/sysctl.h                  | 29 +++++++++---
>  init/do_mounts_initrd.c                 |  2 +-
>  io_uring/io_uring.c                     |  2 +-
>  ipc/mq_sysctl.c                         |  2 +-
>  kernel/acct.c                           |  2 +-
>  kernel/bpf/cgroup.c                     |  2 +-
>  kernel/locking/lockdep.c                |  4 +-
>  kernel/printk/sysctl.c                  |  4 +-
>  kernel/reboot.c                         |  4 +-
>  kernel/seccomp.c                        |  2 +-
>  kernel/signal.c                         |  2 +-
>  kernel/sysctl-test.c                    | 20 ++++-----
>  kernel/sysctl.c                         | 80 ++++++++++++++++-----------------
>  lib/test_sysctl.c                       | 10 ++---
>  mm/hugetlb.c                            |  2 +-
>  mm/hugetlb_vmemmap.c                    |  2 +-
>  mm/oom_kill.c                           |  4 +-
>  net/appletalk/sysctl_net_atalk.c        |  2 +-
>  net/core/sysctl_net_core.c              | 12 ++---
>  net/ipv4/route.c                        | 18 ++++----
>  net/ipv4/sysctl_net_ipv4.c              | 38 ++++++++--------
>  net/ipv4/xfrm4_policy.c                 |  2 +-
>  net/ipv6/addrconf.c                     | 72 ++++++++++++++---------------
>  net/ipv6/route.c                        |  8 ++--
>  net/ipv6/sysctl_net_ipv6.c              | 18 ++++----
>  net/ipv6/xfrm6_policy.c                 |  2 +-
>  net/mptcp/ctrl.c                        |  2 +-
>  net/netfilter/ipvs/ip_vs_ctl.c          | 36 +++++++--------
>  net/netfilter/nf_conntrack_standalone.c |  8 ++--
>  net/netfilter/nf_log.c                  |  2 +-
>  net/rds/ib_sysctl.c                     |  2 +-
>  net/rds/sysctl.c                        |  6 +--
>  net/sctp/sysctl.c                       | 26 +++++------
>  net/sunrpc/xprtrdma/transport.c         |  2 +-
>  net/sysctl_net.c                        |  5 ++-
>  net/unix/sysctl_net_unix.c              |  2 +-
>  net/x25/sysctl_net_x25.c                |  2 +-
>  net/xfrm/xfrm_sysctl.c                  |  4 +-
>  64 files changed, 280 insertions(+), 262 deletions(-)
> ---
> base-commit: 0f5cc96c367f2e780eb492cc9cab84e3b2ca88da
> change-id: 20231116-const-sysctl-e14624f1295c
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> 

-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature