There is a race between the assignment of `table->data` and write value
to the pointer of `table->data` in the __do_proc_doulongvec_minmax() on
the other thread.
CPU0: CPU1:
proc_sys_write
stack_erasing_sysctl proc_sys_call_handler
table->data = &state; stack_erasing_sysctl
table->data = &state;
proc_doulongvec_minmax
do_proc_doulongvec_minmax sysctl_head_finish
__do_proc_doulongvec_minmax unuse_table
i = table->data;
*i = val; // corrupt CPU1's stack
Fix this by duplicating the `table`, and only update the duplicate of
it.
Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
Signed-off-by: Muchun Song <[email protected]>
---
changelogs in v2:
1. Add more details about how the race happened to the commit message.
kernel/stackleak.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index a8fc9ae1d03d..fd95b87478ff 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -25,10 +25,15 @@ 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 dup_table = *table;
- table->data = &state;
- table->maxlen = sizeof(int);
- ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+ /*
+ * In order to avoid races with __do_proc_doulongvec_minmax(), we
+ * can duplicate the @table and alter the duplicate of it.
+ */
+ dup_table.data = &state;
+ dup_table.maxlen = sizeof(int);
+ ret = proc_dointvec_minmax(&dup_table, write, buffer, lenp, ppos);
state = !!state;
if (ret || !write || state == prev_state)
return ret;
--
2.11.0
Hi all,
Any comments or suggestions? Thanks.
On Fri, Aug 28, 2020 at 11:19 AM Muchun Song <[email protected]> wrote:
>
> There is a race between the assignment of `table->data` and write value
> to the pointer of `table->data` in the __do_proc_doulongvec_minmax() on
> the other thread.
>
> CPU0: CPU1:
> proc_sys_write
> stack_erasing_sysctl proc_sys_call_handler
> table->data = &state; stack_erasing_sysctl
> table->data = &state;
> proc_doulongvec_minmax
> do_proc_doulongvec_minmax sysctl_head_finish
> __do_proc_doulongvec_minmax unuse_table
> i = table->data;
> *i = val; // corrupt CPU1's stack
>
> Fix this by duplicating the `table`, and only update the duplicate of
> it.
>
> Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
> Signed-off-by: Muchun Song <[email protected]>
> ---
> changelogs in v2:
> 1. Add more details about how the race happened to the commit message.
>
> kernel/stackleak.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/stackleak.c b/kernel/stackleak.c
> index a8fc9ae1d03d..fd95b87478ff 100644
> --- a/kernel/stackleak.c
> +++ b/kernel/stackleak.c
> @@ -25,10 +25,15 @@ 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 dup_table = *table;
>
> - table->data = &state;
> - table->maxlen = sizeof(int);
> - ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> + /*
> + * In order to avoid races with __do_proc_doulongvec_minmax(), we
> + * can duplicate the @table and alter the duplicate of it.
> + */
> + dup_table.data = &state;
> + dup_table.maxlen = sizeof(int);
> + ret = proc_dointvec_minmax(&dup_table, write, buffer, lenp, ppos);
> state = !!state;
> if (ret || !write || state == prev_state)
> return ret;
> --
> 2.11.0
>
--
Yours,
Muchun
On 07.09.2020 05:54, Muchun Song wrote:
> Hi all,
>
> Any comments or suggestions? Thanks.
>
> On Fri, Aug 28, 2020 at 11:19 AM Muchun Song <[email protected]> wrote:
>>
>> There is a race between the assignment of `table->data` and write value
>> to the pointer of `table->data` in the __do_proc_doulongvec_minmax() on
>> the other thread.
>>
>> CPU0: CPU1:
>> proc_sys_write
>> stack_erasing_sysctl proc_sys_call_handler
>> table->data = &state; stack_erasing_sysctl
>> table->data = &state;
>> proc_doulongvec_minmax
>> do_proc_doulongvec_minmax sysctl_head_finish
>> __do_proc_doulongvec_minmax unuse_table
>> i = table->data;
>> *i = val; // corrupt CPU1's stack
Hello everyone!
As I remember, I implemented stack_erasing_sysctl() very similar to other sysctl
handlers. Is that issue relevant for other handlers as well?
Muchun, could you elaborate how CPU1's stack is corrupted and how you detected
that? Thanks!
Best regards,
Alexander
>> Fix this by duplicating the `table`, and only update the duplicate of
>> it.
>>
>> Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
>> Signed-off-by: Muchun Song <[email protected]>
>> ---
>> changelogs in v2:
>> 1. Add more details about how the race happened to the commit message.
>>
>> kernel/stackleak.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/stackleak.c b/kernel/stackleak.c
>> index a8fc9ae1d03d..fd95b87478ff 100644
>> --- a/kernel/stackleak.c
>> +++ b/kernel/stackleak.c
>> @@ -25,10 +25,15 @@ 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 dup_table = *table;
>>
>> - table->data = &state;
>> - table->maxlen = sizeof(int);
>> - ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>> + /*
>> + * In order to avoid races with __do_proc_doulongvec_minmax(), we
>> + * can duplicate the @table and alter the duplicate of it.
>> + */
>> + dup_table.data = &state;
>> + dup_table.maxlen = sizeof(int);
>> + ret = proc_dointvec_minmax(&dup_table, write, buffer, lenp, ppos);
>> state = !!state;
>> if (ret || !write || state == prev_state)
>> return ret;
>> --
>> 2.11.0
>>
>
>