* [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
@ 2021-03-18 22:41 Ben Dooks
2021-03-18 22:48 ` Arnd Bergmann
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ben Dooks @ 2021-03-18 22:41 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, linux-riscv
Cc: Terry Hu, Ben Dooks, Arnd Bergman
The <asm/uaccess.h> header has a problem with
put_user(a, ptr) if the 'a' is not a simple
variable, such as a function. This can lead
to the compiler producing code as so:
1: enable_user_access()
2: evaluate 'a'
3: put 'a' to 'ptr'
4: disable_user_acess()
The issue is that 'a' is now being evaluated
with the user memory protections disabled. So
we try and force the evaulation by assinging
'x' to __val at the start, and hoping the
compiler barriers in enable_user_access()
do the job of ordering step 2 before step 1.
This has shown up in a bug where 'a' sleeps
and thus schedules out and loses the SR_SUM
flag. This isn't sufficient to fully fix, but
should reduce the window of opportunity.
Cc: Arnd Bergman <arnd@arndb.de>
---
arch/riscv/include/asm/uaccess.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 824b2c9da75b..7bf90d462ec9 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -306,7 +306,10 @@ do { \
* data types like structures or arrays.
*
* @ptr must have pointer-to-simple-variable type, and @x must be assignable
- * to the result of dereferencing @ptr.
+ * to the result of dereferencing @ptr. The @x is copied inside the macro
+ * to avoid code re-ordering where @x gets evaulated within the block that
+ * enables user-space access (thus possibly bypassing some of the protection
+ * this feautre provides).
*
* Caller must check the pointer with access_ok() before calling this
* function.
@@ -316,12 +319,13 @@ do { \
#define __put_user(x, ptr) \
({ \
__typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
+ __typeof__(*__gu_ptr) __val = (x); \
long __pu_err = 0; \
\
__chk_user_ptr(__gu_ptr); \
\
__enable_user_access(); \
- __put_user_nocheck(x, __gu_ptr, __pu_err); \
+ __put_user_nocheck(__val, __gu_ptr, __pu_err); \
__disable_user_access(); \
\
__pu_err; \
--
2.30.2
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-18 22:41 [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access Ben Dooks
@ 2021-03-18 22:48 ` Arnd Bergmann
2021-03-18 23:46 ` Ben Dooks
2021-03-19 13:05 ` Christoph Hellwig
2021-03-19 15:03 ` Alex Ghiti
2 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2021-03-18 22:48 UTC (permalink / raw)
To: Ben Dooks; +Cc: Paul Walmsley, Palmer Dabbelt, linux-riscv, Terry Hu
On Thu, Mar 18, 2021 at 11:41 PM Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>
> The <asm/uaccess.h> header has a problem with
> put_user(a, ptr) if the 'a' is not a simple
> variable, such as a function. This can lead
> to the compiler producing code as so:
>
> 1: enable_user_access()
> 2: evaluate 'a'
> 3: put 'a' to 'ptr'
> 4: disable_user_acess()
>
> The issue is that 'a' is now being evaluated
> with the user memory protections disabled. So
> we try and force the evaulation by assinging
> 'x' to __val at the start, and hoping the
> compiler barriers in enable_user_access()
> do the job of ordering step 2 before step 1.
>
> This has shown up in a bug where 'a' sleeps
> and thus schedules out and loses the SR_SUM
> flag. This isn't sufficient to fully fix, but
> should reduce the window of opportunity.
>
> Cc: Arnd Bergman <arnd@arndb.de>
Reviewed-by: Arnd Bergman <arnd@arndb.de>
Note: your Signed-off-by seems to be missing.
> ---
> arch/riscv/include/asm/uaccess.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 824b2c9da75b..7bf90d462ec9 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -306,7 +306,10 @@ do { \
> * data types like structures or arrays.
> *
> * @ptr must have pointer-to-simple-variable type, and @x must be assignable
> - * to the result of dereferencing @ptr.
> + * to the result of dereferencing @ptr. The @x is copied inside the macro
> + * to avoid code re-ordering where @x gets evaulated within the block that
> + * enables user-space access (thus possibly bypassing some of the protection
> + * this feautre provides).
> *
> * Caller must check the pointer with access_ok() before calling this
> * function.
> @@ -316,12 +319,13 @@ do { \
> #define __put_user(x, ptr) \
> ({ \
> __typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
> + __typeof__(*__gu_ptr) __val = (x); \
> long __pu_err = 0; \
Side note: We should really change the first typeof to an __auto_type
here (and on all other architectures), but that's a separate change
and not strictly a bugfix.
Arnd
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-18 22:48 ` Arnd Bergmann
@ 2021-03-18 23:46 ` Ben Dooks
0 siblings, 0 replies; 9+ messages in thread
From: Ben Dooks @ 2021-03-18 23:46 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Paul Walmsley, Palmer Dabbelt, linux-riscv, Terry Hu
On 18/03/2021 22:48, Arnd Bergmann wrote:
> On Thu, Mar 18, 2021 at 11:41 PM Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>>
>> The <asm/uaccess.h> header has a problem with
>> put_user(a, ptr) if the 'a' is not a simple
>> variable, such as a function. This can lead
>> to the compiler producing code as so:
>>
>> 1: enable_user_access()
>> 2: evaluate 'a'
>> 3: put 'a' to 'ptr'
>> 4: disable_user_acess()
>>
>> The issue is that 'a' is now being evaluated
>> with the user memory protections disabled. So
>> we try and force the evaulation by assinging
>> 'x' to __val at the start, and hoping the
>> compiler barriers in enable_user_access()
>> do the job of ordering step 2 before step 1.
>>
>> This has shown up in a bug where 'a' sleeps
>> and thus schedules out and loses the SR_SUM
>> flag. This isn't sufficient to fully fix, but
>> should reduce the window of opportunity.
>>
>> Cc: Arnd Bergman <arnd@arndb.de>
>
> Reviewed-by: Arnd Bergman <arnd@arndb.de>
>
> Note: your Signed-off-by seems to be missing.
Sorry, forgot as was intending smaller RFC release.
Thanks for the help sorting out the compile issues
I need to review the patch description anyway and make it flow
closer to 73 columns instead of the rather truncated version it is.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-18 22:41 [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access Ben Dooks
2021-03-18 22:48 ` Arnd Bergmann
@ 2021-03-19 13:05 ` Christoph Hellwig
2021-03-19 14:19 ` Ben Dooks
2021-03-19 15:03 ` Alex Ghiti
2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-03-19 13:05 UTC (permalink / raw)
To: Ben Dooks
Cc: Paul Walmsley, Palmer Dabbelt, linux-riscv, Terry Hu, Arnd Bergman
On Thu, Mar 18, 2021 at 10:41:35PM +0000, Ben Dooks wrote:
> The <asm/uaccess.h> header has a problem with
> put_user(a, ptr) if the 'a' is not a simple
> variable, such as a function. This can lead
> to the compiler producing code as so:
Nit: your commit log seeems to truncate lines after 50 chars, you can
and should use almost 1.5 as much.
> * @ptr must have pointer-to-simple-variable type, and @x must be assignable
> - * to the result of dereferencing @ptr.
> + * to the result of dereferencing @ptr. The @x is copied inside the macro
> + * to avoid code re-ordering where @x gets evaulated within the block that
> + * enables user-space access (thus possibly bypassing some of the protection
> + * this feautre provides).
Well, hopefully the compiler is smart enought to not actually copy.
So we should probably talk about evaluating the argument here.
> #define __put_user(x, ptr) \
> ({ \
> __typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
> + __typeof__(*__gu_ptr) __val = (x); \
> long __pu_err = 0; \
> \
> __chk_user_ptr(__gu_ptr); \
> \
> __enable_user_access(); \
> - __put_user_nocheck(x, __gu_ptr, __pu_err); \
> + __put_user_nocheck(__val, __gu_ptr, __pu_err); \
> __disable_user_access(); \
It looks like __get_user needs the same treatment.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-19 13:05 ` Christoph Hellwig
@ 2021-03-19 14:19 ` Ben Dooks
0 siblings, 0 replies; 9+ messages in thread
From: Ben Dooks @ 2021-03-19 14:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Paul Walmsley, Palmer Dabbelt, linux-riscv, Terry Hu, Arnd Bergman
On 19/03/2021 13:05, Christoph Hellwig wrote:
> On Thu, Mar 18, 2021 at 10:41:35PM +0000, Ben Dooks wrote:
>> The <asm/uaccess.h> header has a problem with
>> put_user(a, ptr) if the 'a' is not a simple
>> variable, such as a function. This can lead
>> to the compiler producing code as so:
>
> Nit: your commit log seeems to truncate lines after 50 chars, you can
> and should use almost 1.5 as much.
Thanks, noted this once I'd re-read the patch. I have a few
other minor bits to test and to credit Arnd with helping out
after failing to get the first attempt to compile.
>> * @ptr must have pointer-to-simple-variable type, and @x must be assignable
>> - * to the result of dereferencing @ptr.
>> + * to the result of dereferencing @ptr. The @x is copied inside the macro
>> + * to avoid code re-ordering where @x gets evaulated within the block that
>> + * enables user-space access (thus possibly bypassing some of the protection
>> + * this feautre provides).
>
> Well, hopefully the compiler is smart enought to not actually copy.
> So we should probably talk about evaluating the argument here.
>
>> #define __put_user(x, ptr) \
>> ({ \
>> __typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
>> + __typeof__(*__gu_ptr) __val = (x); \
>> long __pu_err = 0; \
>> \
>> __chk_user_ptr(__gu_ptr); \
>> \
>> __enable_user_access(); \
>> - __put_user_nocheck(x, __gu_ptr, __pu_err); \
>> + __put_user_nocheck(__val, __gu_ptr, __pu_err); \
>> __disable_user_access(); \
>
> It looks like __get_user needs the same treatment.
I will check that, then again I don't think people do anything
that would be an issue. We caught this from the put_user() in the
schedule_tail() code which causes the pid fetch function to be
called within the __enable_user_access().
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-18 22:41 [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access Ben Dooks
2021-03-18 22:48 ` Arnd Bergmann
2021-03-19 13:05 ` Christoph Hellwig
@ 2021-03-19 15:03 ` Alex Ghiti
2021-03-19 15:09 ` Ben Dooks
2 siblings, 1 reply; 9+ messages in thread
From: Alex Ghiti @ 2021-03-19 15:03 UTC (permalink / raw)
To: Ben Dooks, Paul Walmsley, Palmer Dabbelt, linux-riscv
Cc: Terry Hu, Arnd Bergman
Le 3/18/21 à 6:41 PM, Ben Dooks a écrit :
> The <asm/uaccess.h> header has a problem with
> put_user(a, ptr) if the 'a' is not a simple
> variable, such as a function. This can lead
> to the compiler producing code as so:
>
> 1: enable_user_access()
> 2: evaluate 'a'
> 3: put 'a' to 'ptr'
> 4: disable_user_acess()
>
> The issue is that 'a' is now being evaluated
> with the user memory protections disabled. So
> we try and force the evaulation by assinging
> 'x' to __val at the start, and hoping the
> compiler barriers in enable_user_access()
> do the job of ordering step 2 before step 1.
>
> This has shown up in a bug where 'a' sleeps
> and thus schedules out and loses the SR_SUM
> flag. This isn't sufficient to fully fix, but
> should reduce the window of opportunity.
I would say this patch is enough to fix the issue because it only
happens when 'a' *explicitly schedules* when in
__enable_user_access()/__disable_user_access(). Otherwise, I see 2 cases:
- user memory is correctly mapped and nothing stops the current process.
- an exception (interrupt or trap) is triggered: in those cases, the
exception path correctly saves and restores SR_SUM.
>
> Cc: Arnd Bergman <arnd@arndb.de>
> ---
> arch/riscv/include/asm/uaccess.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 824b2c9da75b..7bf90d462ec9 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -306,7 +306,10 @@ do { \
> * data types like structures or arrays.
> *
> * @ptr must have pointer-to-simple-variable type, and @x must be assignable
> - * to the result of dereferencing @ptr.
> + * to the result of dereferencing @ptr. The @x is copied inside the macro
> + * to avoid code re-ordering where @x gets evaulated within the block that
> + * enables user-space access (thus possibly bypassing some of the protection
> + * this feautre provides).
> *
> * Caller must check the pointer with access_ok() before calling this
> * function.
> @@ -316,12 +319,13 @@ do { \
> #define __put_user(x, ptr) \
> ({ \
> __typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
> + __typeof__(*__gu_ptr) __val = (x); \
> long __pu_err = 0; \
> \
> __chk_user_ptr(__gu_ptr); \
> \
> __enable_user_access(); \
> - __put_user_nocheck(x, __gu_ptr, __pu_err); \
> + __put_user_nocheck(__val, __gu_ptr, __pu_err); \
> __disable_user_access(); \
> \
> __pu_err; \
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-19 15:03 ` Alex Ghiti
@ 2021-03-19 15:09 ` Ben Dooks
2021-03-19 16:12 ` Alex Ghiti
0 siblings, 1 reply; 9+ messages in thread
From: Ben Dooks @ 2021-03-19 15:09 UTC (permalink / raw)
To: Alex Ghiti, Paul Walmsley, Palmer Dabbelt, linux-riscv
Cc: Terry Hu, Arnd Bergman
On 19/03/2021 15:03, Alex Ghiti wrote:
> Le 3/18/21 à 6:41 PM, Ben Dooks a écrit :
>> The <asm/uaccess.h> header has a problem with
>> put_user(a, ptr) if the 'a' is not a simple
>> variable, such as a function. This can lead
>> to the compiler producing code as so:
>>
>> 1: enable_user_access()
>> 2: evaluate 'a'
>> 3: put 'a' to 'ptr'
>> 4: disable_user_acess()
>>
>> The issue is that 'a' is now being evaluated
>> with the user memory protections disabled. So
>> we try and force the evaulation by assinging
>> 'x' to __val at the start, and hoping the
>> compiler barriers in enable_user_access()
>> do the job of ordering step 2 before step 1.
>>
>> This has shown up in a bug where 'a' sleeps
>> and thus schedules out and loses the SR_SUM
>> flag. This isn't sufficient to fully fix, but
>> should reduce the window of opportunity.
>
> I would say this patch is enough to fix the issue because it only
> happens when 'a' *explicitly schedules* when in
> __enable_user_access()/__disable_user_access(). Otherwise, I see 2 cases:
>
> - user memory is correctly mapped and nothing stops the current process.
> - an exception (interrupt or trap) is triggered: in those cases, the
> exception path correctly saves and restores SR_SUM.
This fixes part of the other issue.
I did point out in the other email there could be longer cases
where the protections are disabled. The saving of the flags over
switch_to() is still necessary.
Also, I am not sure if this will guarantee ordering. It does
seem to fix it for the cases I checked
>>
>> Cc: Arnd Bergman <arnd@arndb.de>
>> ---
>> arch/riscv/include/asm/uaccess.h | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h
>> b/arch/riscv/include/asm/uaccess.h
>> index 824b2c9da75b..7bf90d462ec9 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -306,7 +306,10 @@ do { \
>> * data types like structures or arrays.
>> *
>> * @ptr must have pointer-to-simple-variable type, and @x must be
>> assignable
>> - * to the result of dereferencing @ptr.
>> + * to the result of dereferencing @ptr. The @x is copied inside the
>> macro
>> + * to avoid code re-ordering where @x gets evaulated within the block
>> that
>> + * enables user-space access (thus possibly bypassing some of the
>> protection
>> + * this feautre provides).
>> *
>> * Caller must check the pointer with access_ok() before calling this
>> * function.
>> @@ -316,12 +319,13 @@ do { \
>> #define __put_user(x, ptr) \
>> ({ \
>> __typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
>> + __typeof__(*__gu_ptr) __val = (x); \
>> long __pu_err = 0; \
>> \
>> __chk_user_ptr(__gu_ptr); \
>> \
>> __enable_user_access(); \
>> - __put_user_nocheck(x, __gu_ptr, __pu_err); \
>> + __put_user_nocheck(__val, __gu_ptr, __pu_err); \
>> __disable_user_access(); \
>> \
>> __pu_err; \
>>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-19 15:09 ` Ben Dooks
@ 2021-03-19 16:12 ` Alex Ghiti
2021-03-19 21:56 ` Ben Dooks
0 siblings, 1 reply; 9+ messages in thread
From: Alex Ghiti @ 2021-03-19 16:12 UTC (permalink / raw)
To: Ben Dooks, Paul Walmsley, Palmer Dabbelt, linux-riscv
Cc: Terry Hu, Arnd Bergman
Le 3/19/21 à 11:09 AM, Ben Dooks a écrit :
> On 19/03/2021 15:03, Alex Ghiti wrote:
>> Le 3/18/21 à 6:41 PM, Ben Dooks a écrit :
>>> The <asm/uaccess.h> header has a problem with
>>> put_user(a, ptr) if the 'a' is not a simple
>>> variable, such as a function. This can lead
>>> to the compiler producing code as so:
>>>
>>> 1: enable_user_access()
>>> 2: evaluate 'a'
>>> 3: put 'a' to 'ptr'
>>> 4: disable_user_acess()
>>>
>>> The issue is that 'a' is now being evaluated
>>> with the user memory protections disabled. So
>>> we try and force the evaulation by assinging
>>> 'x' to __val at the start, and hoping the
>>> compiler barriers in enable_user_access()
>>> do the job of ordering step 2 before step 1.
>>>
>>> This has shown up in a bug where 'a' sleeps
>>> and thus schedules out and loses the SR_SUM
>>> flag. This isn't sufficient to fully fix, but
>>> should reduce the window of opportunity.
>>
>> I would say this patch is enough to fix the issue because it only
>> happens when 'a' *explicitly schedules* when in
>> __enable_user_access()/__disable_user_access(). Otherwise, I see 2 cases:
>>
>> - user memory is correctly mapped and nothing stops the current process.
>> - an exception (interrupt or trap) is triggered: in those cases, the
>> exception path correctly saves and restores SR_SUM.
>
> This fixes part of the other issue.
>
> I did point out in the other email there could be longer cases
> where the protections are disabled. The saving of the flags over
> switch_to() is still necessary.
I can't find your explanation, could you elaborate a bit more here on
why this fix is not enough ?
Thanks !
>
> Also, I am not sure if this will guarantee ordering. It does
> seem to fix it for the cases I checked
>
>>>
>>> Cc: Arnd Bergman <arnd@arndb.de>
>>> ---
>>> arch/riscv/include/asm/uaccess.h | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/uaccess.h
>>> b/arch/riscv/include/asm/uaccess.h
>>> index 824b2c9da75b..7bf90d462ec9 100644
>>> --- a/arch/riscv/include/asm/uaccess.h
>>> +++ b/arch/riscv/include/asm/uaccess.h
>>> @@ -306,7 +306,10 @@ do { \
>>> * data types like structures or arrays.
>>> *
>>> * @ptr must have pointer-to-simple-variable type, and @x must be
>>> assignable
>>> - * to the result of dereferencing @ptr.
>>> + * to the result of dereferencing @ptr. The @x is copied inside the
>>> macro
>>> + * to avoid code re-ordering where @x gets evaulated within the
>>> block that
>>> + * enables user-space access (thus possibly bypassing some of the
>>> protection
>>> + * this feautre provides).
>>> *
>>> * Caller must check the pointer with access_ok() before calling this
>>> * function.
>>> @@ -316,12 +319,13 @@ do { \
>>> #define __put_user(x, ptr) \
>>> ({ \
>>> __typeof__(*(ptr)) __user *__gu_ptr = (ptr); \
>>> + __typeof__(*__gu_ptr) __val = (x); \
>>> long __pu_err = 0; \
>>> \
>>> __chk_user_ptr(__gu_ptr); \
>>> \
>>> __enable_user_access(); \
>>> - __put_user_nocheck(x, __gu_ptr, __pu_err); \
>>> + __put_user_nocheck(__val, __gu_ptr, __pu_err); \
>>> __disable_user_access(); \
>>> \
>>> __pu_err; \
>>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
2021-03-19 16:12 ` Alex Ghiti
@ 2021-03-19 21:56 ` Ben Dooks
0 siblings, 0 replies; 9+ messages in thread
From: Ben Dooks @ 2021-03-19 21:56 UTC (permalink / raw)
To: Alex Ghiti, Paul Walmsley, Palmer Dabbelt, linux-riscv
Cc: Terry Hu, Arnd Bergman
On 19/03/2021 16:12, Alex Ghiti wrote:
> Le 3/19/21 à 11:09 AM, Ben Dooks a écrit :
>> On 19/03/2021 15:03, Alex Ghiti wrote:
>>> Le 3/18/21 à 6:41 PM, Ben Dooks a écrit :
>>>> The <asm/uaccess.h> header has a problem with
>>>> put_user(a, ptr) if the 'a' is not a simple
>>>> variable, such as a function. This can lead
>>>> to the compiler producing code as so:
>>>>
>>>> 1: enable_user_access()
>>>> 2: evaluate 'a'
>>>> 3: put 'a' to 'ptr'
>>>> 4: disable_user_acess()
>>>>
>>>> The issue is that 'a' is now being evaluated
>>>> with the user memory protections disabled. So
>>>> we try and force the evaulation by assinging
>>>> 'x' to __val at the start, and hoping the
>>>> compiler barriers in enable_user_access()
>>>> do the job of ordering step 2 before step 1.
>>>>
>>>> This has shown up in a bug where 'a' sleeps
>>>> and thus schedules out and loses the SR_SUM
>>>> flag. This isn't sufficient to fully fix, but
>>>> should reduce the window of opportunity.
>>>
>>> I would say this patch is enough to fix the issue because it only
>>> happens when 'a' *explicitly schedules* when in
>>> __enable_user_access()/__disable_user_access(). Otherwise, I see 2
>>> cases:
>>>
>>> - user memory is correctly mapped and nothing stops the current process.
>>> - an exception (interrupt or trap) is triggered: in those cases, the
>>> exception path correctly saves and restores SR_SUM.
>>
>> This fixes part of the other issue.
>>
>> I did point out in the other email there could be longer cases
>> where the protections are disabled. The saving of the flags over
>> switch_to() is still necessary.
>
> I can't find your explanation, could you elaborate a bit more here on
> why this fix is not enough ?
I would have to check if this current applies to riscv, but there is
code that does the following (fs/select.c does this):
if (!user_read_access_begin(from, sizeof(*from)))
return -EFAULT;
unsafe_get_user(to->p, &from->p, Efault);
unsafe_get_user(to->size, &from->size, Efault);
user_read_access_end();
My argument for fixing with both is:
- cover more than the put_user() case
- try and avoid any future bug
- ensure we do not leak SR_SUM elsewhere
I may also have a quick check to see if we don't also leak other
flags during these swaps. It might be we should save and restore
all flags.
I do see that this fix is going to hit a good proportion of the
cases we've seen so far. I could try and run a stress test with
just this in over the weekend (so far syz-stress has been running
for over 24hrs with minimal issues)
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-19 21:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 22:41 [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access Ben Dooks
2021-03-18 22:48 ` Arnd Bergmann
2021-03-18 23:46 ` Ben Dooks
2021-03-19 13:05 ` Christoph Hellwig
2021-03-19 14:19 ` Ben Dooks
2021-03-19 15:03 ` Alex Ghiti
2021-03-19 15:09 ` Ben Dooks
2021-03-19 16:12 ` Alex Ghiti
2021-03-19 21:56 ` Ben Dooks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).