Linux SNPS ARC Archive on lore.kernel.org
 help / color / Atom feed
From: Vineet Gupta <Vineet.Gupta1@synopsys.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-arch <linux-arch@vger.kernel.org>,
	Kees Cook <keescook@chromium.org>, Arnd Bergmann <arnd@arndb.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrey Konovalov <andreyknvl@google.com>,
	Aleksa Sarai <cyphar@cyphar.com>, Ingo Molnar <mingo@kernel.org>,
	Khalid Aziz <khalid.aziz@oracle.com>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	"linux-snps-arc@lists.infradead.org"
	<linux-snps-arc@lists.infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RFC 2/4] lib/strncpy_from_user: Remove redundant user space pointer range check
Date: Tue, 14 Jan 2020 21:52:47 +0000
Message-ID: <72451406-6575-c16e-057e-30dc68ed9d2f@synopsys.com> (raw)
In-Reply-To: <CAHk-=wgoc5DaF6=WxsAcft_Lp4XUYTiRhhCJGcmM5PwEDXY6Gw@mail.gmail.com>

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

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14 20:08 [RFC 0/4] Switching ARC to optimized generic strncpy_from_user Vineet Gupta
2020-01-14 20:08 ` [RFC 1/4] asm-generic/uaccess: don't define inline functions if noinline lib/* in use Vineet Gupta
2020-01-14 20:57   ` Arnd Bergmann
2020-01-15 23:01     ` Vineet Gupta
2020-01-16 11:43       ` Arnd Bergmann
2020-01-14 21:32   ` Linus Torvalds
2020-01-15  9:08     ` Arnd Bergmann
2020-01-15 14:12       ` Al Viro
2020-01-15 14:21         ` Arnd Bergmann
2020-01-14 20:08 ` [RFC 2/4] lib/strncpy_from_user: Remove redundant user space pointer range check Vineet Gupta
2020-01-14 21:22   ` Linus Torvalds
2020-01-14 21:52     ` Vineet Gupta [this message]
2020-01-14 23:46     ` Al Viro
2020-01-15 14:42   ` Andrey Konovalov
2020-01-15 23:00     ` Vineet Gupta
2020-01-14 20:08 ` [RFC 3/4] ARC: uaccess: remove noinline variants of __strncpy_from_user() and friends Vineet Gupta
2020-01-14 20:08 ` [RFC 4/4] ARC: uaccess: use optimized generic __strnlen_user/__strncpy_from_user Vineet Gupta
2020-01-14 20:42   ` Arnd Bergmann
2020-01-14 21:36     ` Vineet Gupta
2020-01-14 21:49       ` Linus Torvalds
2020-01-14 22:14         ` Vineet Gupta

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=72451406-6575-c16e-057e-30dc68ed9d2f@synopsys.com \
    --to=vineet.gupta1@synopsys.com \
    --cc=akpm@linux-foundation.org \
    --cc=andreyknvl@google.com \
    --cc=arnd@arndb.de \
    --cc=christian.brauner@ubuntu.com \
    --cc=cyphar@cyphar.com \
    --cc=keescook@chromium.org \
    --cc=khalid.aziz@oracle.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-snps-arc@lists.infradead.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux SNPS ARC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-snps-arc/0 linux-snps-arc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-snps-arc linux-snps-arc/ https://lore.kernel.org/linux-snps-arc \
		linux-snps-arc@lists.infradead.org
	public-inbox-index linux-snps-arc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-snps-arc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git