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
