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,
--
Thomas Weißschuh <[email protected]>
proc_handler_new() prevents the handler function from modifying the
ctl_table which then can be put into .rodata.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
crypto/fips.c | 4 ++--
fs/coredump.c | 2 +-
fs/ocfs2/stackglue.c | 2 +-
fs/proc/proc_sysctl.c | 2 +-
include/linux/sysctl.h | 2 +-
kernel/reboot.c | 2 +-
kernel/seccomp.c | 2 +-
kernel/sysctl.c | 14 +++++++-------
lib/test_sysctl.c | 2 +-
net/mptcp/ctrl.c | 2 +-
10 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/crypto/fips.c b/crypto/fips.c
index 92fd506abb21..d492f23bf53b 100644
--- a/crypto/fips.c
+++ b/crypto/fips.c
@@ -54,14 +54,14 @@ static struct ctl_table crypto_sysctl_table[] = {
.data = &fips_name,
.maxlen = 64,
.mode = 0444,
- .proc_handler = proc_dostring
+ .proc_handler_new = proc_dostring
},
{
.procname = "fips_version",
.data = &fips_version,
.maxlen = 64,
.mode = 0444,
- .proc_handler = proc_dostring
+ .proc_handler_new = proc_dostring
},
{}
};
diff --git a/fs/coredump.c b/fs/coredump.c
index 9d235fa14ab9..733cb795f678 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -972,7 +972,7 @@ static struct ctl_table coredump_sysctls[] = {
.data = core_pattern,
.maxlen = CORENAME_MAX_SIZE,
.mode = 0644,
- .proc_handler = proc_dostring_coredump,
+ .proc_handler_new = proc_dostring_coredump,
},
{
.procname = "core_pipe_limit",
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index a8d5ca98fa57..e4eedd1d6b7d 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -656,7 +656,7 @@ static struct ctl_table ocfs2_nm_table[] = {
.data = ocfs2_hb_ctl_path,
.maxlen = OCFS2_MAX_HB_CTL_PATH,
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{ }
};
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 810ecdd3b84c..0817d315fa36 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1132,7 +1132,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header)
struct ctl_table *entry;
int err = 0;
list_for_each_table_entry(entry, header) {
- if ((entry->proc_handler == proc_dostring) ||
+ if ((entry->proc_handler_new == proc_dostring) ||
(entry->proc_handler == proc_dobool) ||
(entry->proc_handler == proc_dointvec) ||
(entry->proc_handler == proc_douintvec) ||
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index de1a5a714070..2699605c5da5 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -66,7 +66,7 @@ typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer,
typedef int proc_handler_new(const struct ctl_table *ctl, int write,
void *buffer, size_t *lenp, loff_t *ppos);
-int proc_dostring(struct ctl_table *, int, void *, size_t *, loff_t *);
+int proc_dostring(const struct ctl_table *, int, void *, size_t *, loff_t *);
int proc_dobool(struct ctl_table *table, int write, void *buffer,
size_t *lenp, loff_t *ppos);
int proc_dointvec(struct ctl_table *, int, void *, size_t *, loff_t *);
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 395a0ea3c7a8..69681100d884 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -1267,7 +1267,7 @@ static struct ctl_table kern_reboot_table[] = {
.data = &poweroff_cmd,
.maxlen = POWEROFF_CMD_PATH_LEN,
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{
.procname = "ctrl-alt-del",
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 255999ba9190..29e9663cf220 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -2438,7 +2438,7 @@ static struct ctl_table seccomp_sysctl_table[] = {
.data = (void *) &seccomp_actions_avail,
.maxlen = sizeof(seccomp_actions_avail),
.mode = 0444,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{
.procname = "actions_logged",
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 157f7ce2942d..7acd1cde0a5c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -205,7 +205,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
return 0;
}
-static void warn_sysctl_write(struct ctl_table *table)
+static void warn_sysctl_write(const struct ctl_table *table)
{
pr_warn_once("%s wrote to %s when file position was not 0!\n"
"This will not be supported in the future. To silence this\n"
@@ -223,7 +223,7 @@ static void warn_sysctl_write(struct ctl_table *table)
* handlers can ignore the return value.
*/
static bool proc_first_pos_non_zero_ignore(loff_t *ppos,
- struct ctl_table *table)
+ const struct ctl_table *table)
{
if (!*ppos)
return false;
@@ -256,7 +256,7 @@ static bool proc_first_pos_non_zero_ignore(loff_t *ppos,
*
* Returns 0 on success.
*/
-int proc_dostring(struct ctl_table *table, int write,
+int proc_dostring(const struct ctl_table *table, int write,
void *buffer, size_t *lenp, loff_t *ppos)
{
if (write)
@@ -1498,7 +1498,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
#else /* CONFIG_PROC_SYSCTL */
-int proc_dostring(struct ctl_table *table, int write,
+int proc_dostring(const struct ctl_table *table, int write,
void *buffer, size_t *lenp, loff_t *ppos)
{
return -ENOSYS;
@@ -1653,7 +1653,7 @@ static struct ctl_table kern_table[] = {
.data = reboot_command,
.maxlen = 256,
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{
.procname = "stop-a",
@@ -1735,7 +1735,7 @@ static struct ctl_table kern_table[] = {
.data = &modprobe_path,
.maxlen = KMOD_PATH_LEN,
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{
.procname = "modules_disabled",
@@ -1754,7 +1754,7 @@ static struct ctl_table kern_table[] = {
.data = &uevent_helper,
.maxlen = UEVENT_HELPER_PATH_LEN,
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
#endif
#ifdef CONFIG_MAGIC_SYSRQ
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 8036aa91a1cb..de42e3d99912 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -121,7 +121,7 @@ static struct ctl_table test_table[] = {
.data = &test_data.string_0001,
.maxlen = sizeof(test_data.string_0001),
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{
.procname = "bitmap_0001",
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 13fe0748dde8..6de3178b81b4 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -148,7 +148,7 @@ static struct ctl_table mptcp_sysctl_table[] = {
.procname = "scheduler",
.maxlen = MPTCP_SCHED_NAME_MAX,
.mode = 0644,
- .proc_handler = proc_dostring,
+ .proc_handler_new = proc_dostring,
},
{
.procname = "close_timeout",
--
2.43.0
The sysctl core introduce an alternative handler function.
Log it in the debug message, too.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
net/sysctl_net.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 051ed5f6fc93..29d871097ddc 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -132,8 +132,9 @@ static void ensure_safe_net_sysctl(struct net *net, const char *path,
unsigned long addr;
const char *where;
- pr_debug(" procname=%s mode=%o proc_handler=%ps data=%p\n",
- ent->procname, ent->mode, ent->proc_handler, ent->data);
+ pr_debug(" procname=%s mode=%o proc_handler=%ps/%ps data=%p\n",
+ ent->procname, ent->mode, ent->proc_handler,
+ ent->proc_handler_new, ent->data);
/* If it's not writable inside the netns, then it can't hurt. */
if ((ent->mode & 0222) == 0) {
--
2.43.0
proc_handler_new() prevents the handler function from modifying the
ctl_table which then can be put into .rodata.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
arch/riscv/kernel/vector.c | 2 +-
drivers/tty/tty_io.c | 2 +-
fs/lockd/svc.c | 2 +-
fs/proc/proc_sysctl.c | 4 ++--
include/linux/sysctl.h | 2 +-
kernel/sysctl.c | 4 ++--
mm/hugetlb_vmemmap.c | 2 +-
7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
index 578b6292487e..d2a37fe88174 100644
--- a/arch/riscv/kernel/vector.c
+++ b/arch/riscv/kernel/vector.c
@@ -253,7 +253,7 @@ static struct ctl_table riscv_v_default_vstate_table[] = {
.data = &riscv_v_implicit_uacc,
.maxlen = sizeof(riscv_v_implicit_uacc),
.mode = 0644,
- .proc_handler = proc_dobool,
+ .proc_handler_new = proc_dobool,
},
};
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 06414e43e0b5..a7bcc22fdae9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3601,7 +3601,7 @@ static struct ctl_table tty_table[] = {
.data = &tty_legacy_tiocsti,
.maxlen = sizeof(tty_legacy_tiocsti),
.mode = 0644,
- .proc_handler = proc_dobool,
+ .proc_handler_new = proc_dobool,
},
{
.procname = "ldisc_autoload",
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 81be07c1d3d1..90ea8cd382d3 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -466,7 +466,7 @@ static struct ctl_table nlm_sysctls[] = {
.data = &nsm_use_hostnames,
.maxlen = sizeof(bool),
.mode = 0644,
- .proc_handler = proc_dobool,
+ .proc_handler_new = proc_dobool,
},
{
.procname = "nsm_local_state",
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 0817d315fa36..742a99540f2b 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1119,7 +1119,7 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
err |= sysctl_err(path, table, "array not allowed");
}
- if (table->proc_handler == proc_dobool) {
+ if (table->proc_handler_new == proc_dobool) {
if (table->maxlen != sizeof(bool))
err |= sysctl_err(path, table, "array not allowed");
}
@@ -1133,7 +1133,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header)
int err = 0;
list_for_each_table_entry(entry, header) {
if ((entry->proc_handler_new == proc_dostring) ||
- (entry->proc_handler == proc_dobool) ||
+ (entry->proc_handler_new == proc_dobool) ||
(entry->proc_handler == proc_dointvec) ||
(entry->proc_handler == proc_douintvec) ||
(entry->proc_handler == proc_douintvec_minmax) ||
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 2699605c5da5..2dfaf718a21b 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -67,7 +67,7 @@ typedef int proc_handler_new(const struct ctl_table *ctl, int write,
void *buffer, size_t *lenp, loff_t *ppos);
int proc_dostring(const struct ctl_table *, int, void *, size_t *, loff_t *);
-int proc_dobool(struct ctl_table *table, int write, void *buffer,
+int proc_dobool(const struct ctl_table *table, int write, void *buffer,
size_t *lenp, loff_t *ppos);
int proc_dointvec(struct ctl_table *, int, void *, size_t *, loff_t *);
int proc_douintvec(struct ctl_table *, int, void *, size_t *, loff_t *);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 7acd1cde0a5c..c76668f47bcc 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -702,7 +702,7 @@ int do_proc_douintvec(struct ctl_table *table, int write,
*
* Returns 0 on success.
*/
-int proc_dobool(struct ctl_table *table, int write, void *buffer,
+int proc_dobool(const struct ctl_table *table, int write, void *buffer,
size_t *lenp, loff_t *ppos)
{
struct ctl_table tmp;
@@ -1504,7 +1504,7 @@ int proc_dostring(const struct ctl_table *table, int write,
return -ENOSYS;
}
-int proc_dobool(struct ctl_table *table, int write,
+int proc_dobool(const struct ctl_table *table, int write,
void *buffer, size_t *lenp, loff_t *ppos)
{
return -ENOSYS;
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 87818ee7f01d..e61e9fbfd639 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -779,7 +779,7 @@ static struct ctl_table hugetlb_vmemmap_sysctls[] = {
.data = &vmemmap_optimize_enabled,
.maxlen = sizeof(vmemmap_optimize_enabled),
.mode = 0644,
- .proc_handler = proc_dobool,
+ .proc_handler_new = proc_dobool,
},
{ }
};
--
2.43.0
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 <[email protected]>
>
--
Joel Granados
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/[email protected]/
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/[email protected]/
>
>
> Thomas
--
Best
Joel Granados
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/[email protected]/
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/[email protected]/
>
> Thanks for the feedback!
>
> Thomas
--
Joel Granados