Re: [PATCH] powerpc: Add barrier_nospec to raw_copy_in_user()
On Thu, Sep 19, 2019 at 12:04:27AM +1000, Michael Ellerman wrote:
> Michal Suchánek writes:
> > On Wed, 6 Mar 2019 12:10:38 +1100
> > Suraj Jitindar Singh wrote:
> >
> >> Commit ddf35cf3764b ("powerpc: Use barrier_nospec in copy_from_user()")
> >> Added barrier_nospec before loading from user-controller pointers.
> >> The intention was to order the load from the potentially user-controlled
> >> pointer vs a previous branch based on an access_ok() check or similar.
> >>
> >> In order to achieve the same result, add a barrier_nospec to the
> >> raw_copy_in_user() function before loading from such a user-controlled
> >> pointer.
> >>
> >> Signed-off-by: Suraj Jitindar Singh
> >> ---
> >> arch/powerpc/include/asm/uaccess.h | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/uaccess.h
> >> b/arch/powerpc/include/asm/uaccess.h
> >> index e3a731793ea2..bb615592d5bb 100644
> >> --- a/arch/powerpc/include/asm/uaccess.h
> >> +++ b/arch/powerpc/include/asm/uaccess.h
> >> @@ -306,6 +306,7 @@ extern unsigned long __copy_tofrom_user(void __user
> >> *to,
> >> static inline unsigned long
> >> raw_copy_in_user(void __user *to, const void __user *from, unsigned long
> >> n)
> >> {
> >> + barrier_nospec();
> >>return __copy_tofrom_user(to, from, n);
> >> }
> >> #endif /* __powerpc64__ */
> >
> > Hello,
> >
> > AFAICT this is not needed. The data cannot be used in the kernel without
> > subsequent copy_from_user which already has the nospec barrier.
>
> Suraj did talk to me about this before sending the patch. My memory is
> that he came up with a scenario where it was at least theoretically
> useful, but he didn't mention that in the change log. He was working on
> nested KVM at the time.
>
> I've now forgotten, and he's moved on to other things so probably won't
> remember either, if he's even checking his mail :/
In absence of any argument for the code and absence of the same code on
other architectures I would say you were just confused when merging
this. The code is confusing after all.
Thanks
Michal
Re: [PATCH] powerpc: Add barrier_nospec to raw_copy_in_user()
Michal Suchánek writes:
> On Wed, 6 Mar 2019 12:10:38 +1100
> Suraj Jitindar Singh wrote:
>
>> Commit ddf35cf3764b ("powerpc: Use barrier_nospec in copy_from_user()")
>> Added barrier_nospec before loading from user-controller pointers.
>> The intention was to order the load from the potentially user-controlled
>> pointer vs a previous branch based on an access_ok() check or similar.
>>
>> In order to achieve the same result, add a barrier_nospec to the
>> raw_copy_in_user() function before loading from such a user-controlled
>> pointer.
>>
>> Signed-off-by: Suraj Jitindar Singh
>> ---
>> arch/powerpc/include/asm/uaccess.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h
>> b/arch/powerpc/include/asm/uaccess.h
>> index e3a731793ea2..bb615592d5bb 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -306,6 +306,7 @@ extern unsigned long __copy_tofrom_user(void __user *to,
>> static inline unsigned long
>> raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
>> {
>> +barrier_nospec();
>> return __copy_tofrom_user(to, from, n);
>> }
>> #endif /* __powerpc64__ */
>
> Hello,
>
> AFAICT this is not needed. The data cannot be used in the kernel without
> subsequent copy_from_user which already has the nospec barrier.
Suraj did talk to me about this before sending the patch. My memory is
that he came up with a scenario where it was at least theoretically
useful, but he didn't mention that in the change log. He was working on
nested KVM at the time.
I've now forgotten, and he's moved on to other things so probably won't
remember either, if he's even checking his mail :/
cheers
Re: [PATCH] powerpc: Add barrier_nospec to raw_copy_in_user()
On Wed, 6 Mar 2019 12:10:38 +1100
Suraj Jitindar Singh wrote:
> Commit ddf35cf3764b ("powerpc: Use barrier_nospec in copy_from_user()")
> Added barrier_nospec before loading from user-controller pointers.
> The intention was to order the load from the potentially user-controlled
> pointer vs a previous branch based on an access_ok() check or similar.
>
> In order to achieve the same result, add a barrier_nospec to the
> raw_copy_in_user() function before loading from such a user-controlled
> pointer.
>
> Signed-off-by: Suraj Jitindar Singh
> ---
> arch/powerpc/include/asm/uaccess.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h
> index e3a731793ea2..bb615592d5bb 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -306,6 +306,7 @@ extern unsigned long __copy_tofrom_user(void __user *to,
> static inline unsigned long
> raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
> {
> + barrier_nospec();
> return __copy_tofrom_user(to, from, n);
> }
> #endif /* __powerpc64__ */
Hello,
AFAICT this is not needed. The data cannot be used in the kernel without
subsequent copy_from_user which already has the nospec barrier.
Thanks
Michal
Re: [PATCH] powerpc: Add barrier_nospec to raw_copy_in_user()
On Wed, 2019-03-06 at 01:10:38 UTC, Suraj Jitindar Singh wrote:
> Commit ddf35cf3764b ("powerpc: Use barrier_nospec in copy_from_user()")
> Added barrier_nospec before loading from user-controller pointers.
> The intention was to order the load from the potentially user-controlled
> pointer vs a previous branch based on an access_ok() check or similar.
>
> In order to achieve the same result, add a barrier_nospec to the
> raw_copy_in_user() function before loading from such a user-controlled
> pointer.
>
> Signed-off-by: Suraj Jitindar Singh
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/6fbcdd59094ade30db63f32316e9502425d7b256
cheers
[PATCH] powerpc: Add barrier_nospec to raw_copy_in_user()
Commit ddf35cf3764b ("powerpc: Use barrier_nospec in copy_from_user()")
Added barrier_nospec before loading from user-controller pointers.
The intention was to order the load from the potentially user-controlled
pointer vs a previous branch based on an access_ok() check or similar.
In order to achieve the same result, add a barrier_nospec to the
raw_copy_in_user() function before loading from such a user-controlled
pointer.
Signed-off-by: Suraj Jitindar Singh
---
arch/powerpc/include/asm/uaccess.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/include/asm/uaccess.h
b/arch/powerpc/include/asm/uaccess.h
index e3a731793ea2..bb615592d5bb 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -306,6 +306,7 @@ extern unsigned long __copy_tofrom_user(void __user *to,
static inline unsigned long
raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
{
+ barrier_nospec();
return __copy_tofrom_user(to, from, n);
}
#endif /* __powerpc64__ */
--
2.13.6
