On 1/14/20 1:22 PM, Linus Torvalds wrote:
> On Tue, Jan 14, 2020 at 12:09 PM Vineet Gupta
> <vineet.gup...@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!
> 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 
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 
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 :-)


linux-snps-arc mailing list

Reply via email to