Sysctl handlers are not supposed to modify the ctl_table passed to them. Adapt the logic to work with a temporary variable, similar to how it is done in other parts of the kernel.
This is also a prerequisite to enforce the immutability of the argument through the callbacks prototy.
Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing") Cc: stable@vger.kernel.org Acked-by: Kees Cook keescook@chromium.org Reviewed-by: Luis Chamberlain mcgrof@kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- This was split out of my sysctl-const-handler series [0].
As that series will take some more time, submit the patch on its own, as it is a generic bugfix that is valuable on its own. And I can get it out of my books.
Changelog in contrast to the patch in the series: * Reword commit message to remove strong relation to the constification * Cc stable
[0] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-0-e0beccb836e2...
Cc: Joel Granados j.granados@samsung.com --- kernel/stackleak.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 34c9d81eea94..b292e5ca0b7d 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -27,10 +27,11 @@ static int stack_erasing_sysctl(struct ctl_table *table, int write, int ret = 0; int state = !static_branch_unlikely(&stack_erasing_bypass); int prev_state = state; + struct ctl_table tmp = *table;
- table->data = &state; - table->maxlen = sizeof(int); - ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); + tmp.data = &state; + tmp.maxlen = sizeof(int); + ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos); state = !!state; if (ret || !write || state == prev_state) return ret;
--- base-commit: f03359bca01bf4372cf2c118cd9a987a5951b1c8 change-id: 20240503-sysctl-const-stackleak-af3e67bc65b0
Best regards,
On Fri, May 03, 2024 at 03:44:09PM +0200, Thomas Weißschuh wrote:
Sysctl handlers are not supposed to modify the ctl_table passed to them. Adapt the logic to work with a temporary variable, similar to how it is done in other parts of the kernel.
This is also a prerequisite to enforce the immutability of the argument through the callbacks prototy.
Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
Reviewed-by: Tycho Andersen tycho@tycho.pizza
Tycho
On Fri, May 03, 2024 at 03:44:09PM +0200, Thomas Weißschuh wrote:
Sysctl handlers are not supposed to modify the ctl_table passed to them. Adapt the logic to work with a temporary variable, similar to how it is done in other parts of the kernel.
This is also a prerequisite to enforce the immutability of the argument through the callbacks prototy.
Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing") Cc: stable@vger.kernel.org
I realize I've already Acked, but does this actually need to be CCed to stable?
Acked-by: Kees Cook keescook@chromium.org Reviewed-by: Luis Chamberlain mcgrof@kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This was split out of my sysctl-const-handler series [0].
As that series will take some more time, submit the patch on its own, as it is a generic bugfix that is valuable on its own. And I can get it out of my books.
Changelog in contrast to the patch in the series:
- Reword commit message to remove strong relation to the constification
- Cc stable
[0] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-0-e0beccb836e2...
Cc: Joel Granados j.granados@samsung.com
kernel/stackleak.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 34c9d81eea94..b292e5ca0b7d 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -27,10 +27,11 @@ static int stack_erasing_sysctl(struct ctl_table *table, int write, int ret = 0; int state = !static_branch_unlikely(&stack_erasing_bypass); int prev_state = state;
- struct ctl_table tmp = *table;
- table->data = &state;
- table->maxlen = sizeof(int);
- ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
- tmp.data = &state;
- tmp.maxlen = sizeof(int);
- ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos); state = !!state; if (ret || !write || state == prev_state) return ret;
I can pick this up; thanks!
-Kees
base-commit: f03359bca01bf4372cf2c118cd9a987a5951b1c8 change-id: 20240503-sysctl-const-stackleak-af3e67bc65b0
Best regards,
Thomas Weißschuh linux@weissschuh.net
May 3, 2024 19:55:37 Kees Cook keescook@chromium.org:
On Fri, May 03, 2024 at 03:44:09PM +0200, Thomas Weißschuh wrote:
Sysctl handlers are not supposed to modify the ctl_table passed to them. Adapt the logic to work with a temporary variable, similar to how it is done in other parts of the kernel.
This is also a prerequisite to enforce the immutability of the argument through the callbacks prototy.
Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing") Cc: stable@vger.kernel.org
I realize I've already Acked, but does this actually need to be CCed to stable?
You acked it without the Cc stable. I shouldn't have kept your Ack, sorry.
Feel free to drop the Cc, it shouldn't be critical. I suspect the bots will pick it up anyways.
Acked-by: Kees Cook keescook@chromium.org Reviewed-by: Luis Chamberlain mcgrof@kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This was split out of my sysctl-const-handler series [0].
As that series will take some more time, submit the patch on its own, as it is a generic bugfix that is valuable on its own. And I can get it out of my books.
Changelog in contrast to the patch in the series:
- Reword commit message to remove strong relation to the constification
- Cc stable
[0] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-0-e0beccb836e2...
Cc: Joel Granados j.granados@samsung.com
kernel/stackleak.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 34c9d81eea94..b292e5ca0b7d 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -27,10 +27,11 @@ static int stack_erasing_sysctl(struct ctl_table *table, int write, int ret = 0; int state = !static_branch_unlikely(&stack_erasing_bypass); int prev_state = state; + struct ctl_table tmp = *table;
- table->data = &state; - table->maxlen = sizeof(int); - ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); + tmp.data = &state; + tmp.maxlen = sizeof(int); + ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos); state = !!state; if (ret || !write || state == prev_state) return ret;
I can pick this up; thanks!
Thanks!
On Fri, May 03, 2024 at 03:44:09PM +0200, Thomas Weißschuh wrote:
Sysctl handlers are not supposed to modify the ctl_table passed to them. Adapt the logic to work with a temporary variable, similar to how it is done in other parts of the kernel.
This is also a prerequisite to enforce the immutability of the argument through the callbacks prototy.
^^^^^^^
Was this supposed to be "prototype"? I couldn't quite figure out what was meant there; the sentence reads fine to me without the word there at all. :)
Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing") Cc: stable@vger.kernel.org Acked-by: Kees Cook keescook@chromium.org Reviewed-by: Luis Chamberlain mcgrof@kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This was split out of my sysctl-const-handler series [0].
As that series will take some more time, submit the patch on its own, as it is a generic bugfix that is valuable on its own. And I can get it out of my books.
Changelog in contrast to the patch in the series:
- Reword commit message to remove strong relation to the constification
- Cc stable
[0] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-0-e0beccb836e2...
Cc: Joel Granados j.granados@samsung.com
kernel/stackleak.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 34c9d81eea94..b292e5ca0b7d 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -27,10 +27,11 @@ static int stack_erasing_sysctl(struct ctl_table *table, int write, int ret = 0; int state = !static_branch_unlikely(&stack_erasing_bypass); int prev_state = state;
- struct ctl_table tmp = *table;
- table->data = &state;
- table->maxlen = sizeof(int);
- ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
- tmp.data = &state;
- tmp.maxlen = sizeof(int);
- ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
In looking at this yet again, I can't figure out why maxlen is being set. It starts its life as sizeof(int):
static struct ctl_table stackleak_sysctls[] = { { .procname = "stack_erasing", .data = NULL, .maxlen = sizeof(int),
-Kees
state = !!state; if (ret || !write || state == prev_state) return ret;
base-commit: f03359bca01bf4372cf2c118cd9a987a5951b1c8 change-id: 20240503-sysctl-const-stackleak-af3e67bc65b0
Best regards,
Thomas Weißschuh linux@weissschuh.net
On Fri, 03 May 2024 15:44:09 +0200, Thomas Weißschuh wrote:
Sysctl handlers are not supposed to modify the ctl_table passed to them. Adapt the logic to work with a temporary variable, similar to how it is done in other parts of the kernel.
This is also a prerequisite to enforce the immutability of the argument through the callbacks prototy.
[...]
Applied to for-next/hardening, thanks!
[1/1] stackleak: don't modify ctl_table argument https://git.kernel.org/kees/c/0e148d3cca0d
Take care,
linux-stable-mirror@lists.linaro.org
-
Kees Cook -
linux＠weissschuh.net -
Thomas Weißschuh
-
Tycho Andersen