| Message ID | 20231125-const-sysctl-v1-0-5e881b0e0290@weissschuh.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | sysctl: constify sysctl ctl_tables | expand |
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@weissschuh.net> >
Hi Joel, On 2023-11-27 11:13:23+0100, Joel Granados wrote: > In general I would like to see more clarity with the motivation and I > would also expect some system testing. My comments inline: Thanks for your feedback, response are below. > 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? There is no specific attack vector I have in mind. The goal is to remove mutable data, especially if it contains pointers, that could be used by an attacker as a step in an exploit. See for example [0], [1]. Accidental can be any out-of-bounds write throughout the kernel. > 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? It is still completely fine to modify the tables before registering, like sysctl_route_net_init is doing. That code should not need any changes. Modifying the table inside the handler function would bypass the validation done when registering so sounds like a bad idea in general. It would still be possible however for a subsystem to do so by just not making their sysctl table const and then modifying the table directly. > > 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? If the initial feedback to the RFC and general process is positive, yes. > > > > 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 > :) Hopefully somebody comes up with a better name! > 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. I managed to miss one change in my initial submission: With the hunk below selftests and typing emails work. --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header) else err |= sysctl_check_table_array(path, entry); } - if (!entry->proc_handler) + if (!entry->proc_handler && !entry->proc_handler_new) err |= sysctl_err(path, entry, "No proc_handler"); if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode) > [..] [0] 43a7206b0963 ("driver core: class: make class_register() take a const *") [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/ Thomas
Hey Thomas. Thx for the clarifications. I did more of a deep dive into your set and have additional comments (in line). I think const-ing all this is a good approach. The way forward is to be able to see the entire patch set of changes in a V1 or a shared repo somewhere to have a better picture of what is going on. By the "entire patchset" I mean all the changes that you described in the "full process". On Tue, Nov 28, 2023 at 09:18:30AM +0100, Thomas Weißschuh wrote: > Hi Joel, > > On 2023-11-27 11:13:23+0100, Joel Granados wrote: > > In general I would like to see more clarity with the motivation and I > > would also expect some system testing. My comments inline: > > Thanks for your feedback, response are below. > > > 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? > > There is no specific attack vector I have in mind. The goal is to remove > mutable data, especially if it contains pointers, that could be used by > an attacker as a step in an exploit. See for example [0], [1]. I think you should work "remove mutable data" as part of you main motivation when you send the non-RFC patch. I would also including [0] and [1] (and any other previous work) to help contextualize. > > Accidental can be any out-of-bounds write throughout the kernel. > > > 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? > > It is still completely fine to modify the tables before registering, > like sysctl_route_net_init is doing. That code should not need any > changes. > > Modifying the table inside the handler function would bypass the > validation done when registering so sounds like a bad idea in general. This is done before registering. So the approach *is* sound. > It would still be possible however for a subsystem to do so by just not > making their sysctl table const and then modifying the table directly. Indeed. Which might be intended or migth be someone that just forgets to put const. I think you mentioned that there would be some sort of static check for this (coccinelle or smach, or something else)? > > > > 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? Sorry, this was supposed to be "number 1", but you got my meaning :) > > If the initial feedback to the RFC and general process is positive, yes. Off the top of my head, putting that type in the header instead of the ctl_table seems ok. I would include it in non-RFC version together with 2. > > > > > > > Full process: > > > > > > * Introduce field proc_handler_new for const handlers (this series) I don't understand why we need a new handler. Couldn't we just change the existing handler to receive `const struct ctl_table` and change all the `proc_do*` handlers? I'm guessing its because you want to do this in steps? if that is the case, it would be very helpfull to see (in some repo or V1) the steps to change all the handlers in the non-RFC version > > > * 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. It would be helpful to see what the "big switch" would look like. If it is all sysctl code and cannot be chunked up because of dependencies, then it should be ok to do it in one go. > > > * 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. Have you considered how to ignore the cases where the ctl_tables are supposed to be non-const when they are defined (like in the network code that we were discussing earlier) > > > * Migrate definitions of "struct ctl_table" to "const" where applicable. These migrations are treewide and are usually reviewed by a wider audience. You might need to chunk it up to make the review more palpable for the other maintainers. > > > > > > > > > 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 > > :) > > Hopefully somebody comes up with a better name! I would like to avoid this all together and just do add the const to the existing "proc_handler" > > > 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. > > I managed to miss one change in my initial submission: > With the hunk below selftests and typing emails work. > > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header) > else > err |= sysctl_check_table_array(path, entry); > } > - if (!entry->proc_handler) > + if (!entry->proc_handler && !entry->proc_handler_new) > err |= sysctl_err(path, entry, "No proc_handler"); > > if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode) > > > [..] > > [0] 43a7206b0963 ("driver core: class: make class_register() take a const *") > [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/ > > > Thomas
Hi Joel, On 2023-12-01 17:31:20+0100, Joel Granados wrote: > Hey Thomas. > > Thx for the clarifications. I did more of a deep dive into your set and > have additional comments (in line). I think const-ing all this is a good > approach. The way forward is to be able to see the entire patch set of > changes in a V1 or a shared repo somewhere to have a better picture of > what is going on. By the "entire patchset" I mean all the changes that > you described in the "full process". All the changes will be a lot. I don't think the incremental value to migrate all proc_handlers versus the work is useful for the discussion. I can however write up my proposed changes for the sysctl core properly and submit them as part of the next revision. > On Tue, Nov 28, 2023 at 09:18:30AM +0100, Thomas Weißschuh wrote: > > Hi Joel, > > > > On 2023-11-27 11:13:23+0100, Joel Granados wrote: > > > In general I would like to see more clarity with the motivation and I > > > would also expect some system testing. My comments inline: > > > > Thanks for your feedback, response are below. > > > > > 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? > > > > There is no specific attack vector I have in mind. The goal is to remove > > mutable data, especially if it contains pointers, that could be used by > > an attacker as a step in an exploit. See for example [0], [1]. > I think you should work "remove mutable data" as part of you main > motivation when you send the non-RFC patch. I would also including [0] > and [1] (and any other previous work) to help contextualize. Ack. > > > > > Accidental can be any out-of-bounds write throughout the kernel. > > > > > 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? > > > > It is still completely fine to modify the tables before registering, > > like sysctl_route_net_init is doing. That code should not need any > > changes. > > > > Modifying the table inside the handler function would bypass the > > validation done when registering so sounds like a bad idea in general. > This is done before registering. So the approach *is* sound. Absolutely. Though, I wouldn't be surprised if some other subsystem is doing this stuff in the handler. > > It would still be possible however for a subsystem to do so by just not > > making their sysctl table const and then modifying the table directly. > Indeed. Which might be intended or migth be someone that just forgets to > put const. I think you mentioned that there would be some sort of static > check for this (coccinelle or smach, or something else)? My intention was to put the struct into scripts/const_structs.checkpatch so checkpatch.pl warns about non-const instances. > > > > > > 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? > Sorry, this was supposed to be "number 1", but you got my meaning :) I was not entirely sure :-) As mentioned above I do have a proposal for 1) and will submit this as part of the next revision. Or maybe as a standalone non-RFC patchset, because IMHO this is valuable on its own. > > > > If the initial feedback to the RFC and general process is positive, yes. > Off the top of my head, putting that type in the header instead of the > ctl_table seems ok. I would include it in non-RFC version together with > 2. > > > > > > > > > > > Full process: > > > > > > > > * Introduce field proc_handler_new for const handlers (this series) > I don't understand why we need a new handler. Couldn't we just change > the existing handler to receive `const struct ctl_table` and change all > the `proc_do*` handlers? The idea was that there are a lot of nonstandard proc handlers. By doing it in steps we would avoid having to change all nonstandard handlers in one go. I looked a bit around and it seems that only 20% of sysctls use nonstandard handlers. Let's see if it's feasible to do those in one step. It would indeed avoid a bunch of complexity all over the place. > I'm guessing its because you want to do this in steps? if that is the > case, it would be very helpfull to see (in some repo or V1) the steps > to change all the handlers in the non-RFC version > > > > > * 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. > It would be helpful to see what the "big switch" would look like. If it > is all sysctl code and cannot be chunked up because of dependencies, > then it should be ok to do it in one go. > > > > > * 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. > Have you considered how to ignore the cases where the ctl_tables are > supposed to be non-const when they are defined (like in the network > code that we were discussing earlier) As it would be a checkpatch warning it can be ignore while writing the patch and it won't trigger afterwards. > > > > * Migrate definitions of "struct ctl_table" to "const" where applicable. > These migrations are treewide and are usually reviewed by a wider > audience. You might need to chunk it up to make the review more palpable > for the other maintainers. Ack. > > > > > > > > > > > > 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 > > > :) > > > > Hopefully somebody comes up with a better name! > I would like to avoid this all together and just do add the const to the > existing "proc_handler" Ack. > > > > > 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. > > > > I managed to miss one change in my initial submission: > > With the hunk below selftests and typing emails work. > > > > --- a/fs/proc/proc_sysctl.c > > +++ b/fs/proc/proc_sysctl.c > > @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header) > > else > > err |= sysctl_check_table_array(path, entry); > > } > > - if (!entry->proc_handler) > > + if (!entry->proc_handler && !entry->proc_handler_new) > > err |= sysctl_err(path, entry, "No proc_handler"); > > > > if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode) > > > > > [..] > > > > [0] 43a7206b0963 ("driver core: class: make class_register() take a const *") > > [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/ Thanks for the feedback! Thomas
Hey I see that you sent a V2. I'll try to get to it at the end of the week. On Sun, Dec 03, 2023 at 04:37:01PM +0100, Thomas Weißschuh wrote: > Hi Joel, > > On 2023-12-01 17:31:20+0100, Joel Granados wrote: > > Hey Thomas. > > > > Thx for the clarifications. I did more of a deep dive into your set and > > have additional comments (in line). I think const-ing all this is a good > > approach. The way forward is to be able to see the entire patch set of > > changes in a V1 or a shared repo somewhere to have a better picture of > > what is going on. By the "entire patchset" I mean all the changes that > > you described in the "full process". > > All the changes will be a lot. I don't think the incremental value to > migrate all proc_handlers versus the work is useful for the discussion. > I can however write up my proposed changes for the sysctl core properly > and submit them as part of the next revision. Looking forward to seeing them in V2 > > > On Tue, Nov 28, 2023 at 09:18:30AM +0100, Thomas Weißschuh wrote: > > > Hi Joel, > > > > > > On 2023-11-27 11:13:23+0100, Joel Granados wrote: > > > > In general I would like to see more clarity with the motivation and I > > > > would also expect some system testing. My comments inline: <--- snip ---> > > is all sysctl code and cannot be chunked up because of dependencies, > > then it should be ok to do it in one go. > > > > > > > * 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. > > > Have you considered how to ignore the cases where the ctl_tables are > > supposed to be non-const when they are defined (like in the network > > code that we were discussing earlier) > > As it would be a checkpatch warning it can be ignore while writing the > patch and it won't trigger afterwards. I mention coccinelle it is able to identify const vs non-const uses of the ctl_table and only warn on the cases where it makes sense. This would remove false negatives from pushing patches through. > > > > > > * Migrate definitions of "struct ctl_table" to "const" where applicable. > > These migrations are treewide and are usually reviewed by a wider > > audience. You might need to chunk it up to make the review more palpable > > for the other maintainers. > > Ack. > > > > > > > > > > > > > > > > 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 > > > > :) > > > > > > Hopefully somebody comes up with a better name! > > > I would like to avoid this all together and just do add the const to the > > existing "proc_handler" > > Ack. > > > > > > > > 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. > > > > > > I managed to miss one change in my initial submission: > > > With the hunk below selftests and typing emails work. > > > > > > --- a/fs/proc/proc_sysctl.c > > > +++ b/fs/proc/proc_sysctl.c > > > @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header) > > > else > > > err |= sysctl_check_table_array(path, entry); > > > } > > > - if (!entry->proc_handler) > > > + if (!entry->proc_handler && !entry->proc_handler_new) > > > err |= sysctl_err(path, entry, "No proc_handler"); > > > > > > if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode) > > > > > > > [..] > > > > > > [0] 43a7206b0963 ("driver core: class: make class_register() take a const *") > > > [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/ > > Thanks for the feedback! > > Thomas
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. 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). 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. --- 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,