[RFC,2/4] lib/strncpy_from_user: Remove redundant user space pointer range check

This came up when switching ARC to word-at-a-time interface and using
generic/optimized strncpy_from_user

It seems the existing code checks for user buffer/string range multiple
times and one of tem cn be avoided.

There's an open-coded range check which computes @max off of user_addr_max()
and thus typically way larger than the kernel buffer @count and subsequently
discarded in do_strncpy_from_user()

	if (max > count)
		max = count;

The canonical user_access_begin() => access_ok() follow anyways and even
with @count it should suffice for an intial range check as is true for
any copy_{to,from}_user()

And in case actual user space buffer is smaller than kernel dest pointer
(i.e. @max < @count) the usual string copy, null byte detection would
abort the process early anyways

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 lib/strncpy_from_user.c | 36 +++++++++++-------------------------
 lib/strnlen_user.c      | 28 +++++++---------------------
 2 files changed, 18 insertions(+), 46 deletions(-)

On Tue, Jan 14, 2020 at 12:09 PM Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
>
> This came up when switching ARC to word-at-a-time interface and using
> generic/optimized strncpy_from_user
>
> It seems the existing code checks for user buffer/string range multiple
> times and one of tem cn be avoided.

NO!

DO NOT DO THIS.

This is seriously buggy.

>  long strncpy_from_user(char *dst, const char __user *src, long count)
>  {
> -       unsigned long max_addr, src_addr;
> -
>         if (unlikely(count <= 0))
>                 return 0;
>
> -       max_addr = user_addr_max();
> -       src_addr = (unsigned long)untagged_addr(src);
> -       if (likely(src_addr < max_addr)) {
> -               unsigned long max = max_addr - src_addr;
> +       kasan_check_write(dst, count);
> +       check_object_size(dst, count, false);
> +       if (user_access_begin(src, count)) {

You can't do that "user_access_begin(src, count)", because "count" is
the maximum _possible_ length, but it is *NOT* necessarily the actual
length of the string we really get from user space!

Think of this situation:

 - user has a 5-byte string at the end of the address space

 - kernel does a

     n = strncpy_from_user(uaddr, page, PAGE_SIZE)

now your "user_access_begin(src, count)" will _fail_, because "uaddr"
is close to the end of the user address space, and there's not room
for PAGE_SIZE bytes any more.

But "count" isn't actually how many bytes we will access from user
space, it's only the maximum limit on the *target*. IOW, it's about a
kernel buffer size, not about the user access size.

Because we'll only access that 5-byte string, which fits just fine in
the user space, and doing that "user_access_begin(src, count)" gives
the wrong answer.

The fact is, copying a string from user space is *very* different from
copying a fixed number of bytes, and that whole dance with

        max_addr = user_addr_max();

is absolutely required and necessary.

You completely broke string copying.

It is very possible that string copying was horribly broken on ARC
before too - almost nobody ever gets this right, but the generic
routine does.

So the generic routine is not only faster, it is *correct*, and your
change broke it.

Don't touch generic code. If you want to use the generic code, please
do so. But DO NOT TOUCH IT. It is correct, your patch is wrong.

The exact same issue is true in strnlen_user(). Don't break it.

              Linus

On 1/14/20 1:22 PM, Linus Torvalds wrote:
> On Tue, Jan 14, 2020 at 12:09 PM Vineet Gupta
> <Vineet.Gupta1@synopsys.com> wrote:
>>
>> This came up when switching ARC to word-at-a-time interface and using
>> generic/optimized strncpy_from_user
>>
>> It seems the existing code checks for user buffer/string range multiple
>> times and one of tem cn be avoided.
> 
> NO!
> 
> DO NOT DO THIS.
> 
> This is seriously buggy.
> 
>>  long strncpy_from_user(char *dst, const char __user *src, long count)
>>  {
>> -       unsigned long max_addr, src_addr;
>> -
>>         if (unlikely(count <= 0))
>>                 return 0;
>>
>> -       max_addr = user_addr_max();
>> -       src_addr = (unsigned long)untagged_addr(src);
>> -       if (likely(src_addr < max_addr)) {
>> -               unsigned long max = max_addr - src_addr;
>> +       kasan_check_write(dst, count);
>> +       check_object_size(dst, count, false);
>> +       if (user_access_begin(src, count)) {
> 
> You can't do that "user_access_begin(src, count)", because "count" is
> the maximum _possible_ length, but it is *NOT* necessarily the actual
> length of the string we really get from user space!
> 
> Think of this situation:
> 
>  - user has a 5-byte string at the end of the address space
> 
>  - kernel does a
> 
>      n = strncpy_from_user(uaddr, page, PAGE_SIZE)
> 
> now your "user_access_begin(src, count)" will _fail_, because "uaddr"
> is close to the end of the user address space, and there's not room
> for PAGE_SIZE bytes any more.

Oops indeed that was the case I didn't comprehend. In my initial tests with
debugger, every single hit on strncpy_from_user() had user addresses well into the
address space such that @max was ridiculously large (0xFFFF_FFFF - ptr) compared
to @count.

> But "count" isn't actually how many bytes we will access from user
> space, it's only the maximum limit on the *target*. IOW, it's about a
> kernel buffer size, not about the user access size.

Right I understood all that, but missed the case when user buffer is towards end
of address space and access_ok() will erroneously flag it.

> Because we'll only access that 5-byte string, which fits just fine in
> the user space, and doing that "user_access_begin(src, count)" gives
> the wrong answer.
> 
> The fact is, copying a string from user space is *very* different from
> copying a fixed number of bytes, and that whole dance with
> 
>         max_addr = user_addr_max();
> 
> is absolutely required and necessary.
> 
> You completely broke string copying.

I'm sorry and I wasn't sure to begin with hence the disclaimer in 0/4

> It is very possible that string copying was horribly broken on ARC
> before too - almost nobody ever gets this right, but the generic
> routine does.

No it is not. It is just dog slow since it does byte copy and uses the Zero delay
loops which I'm trying to get rid of. That's when I recalled the word-at-a-time
API which I'd meaning to go back to for last 7 years :-)

-Vineet

On Tue, Jan 14, 2020 at 01:22:07PM -0800, Linus Torvalds wrote:

> The fact is, copying a string from user space is *very* different from
> copying a fixed number of bytes, and that whole dance with
> 
>         max_addr = user_addr_max();
> 
> is absolutely required and necessary.
> 
> You completely broke string copying.

BTW, a quick grep through the callers has found something odd -
static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
                              size_t size, loff_t *ppos)
{
        char buf[64];
        int buf_size;
        int ret;

        buf_size = min(size, (sizeof(buf) - 1));
        if (strncpy_from_user(buf, user_buf, buf_size) < 0)
                return -EFAULT;
        buf[buf_size] = 0;

What the hell?  If somebody is calling write(fd, buf, n) they'd
better be ready to see any byte from buf[0] up to buf[n - 1]
fetched, and if something is unmapped - deal with -EFAULT.
Is something really doing that and if so, why does kmemleak
try to accomodate that idiocy?

The same goes for several more ->write() instances - mtrr_write(),
armada_debugfs_crtc_reg_write() and cio_ignore_write(); IMO that's
seriously misguided (and cio one ought use vmemdup_user() instead
of what it's doing)...

On Tue, Jan 14, 2020 at 9:08 PM Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>
> This came up when switching ARC to word-at-a-time interface and using
> generic/optimized strncpy_from_user
>
> It seems the existing code checks for user buffer/string range multiple
> times and one of tem cn be avoided.
>
> There's an open-coded range check which computes @max off of user_addr_max()
> and thus typically way larger than the kernel buffer @count and subsequently
> discarded in do_strncpy_from_user()
>
>         if (max > count)
>                 max = count;
>
> The canonical user_access_begin() => access_ok() follow anyways and even
> with @count it should suffice for an intial range check as is true for
> any copy_{to,from}_user()
>
> And in case actual user space buffer is smaller than kernel dest pointer
> (i.e. @max < @count) the usual string copy, null byte detection would
> abort the process early anyways
>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  lib/strncpy_from_user.c | 36 +++++++++++-------------------------
>  lib/strnlen_user.c      | 28 +++++++---------------------
>  2 files changed, 18 insertions(+), 46 deletions(-)
>
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index dccb95af6003..a1622d71f037 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -21,22 +21,15 @@
>  /*
>   * Do a strncpy, return length of string without final '\0'.
>   * 'count' is the user-supplied count (return 'count' if we
> - * hit it), 'max' is the address space maximum (and we return
> - * -EFAULT if we hit it).
> + * hit it). If access fails, return -EFAULT.
>   */
>  static inline long do_strncpy_from_user(char *dst, const char __user *src,
> -                                       unsigned long count, unsigned long max)
> +                                       unsigned long count)
>  {
>         const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
> +       unsigned long max = count;
>         unsigned long res = 0;
>
> -       /*
> -        * Truncate 'max' to the user-specified limit, so that
> -        * we only have one limit we need to check in the loop
> -        */
> -       if (max > count)
> -               max = count;
> -
>         if (IS_UNALIGNED(src, dst))
>                 goto byte_at_a_time;
>
> @@ -72,7 +65,7 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
>          * Uhhuh. We hit 'max'. But was that the user-specified maximum
>          * too? If so, that's ok - we got as much as the user asked for.
>          */
> -       if (res >= count)
> +       if (res == count)
>                 return res;
>
>         /*
> @@ -103,25 +96,18 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
>   */
>  long strncpy_from_user(char *dst, const char __user *src, long count)
>  {
> -       unsigned long max_addr, src_addr;
> -
>         if (unlikely(count <= 0))
>                 return 0;
>
> -       max_addr = user_addr_max();
> -       src_addr = (unsigned long)untagged_addr(src);

If you end up changing this code, you need to keep the untagged_addr()
logic, otherwise this breaks arm64 tagged address ABI [1].

[1] https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html

> -       if (likely(src_addr < max_addr)) {
> -               unsigned long max = max_addr - src_addr;
> +       kasan_check_write(dst, count);
> +       check_object_size(dst, count, false);
> +       if (user_access_begin(src, count)) {
>                 long retval;
> -
> -               kasan_check_write(dst, count);
> -               check_object_size(dst, count, false);
> -               if (user_access_begin(src, max)) {
> -                       retval = do_strncpy_from_user(dst, src, count, max);
> -                       user_access_end();
> -                       return retval;
> -               }
> +               retval = do_strncpy_from_user(dst, src, count);
> +               user_access_end();
> +               return retval;
>         }
> +
>         return -EFAULT;
>  }
>  EXPORT_SYMBOL(strncpy_from_user);
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 6c0005d5dd5c..5ce61f303d6e 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -20,19 +20,13 @@
>   * if it fits in a aligned 'long'. The caller needs to check
>   * the return value against "> max".
>   */
> -static inline long do_strnlen_user(const char __user *src, unsigned long count, unsigned long max)
> +static inline long do_strnlen_user(const char __user *src, unsigned long count)
>  {
>         const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
>         unsigned long align, res = 0;
> +       unsigned long max = count;
>         unsigned long c;
>
> -       /*
> -        * Truncate 'max' to the user-specified limit, so that
> -        * we only have one limit we need to check in the loop
> -        */
> -       if (max > count)
> -               max = count;
> -
>         /*
>          * Do everything aligned. But that means that we
>          * need to also expand the maximum..
> @@ -64,7 +58,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
>          * Uhhuh. We hit 'max'. But was that the user-specified maximum
>          * too? If so, return the marker for "too long".
>          */
> -       if (res >= count)
> +       if (res == count)
>                 return count+1;
>
>         /*
> @@ -98,22 +92,14 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
>   */
>  long strnlen_user(const char __user *str, long count)
>  {
> -       unsigned long max_addr, src_addr;
> -
>         if (unlikely(count <= 0))
>                 return 0;
>
> -       max_addr = user_addr_max();
> -       src_addr = (unsigned long)untagged_addr(str);
> -       if (likely(src_addr < max_addr)) {
> -               unsigned long max = max_addr - src_addr;
> +       if (user_access_begin(str, count)) {
>                 long retval;
> -
> -               if (user_access_begin(str, max)) {
> -                       retval = do_strnlen_user(str, count, max);
> -                       user_access_end();
> -                       return retval;
> -               }
> +               retval = do_strnlen_user(str, count);
> +               user_access_end();
> +               return retval;
>         }
>         return 0;
>  }
> --
> 2.20.1
>

On 1/15/20 6:42 AM, Andrey Konovalov wrote:
>> -       max_addr = user_addr_max();
>> -       src_addr = (unsigned long)untagged_addr(src);
>
> If you end up changing this code, you need to keep the untagged_addr()
> logic, otherwise this breaks arm64 tagged address ABI [1].

It is moot point now, but fwiw untagged_addr() would not have been needed anymore
as it was only needed to compute the pointer difference which my patch got rid of.

> 
> [1] https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html
> 
>> -       if (likely(src_addr < max_addr)) {
>> -               unsigned long max = max_addr - src_addr;
>> +       kasan_check_write(dst, count);
>> +       check_object_size(dst, count, false);
>> +       if (user_access_begin(src, count)) {