[PATCH] MIPS: Fix strnlen_user access check

[PATCH] MIPS: Fix strnlen_user access check

[Jinyang He]

Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
strnlen_user(). Jump out when checking access_ok() with condition that
(s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
just checked (ua_limit & s) without checking (ua_limit & (s + n)).
Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
 arch/mips/include/asm/uaccess.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
index 91bc7fb..85ba0c8 100644
--- a/arch/mips/include/asm/uaccess.h
+++ b/arch/mips/include/asm/uaccess.h
@@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n)
 {
 	long res;
 
-	if (!access_ok(s, n))
-		return -0;
+	if (unlikely(n <= 0))
+		return 0;
+
+	if (!access_ok(s, n)) {
+		if (!access_ok(s, 0))
+			return 0;
+
+		n = __UA_LIMIT - (unsigned long)s - 1;
+	}
 
 	might_fault();
 	__asm__ __volatile__(
-- 
2.1.0

Re: [PATCH] MIPS: Fix strnlen_user access check

[Tiezhu Yang]

On 04/11/2021 07:04 PM, Jinyang He wrote:
> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
> strnlen_user(). Jump out when checking access_ok() with condition that
> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
> just checked (ua_limit & s) without checking (ua_limit & (s + n)).
> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
>   arch/mips/include/asm/uaccess.h | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> index 91bc7fb..85ba0c8 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n)
>   {
>   	long res;
>   
> -	if (!access_ok(s, n))
> -		return -0;
> +	if (unlikely(n <= 0))
> +		return 0;
> +
> +	if (!access_ok(s, n)) {
> +		if (!access_ok(s, 0))
> +			return 0;
> +
> +		n = __UA_LIMIT - (unsigned long)s - 1;
> +	}
>   
>   	might_fault();
>   	__asm__ __volatile__(

The following simple changes are OK to fix this issue?

diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
index 91bc7fb..eafc99b 100644
--- a/arch/mips/include/asm/uaccess.h
+++ b/arch/mips/include/asm/uaccess.h
@@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n)
  {
         long res;
  
-       if (!access_ok(s, n))
-               return -0;
+       if (!access_ok(s, 1))
+               return 0;
  
         might_fault();
         __asm__ __volatile__(

Thanks,
Tiezhu

Re: [PATCH] MIPS: Fix strnlen_user access check

[Jinyang He]

On 04/12/2021 11:02 AM, Tiezhu Yang wrote:

> On 04/11/2021 07:04 PM, Jinyang He wrote:
>> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
>> strnlen_user(). Jump out when checking access_ok() with condition that
>> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
>> just checked (ua_limit & s) without checking (ua_limit & (s + n)).
>> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
>>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>>   arch/mips/include/asm/uaccess.h | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/uaccess.h 
>> b/arch/mips/include/asm/uaccess.h
>> index 91bc7fb..85ba0c8 100644
>> --- a/arch/mips/include/asm/uaccess.h
>> +++ b/arch/mips/include/asm/uaccess.h
>> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char 
>> __user *s, long n)
>>   {
>>       long res;
>>   -    if (!access_ok(s, n))
>> -        return -0;
>> +    if (unlikely(n <= 0))
>> +        return 0;
>> +
>> +    if (!access_ok(s, n)) {
>> +        if (!access_ok(s, 0))
>> +            return 0;
>> +
>> +        n = __UA_LIMIT - (unsigned long)s - 1;
>> +    }
>>         might_fault();
>>       __asm__ __volatile__(
>
> The following simple changes are OK to fix this issue?
>
> diff --git a/arch/mips/include/asm/uaccess.h 
> b/arch/mips/include/asm/uaccess.h
> index 91bc7fb..eafc99b 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user 
> *s, long n)
>  {
>         long res;
>
> -       if (!access_ok(s, n))
> -               return -0;
> +       if (!access_ok(s, 1))
> +               return 0;
>
>         might_fault();
>         __asm__ __volatile__(
>
> Thanks,
> Tiezhu
>
Thanks for your comment. That looks similar to other archs, but I don't
know how the access_ok() implementation in other archs.

Using access_ok(s, 0) is similar to the old strnlen_user(). Using
access_ok(s, 1) may have a problem in this extreme case,
s = __UA_LIMIT - 1, *s = 0, and we hope it returns 1. But it returns 0 by
!access_ok(s, 1). Of course, it is so extrme.

More importantly, I want to set up a maximum for strnlen_user_asm. And do
not access the part of beyond __ua_limit. As follow shows,

                     +-----------+
                     |    ...    |
                     +-----------+  <---- s + n
                     |     0     |
                     +-----------+
                     |     s     |
                     +-----------+
                     |     r     |
                     +-----------+
                     |     e     |
                     +-----------+
                     |     h     |
                     +-----------+
                     |     t     |
                     +-----------+
                     |     o     |
                     +-----------+  <---- __UA_LIMIT
                     |     r     |
                     +-----------+
                     |     t     |
                     +-----------+
                     |     s     |
                     +-----------+  <---- s
                     |    ...    |
                     +-----------+

It is dangerous to access "others", for user, only "str" is safe.

I don't know whether it would be happend, I just limited it by change `n`.
Should do other things if meet __UA_LIMIT - 1?

Thanks,
Jinyang

Re: [PATCH] MIPS: Fix strnlen_user access check

[Tiezhu Yang]

On 04/12/2021 11:02 AM, Tiezhu Yang wrote:
> On 04/11/2021 07:04 PM, Jinyang He wrote:
>> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
>> strnlen_user(). Jump out when checking access_ok() with condition that
>> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
>> just checked (ua_limit & s) without checking (ua_limit & (s + n)).
>> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
>>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>>   arch/mips/include/asm/uaccess.h | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/uaccess.h 
>> b/arch/mips/include/asm/uaccess.h
>> index 91bc7fb..85ba0c8 100644
>> --- a/arch/mips/include/asm/uaccess.h
>> +++ b/arch/mips/include/asm/uaccess.h
>> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char 
>> __user *s, long n)
>>   {
>>       long res;
>>   -    if (!access_ok(s, n))
>> -        return -0;
>> +    if (unlikely(n <= 0))
>> +        return 0;
>> +
>> +    if (!access_ok(s, n)) {
>> +        if (!access_ok(s, 0))
>> +            return 0;
>> +
>> +        n = __UA_LIMIT - (unsigned long)s - 1;
>> +    }
>>         might_fault();
>>       __asm__ __volatile__(
>
> The following simple changes are OK to fix this issue?
>
> diff --git a/arch/mips/include/asm/uaccess.h 
> b/arch/mips/include/asm/uaccess.h
> index 91bc7fb..eafc99b 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user 
> *s, long n)
>  {
>         long res;
>
> -       if (!access_ok(s, n))
> -               return -0;
> +       if (!access_ok(s, 1))
> +               return 0;
>
>         might_fault();
>         __asm__ __volatile__(
>
> Thanks,
> Tiezhu
>

Hi all,

Here is some detail info about background and analysis process,
I hope it is useful to understand this issue.

When update kernel with the latest mips-next, we can not login through a
graphical interface, this is because drm radeon GPU driver does not work,
we can not see the boot message "[drm] radeon kernel modesetting enabled."
through the serial console.

drivers/gpu/drm/radeon/radeon_drv.c
static int __init radeon_module_init(void)
{
         [...]
         DRM_INFO("radeon kernel modesetting enabled.\n");
         [...]
}

I use git bisect to find commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs")
is the first bad commit:

  $ git bisect log
  git bisect start
  # good: [666c1fc90cd82184624d4cc5d124c66025f89a47] mips: bmips: 
bcm63268: populate device tree nodes
  git bisect good 666c1fc90cd82184624d4cc5d124c66025f89a47
  # bad: [e86e75596623e1ce5d784db8214687326712a8ae] MIPS: octeon: Add 
__raw_copy_[from|to|in]_user symbols
  git bisect bad e86e75596623e1ce5d784db8214687326712a8ae
  # good: [45deb5faeb9e02951361ceba5ffee721745661c3] MIPS: uaccess: 
Remove get_fs/set_fs call sites
  git bisect good 45deb5faeb9e02951361ceba5ffee721745661c3
  # bad: [5e65c52ec716af6e8f51dacdaeb4a4d872249af1] MIPS: Loongson64: 
Use _CACHE_UNCACHED instead of _CACHE_UNCACHED_ACCELERATED
  git bisect bad 5e65c52ec716af6e8f51dacdaeb4a4d872249af1
  # bad: [04324f44cb69a03fdc8f2ee52386a4fdf6a0043b] MIPS: Remove 
get_fs/set_fs
  git bisect bad 04324f44cb69a03fdc8f2ee52386a4fdf6a0043b
  # first bad commit: [04324f44cb69a03fdc8f2ee52386a4fdf6a0043b] MIPS: 
Remove get_fs/set_fs

I analysis and test the changes in the above first bad commit and find out
the following obvious difference which leads to the login issue.

arch/mips/include/asm/uaccess.h
static inline long strnlen_user(const char __user *s, long n)
{
         [...]
         if (!access_ok(s, n))
                 return -0;
         [...]
}

Thanks,
Tiezhu

Re: [PATCH] MIPS: Fix strnlen_user access check

[Thomas Bogendoerfer]

On Mon, Apr 12, 2021 at 11:02:19AM +0800, Tiezhu Yang wrote:
> On 04/11/2021 07:04 PM, Jinyang He wrote:
> > Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
> > strnlen_user(). Jump out when checking access_ok() with condition that
> > (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
> > just checked (ua_limit & s) without checking (ua_limit & (s + n)).
> > Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
> > 
> > Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> > ---
> >   arch/mips/include/asm/uaccess.h | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> > index 91bc7fb..85ba0c8 100644
> > --- a/arch/mips/include/asm/uaccess.h
> > +++ b/arch/mips/include/asm/uaccess.h
> > @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n)
> >   {
> >   	long res;
> > -	if (!access_ok(s, n))
> > -		return -0;
> > +	if (unlikely(n <= 0))
> > +		return 0;
> > +
> > +	if (!access_ok(s, n)) {
> > +		if (!access_ok(s, 0))
> > +			return 0;
> > +
> > +		n = __UA_LIMIT - (unsigned long)s - 1;
> > +	}
> >   	might_fault();
> >   	__asm__ __volatile__(
> 
> The following simple changes are OK to fix this issue?
> 
> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> index 91bc7fb..eafc99b 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n)
>  {
>         long res;
> -       if (!access_ok(s, n))
> -               return -0;
> +       if (!access_ok(s, 1))
> +               return 0;
>         might_fault();
>         __asm__ __volatile__(

that's the fix I'd like to apply. Could someone send it as a formal
patch ? Thanks.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Fix strnlen_user access check

[Jinyang He]

On 04/12/2021 10:27 PM, Thomas Bogendoerfer wrote:

> On Mon, Apr 12, 2021 at 11:02:19AM +0800, Tiezhu Yang wrote:
>> On 04/11/2021 07:04 PM, Jinyang He wrote:
>>> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
>>> strnlen_user(). Jump out when checking access_ok() with condition that
>>> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
>>> just checked (ua_limit & s) without checking (ua_limit & (s + n)).
>>> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
>>>
>>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>>> ---
>>>    arch/mips/include/asm/uaccess.h | 11 +++++++++--
>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
>>> index 91bc7fb..85ba0c8 100644
>>> --- a/arch/mips/include/asm/uaccess.h
>>> +++ b/arch/mips/include/asm/uaccess.h
>>> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n)
>>>    {
>>>    	long res;
>>> -	if (!access_ok(s, n))
>>> -		return -0;
>>> +	if (unlikely(n <= 0))
>>> +		return 0;
>>> +
>>> +	if (!access_ok(s, n)) {
>>> +		if (!access_ok(s, 0))
>>> +			return 0;
>>> +
>>> +		n = __UA_LIMIT - (unsigned long)s - 1;
>>> +	}
>>>    	might_fault();
>>>    	__asm__ __volatile__(
>> The following simple changes are OK to fix this issue?
>>
>> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
>> index 91bc7fb..eafc99b 100644
>> --- a/arch/mips/include/asm/uaccess.h
>> +++ b/arch/mips/include/asm/uaccess.h
>> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n)
>>   {
>>          long res;
>> -       if (!access_ok(s, n))
>> -               return -0;
>> +       if (!access_ok(s, 1))
>> +               return 0;
>>          might_fault();
>>          __asm__ __volatile__(
> that's the fix I'd like to apply. Could someone send it as a formal
> patch ? Thanks.
>
> Thomas.
>
Hi, Thomas,

Thank you for bringing me more thinking.

I always think it is better to use access_ok(s, 0) on MIPS. I have been
curious about the difference between access_ok(s, 0) and access_ok(s, 1)
until I saw __access_ok() on RISCV at arch/riscv/include/asm/uaccess.h

The __access_ok() is noted with `Ensure that the range [addr, addr+size)
is within the process's address space`. Does the range checked by
__access_ok() on MIPS is [addr, addr+size]. So if we want to use
access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?

More importantly, the implementation of strnlen_user in lib/strnlen_user.c
is noted `we hit the address space limit, and we still had more characters
the caller would have wanted. That's 0.` Does it make sense? It is not
achieved on MIPS when hit __ua_limit, if only access_ok(s, 1) is used.

Thanks,
Jinyang

RE: [PATCH] MIPS: Fix strnlen_user access check

[David Laight]

From: Jinyang He
> Sent: 13 April 2021 02:16
> 
> > On Mon, Apr 12, 2021 at 11:02:19AM +0800, Tiezhu Yang wrote:
> >> On 04/11/2021 07:04 PM, Jinyang He wrote:
> >>> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
> >>> strnlen_user(). Jump out when checking access_ok() with condition that
> >>> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
> >>> just checked (ua_limit & s) without checking (ua_limit & (s + n)).
> >>> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
> >>>
> >>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> >>> ---
> >>>    arch/mips/include/asm/uaccess.h | 11 +++++++++--
> >>>    1 file changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> >>> index 91bc7fb..85ba0c8 100644
> >>> --- a/arch/mips/include/asm/uaccess.h
> >>> +++ b/arch/mips/include/asm/uaccess.h
> >>> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n)
> >>>    {
> >>>    	long res;
> >>> -	if (!access_ok(s, n))
> >>> -		return -0;
> >>> +	if (unlikely(n <= 0))
> >>> +		return 0;
> >>> +
> >>> +	if (!access_ok(s, n)) {
> >>> +		if (!access_ok(s, 0))
> >>> +			return 0;
> >>> +
> >>> +		n = __UA_LIMIT - (unsigned long)s - 1;
> >>> +	}
> >>>    	might_fault();
> >>>    	__asm__ __volatile__(
> >> The following simple changes are OK to fix this issue?
> >>
> >> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> >> index 91bc7fb..eafc99b 100644
> >> --- a/arch/mips/include/asm/uaccess.h
> >> +++ b/arch/mips/include/asm/uaccess.h
> >> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n)
> >>   {
> >>          long res;
> >> -       if (!access_ok(s, n))
> >> -               return -0;
> >> +       if (!access_ok(s, 1))
> >> +               return 0;
> >>          might_fault();
> >>          __asm__ __volatile__(
> > that's the fix I'd like to apply. Could someone send it as a formal
> > patch ? Thanks.
> >
> > Thomas.
> >
> Hi, Thomas,
> 
> Thank you for bringing me more thinking.
> 
> I always think it is better to use access_ok(s, 0) on MIPS. I have been
> curious about the difference between access_ok(s, 0) and access_ok(s, 1)
> until I saw __access_ok() on RISCV at arch/riscv/include/asm/uaccess.h
> 
> The __access_ok() is noted with `Ensure that the range [addr, addr+size)
> is within the process's address space`. Does the range checked by
> __access_ok() on MIPS is [addr, addr+size]. So if we want to use
> access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?

ISTR that access_ok(xxx, 0) is unconditionally true on some architectures.
The range checked should be [addr, addr+size).
These are needed so that write(fd, random(), 0) doesn't ever fault.

> More importantly, the implementation of strnlen_user in lib/strnlen_user.c
> is noted `we hit the address space limit, and we still had more characters
> the caller would have wanted. That's 0.` Does it make sense? It is not
> achieved on MIPS when hit __ua_limit, if only access_ok(s, 1) is used.

There is the question of whether one call to access_ok(addr, 1) is
sufficient for any code that does sequential accesses.
It is if there is an unmapped page between the last valid user page
and the first valid kernel page.
IIRC x86 has such an unmapped page because 'horrid things' (tm) happen
if the cpu prefetches across the user-kernel boundary.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] MIPS: Fix strnlen_user access check

[Thomas Bogendoerfer]

On Tue, Apr 13, 2021 at 09:15:48AM +0800, Jinyang He wrote:
> On 04/12/2021 10:27 PM, Thomas Bogendoerfer wrote:
> > > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> > > index 91bc7fb..eafc99b 100644
> > > --- a/arch/mips/include/asm/uaccess.h
> > > +++ b/arch/mips/include/asm/uaccess.h
> > > @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n)
> > >   {
> > >          long res;
> > > -       if (!access_ok(s, n))
> > > -               return -0;
> > > +       if (!access_ok(s, 1))
> > > +               return 0;
> > >          might_fault();
> > >          __asm__ __volatile__(
> > that's the fix I'd like to apply. Could someone send it as a formal
> > patch ? Thanks.
> > 
> > Thomas.
> > 
> Hi, Thomas,
> 
> I always think it is better to use access_ok(s, 0) on MIPS. I have been
> curious about the difference between access_ok(s, 0) and access_ok(s, 1)
> until I saw __access_ok() on RISCV at arch/riscv/include/asm/uaccess.h
> 
> The __access_ok() is noted with `Ensure that the range [addr, addr+size)
> is within the process's address space`. Does the range checked by
> __access_ok() on MIPS is [addr, addr+size]. So if we want to use
> access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?

you are right, I'm going to apply

https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/

to fix that.

> More importantly, the implementation of strnlen_user in lib/strnlen_user.c
> is noted `we hit the address space limit, and we still had more characters
> the caller would have wanted. That's 0.` Does it make sense? It is not
> achieved on MIPS when hit __ua_limit, if only access_ok(s, 1) is used.

see the comment in arch/mips/lib/strnlen_user.S

 * Note: for performance reasons we deliberately accept that a user may
 *       make strlen_user and strnlen_user access the first few KSEG0
 *       bytes.  There's nothing secret there.  On 64-bit accessing beyond
 *       the maximum is a tad hairier ...

for 32bit kernels strnlen_user could possibly access KSEG0 and will find
a 0 sooner or later. I don't see much problems there. For 64bit kernels
strnlen_user will stop inside user space as there will be nothing
mapped after __UA_LIMIT.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

RE: [PATCH] MIPS: Fix strnlen_user access check

[David Laight]

From: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Sent: 13 April 2021 12:15
...
> > The __access_ok() is noted with `Ensure that the range [addr, addr+size)
> > is within the process's address space`. Does the range checked by
> > __access_ok() on MIPS is [addr, addr+size]. So if we want to use
> > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?
> 
> you are right, I'm going to apply
> 
> https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/
> 
> to fix that.

Isn't that still wrong?
If an application does:
	write(fd, (void *)0xffff0000, 0);
it should return 0, not -1 and EFAULT/SIGSEGV.

There is also the question about why this makes any difference
to the original problem of logging in via the graphical interface.

ISTM that it is very unlikely that the length passed to strnlen_user()
is long enough to take potential buffer beyond the end of user
address space.
It might be that it is passing 'huge' to do strlen_user().
But since the remove set_fs() changes are reported to have
broken it, was it actually being called for a kernel buffer?

There is more going on here.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] MIPS: Fix strnlen_user access check

[Thomas Bogendoerfer]

On Tue, Apr 13, 2021 at 12:37:25PM +0000, David Laight wrote:
> From: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Sent: 13 April 2021 12:15
> ...
> > > The __access_ok() is noted with `Ensure that the range [addr, addr+size)
> > > is within the process's address space`. Does the range checked by
> > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use
> > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?
> > 
> > you are right, I'm going to apply
> > 
> > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/
> > 
> > to fix that.
> 
> Isn't that still wrong?
> If an application does:
> 	write(fd, (void *)0xffff0000, 0);
> it should return 0, not -1 and EFAULT/SIGSEGV.

WRITE(2)                   Linux Programmer's Manual                  WRITE(2)
[...]
       If  count  is  zero  and  fd refers to a regular file, then write() may
       return a failure status if one of the errors below is detected.  If  no
       errors  are  detected,  or  error detection is not performed, 0 will be
       returned without causing any other effect.  If count  is  zero  and  fd
       refers  to a file other than a regular file, the results are not speci-
       fied.
[...]
       EFAULT buf is outside your accessible address space.

at least it's covered by the man page on my Linux system.

> There is also the question about why this makes any difference
> to the original problem of logging in via the graphical interface.

kernel/module.c:        mod->args = strndup_user(uargs, ~0UL >> 1);

and strndup_user does a strnlen_user.

> ISTM that it is very unlikely that the length passed to strnlen_user()
> is long enough to take potential buffer beyond the end of user
> address space.

see above.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

RE: [PATCH] MIPS: Fix strnlen_user access check

[David Laight]

From: Thomas Bogendoerfer
> Sent: 13 April 2021 16:19
> 
> On Tue, Apr 13, 2021 at 12:37:25PM +0000, David Laight wrote:
> > From: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > Sent: 13 April 2021 12:15
> > ...
> > > > The __access_ok() is noted with `Ensure that the range [addr, addr+size)
> > > > is within the process's address space`. Does the range checked by
> > > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use
> > > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?
> > >
> > > you are right, I'm going to apply
> > >
> > > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/
> > >
> > > to fix that.
> >
> > Isn't that still wrong?
> > If an application does:
> > 	write(fd, (void *)0xffff0000, 0);
> > it should return 0, not -1 and EFAULT/SIGSEGV.
> 
> WRITE(2)                   Linux Programmer's Manual                  WRITE(2)
> [...]
>        If  count  is  zero  and  fd refers to a regular file, then write() may
>        return a failure status if one of the errors below is detected.  If  no
>        errors  are  detected,  or  error detection is not performed, 0 will be
>        returned without causing any other effect.  If count  is  zero  and  fd
>        refers  to a file other than a regular file, the results are not speci-
>        fied.
> [...]
>        EFAULT buf is outside your accessible address space.
> 
> at least it's covered by the man page on my Linux system.

Something related definitely caused grief in the setsockopt() changes.

> > There is also the question about why this makes any difference
> > to the original problem of logging in via the graphical interface.
> 
> kernel/module.c:        mod->args = strndup_user(uargs, ~0UL >> 1);
> 
> and strndup_user does a strnlen_user.

That call is just gross.
Why did it work before the removal of set_fs() etc.

Or was there another change that affected strndup_user() ?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] MIPS: Fix strnlen_user access check

[Thomas Bogendoerfer]

On Tue, Apr 13, 2021 at 04:01:13PM +0000, David Laight wrote:
> From: Thomas Bogendoerfer
> > Sent: 13 April 2021 16:19
> > 
> > On Tue, Apr 13, 2021 at 12:37:25PM +0000, David Laight wrote:
> > > From: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > > Sent: 13 April 2021 12:15
> > > ...
> > > > > The __access_ok() is noted with `Ensure that the range [addr, addr+size)
> > > > > is within the process's address space`. Does the range checked by
> > > > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use
> > > > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding?
> > > >
> > > > you are right, I'm going to apply
> > > >
> > > > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/
> > > >
> > > > to fix that.
> > >
> > > Isn't that still wrong?
> > > If an application does:
> > > 	write(fd, (void *)0xffff0000, 0);
> > > it should return 0, not -1 and EFAULT/SIGSEGV.
> > 
> > WRITE(2)                   Linux Programmer's Manual                  WRITE(2)
> > [...]
> >        If  count  is  zero  and  fd refers to a regular file, then write() may
> >        return a failure status if one of the errors below is detected.  If  no
> >        errors  are  detected,  or  error detection is not performed, 0 will be
> >        returned without causing any other effect.  If count  is  zero  and  fd
> >        refers  to a file other than a regular file, the results are not speci-
> >        fied.
> > [...]
> >        EFAULT buf is outside your accessible address space.
> > 
> > at least it's covered by the man page on my Linux system.
> 
> Something related definitely caused grief in the setsockopt() changes.
> 
> > > There is also the question about why this makes any difference
> > > to the original problem of logging in via the graphical interface.
> > 
> > kernel/module.c:        mod->args = strndup_user(uargs, ~0UL >> 1);
> > 
> > and strndup_user does a strnlen_user.
> 
> That call is just gross.
> Why did it work before the removal of set_fs() etc.

strnlen_user just did the equivalent of access_ok(s, 0) and I copy&pasted
the wrong access_ok() statement :-( 

> Or was there another change that affected strndup_user() ?

no, just the change in strnlen_user.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Fix strnlen_user access check

[Jinyang He]

On 04/11/2021 07:04 PM, Jinyang He wrote:
> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for
> strnlen_user(). Jump out when checking access_ok() with condition that
> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm()
> just checked (ua_limit & s) without checking (ua_limit & (s + n)).
> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition.
>
> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
> ---
>   arch/mips/include/asm/uaccess.h | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> index 91bc7fb..85ba0c8 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n)
>   {
>   	long res;
>   
> -	if (!access_ok(s, n))
> -		return -0;
> +	if (unlikely(n <= 0))
> +		return 0;
> +
> +	if (!access_ok(s, n)) {
> +		if (!access_ok(s, 0))
> +			return 0;
> +
> +		n = __UA_LIMIT - (unsigned long)s - 1;
Sorry for here and it should be ~__UA_LIMIT...
earlier discussed:
https://patchwork.kernel.org/project/linux-mips/patch/1618139092-4018-1-git-send-email-hejinyang@loongson.cn/#24107107

Hope other comment. :-)

Jinyang

> +	}
>   
>   	might_fault();
>   	__asm__ __volatile__(

[PATCH] MIPS: Fix strnlen_user access check

[Thomas Bogendoerfer]

Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") changed the access_ok
for strnlen_user to check the whole range, which broke some callers
of strndup_user(). Restore the old behaviour and just check the first byte.

Fixes: 04324f44cb69 ("MIPS: Remove get_fs/set_fs")
Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
 arch/mips/include/asm/uaccess.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
index ab47e597656a..783fecce65c8 100644
--- a/arch/mips/include/asm/uaccess.h
+++ b/arch/mips/include/asm/uaccess.h
@@ -614,8 +614,8 @@ static inline long strnlen_user(const char __user *s, long n)
 {
 	long res;
 
-	if (!access_ok(s, n))
-		return -0;
+	if (!access_ok(s, 1))
+		return 0;
 
 	might_fault();
 	__asm__ __volatile__(
-- 
2.29.2

Re: [PATCH] MIPS: Fix strnlen_user access check

[Thomas Bogendoerfer]

On Thu, Apr 15, 2021 at 11:26:40PM +0200, Thomas Bogendoerfer wrote:
> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") changed the access_ok
> for strnlen_user to check the whole range, which broke some callers
> of strndup_user(). Restore the old behaviour and just check the first byte.
> 
> Fixes: 04324f44cb69 ("MIPS: Remove get_fs/set_fs")
> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> ---
>  arch/mips/include/asm/uaccess.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]