2024-05-03 13:44:45

by Thomas Weißschuh

[permalink] [raw]
Subject: [PATCH] stackleak: don't modify ctl_table argument

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: [email protected]
Acked-by: Kees Cook <[email protected]>
Reviewed-by: Luis Chamberlain <[email protected]>
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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/[email protected]/

Cc: Joel Granados <[email protected]>
---
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,
--
Thomas Weißschuh <[email protected]>



2024-05-03 14:07:07

by Tycho Andersen

[permalink] [raw]
Subject: Re: [PATCH] stackleak: don't modify ctl_table argument

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 <[email protected]>

Tycho

2024-05-03 17:55:57

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] stackleak: don't modify ctl_table argument

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: [email protected]

I realize I've already Acked, but does this actually need to be CCed
to stable?

> Acked-by: Kees Cook <[email protected]>
> Reviewed-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Thomas Wei?schuh <[email protected]>
> ---
> 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/[email protected]/
>
> Cc: Joel Granados <[email protected]>
> ---
> 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 <[email protected]>
>

--
Kees Cook

2024-05-03 18:25:44

by Thomas Weißschuh

[permalink] [raw]
Subject: Re: [PATCH] stackleak: don't modify ctl_table argument

May 3, 2024 19:55:37 Kees Cook <[email protected]>:

> 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: [email protected]
>
> 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 <[email protected]>
>> Reviewed-by: Luis Chamberlain <[email protected]>
>> Signed-off-by: Thomas Weißschuh <[email protected]>
>> ---
>> 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/[email protected]/
>>
>> Cc: Joel Granados <[email protected]>
>> ---
>> 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!

2024-05-03 19:32:40

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] stackleak: don't modify ctl_table argument

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: [email protected]
> Acked-by: Kees Cook <[email protected]>
> Reviewed-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Thomas Wei?schuh <[email protected]>
> ---
> 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/[email protected]/
>
> Cc: Joel Granados <[email protected]>
> ---
> 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 <[email protected]>
>

--
Kees Cook

2024-05-03 19:36:19

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] stackleak: don't modify ctl_table argument

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,

--
Kees Cook