From: Linus Torvalds <torvalds@linux-foundation.org> To: Vineet Gupta <Vineet.Gupta1@synopsys.com> Cc: Arnd Bergmann <arnd@arndb.de>, Khalid Aziz <khalid.aziz@oracle.com>, Andrey Konovalov <andreyknvl@google.com>, Andrew Morton <akpm@linux-foundation.org>, Peter Zijlstra <peterz@infradead.org>, Christian Brauner <christian.brauner@ubuntu.com>, Kees Cook <keescook@chromium.org>, Ingo Molnar <mingo@kernel.org>, Aleksa Sarai <cyphar@cyphar.com>, linux-snps-arc@lists.infradead.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-arch <linux-arch@vger.kernel.org> Subject: Re: [RFC 2/4] lib/strncpy_from_user: Remove redundant user space pointer range check Date: Tue, 14 Jan 2020 13:22:07 -0800 [thread overview] Message-ID: <CAHk-=wgoc5DaF6=WxsAcft_Lp4XUYTiRhhCJGcmM5PwEDXY6Gw@mail.gmail.com> (raw) In-Reply-To: <20200114200846.29434-3-vgupta@synopsys.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org> To: Vineet Gupta <Vineet.Gupta1@synopsys.com> 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, 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 13:22:07 -0800 [thread overview] Message-ID: <CAHk-=wgoc5DaF6=WxsAcft_Lp4XUYTiRhhCJGcmM5PwEDXY6Gw@mail.gmail.com> (raw) In-Reply-To: <20200114200846.29434-3-vgupta@synopsys.com> 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 _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
next prev parent reply other threads:[~2020-01-14 21:22 UTC|newest] Thread overview: 56+ 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 ` Vineet Gupta 2020-01-14 20:08 ` 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:08 ` Vineet Gupta 2020-01-14 20:57 ` Arnd Bergmann 2020-01-14 20:57 ` Arnd Bergmann 2020-01-14 20:57 ` Arnd Bergmann 2020-01-15 23:01 ` Vineet Gupta 2020-01-15 23:01 ` Vineet Gupta 2020-01-15 23:01 ` Vineet Gupta 2020-01-16 11:43 ` Arnd Bergmann 2020-01-16 11:43 ` Arnd Bergmann 2020-01-16 11:43 ` Arnd Bergmann 2020-01-14 21:32 ` Linus Torvalds 2020-01-14 21:32 ` Linus Torvalds 2020-01-15 9:08 ` Arnd Bergmann 2020-01-15 9:08 ` Arnd Bergmann 2020-01-15 9:08 ` Arnd Bergmann 2020-01-15 14:12 ` Al Viro 2020-01-15 14:12 ` Al Viro 2020-01-15 14:12 ` Al Viro 2020-01-15 14:21 ` Arnd Bergmann 2020-01-15 14:21 ` Arnd Bergmann 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 20:08 ` Vineet Gupta 2020-01-14 21:22 ` Linus Torvalds [this message] 2020-01-14 21:22 ` Linus Torvalds 2020-01-14 21:52 ` Vineet Gupta 2020-01-14 21:52 ` Vineet Gupta 2020-01-14 21:52 ` Vineet Gupta 2020-01-14 23:46 ` Al Viro 2020-01-14 23:46 ` Al Viro 2020-01-15 14:42 ` Andrey Konovalov 2020-01-15 14:42 ` Andrey Konovalov 2020-01-15 14:42 ` Andrey Konovalov 2020-01-15 23:00 ` Vineet Gupta 2020-01-15 23:00 ` Vineet Gupta 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 ` 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:08 ` Vineet Gupta 2020-01-14 20:42 ` Arnd Bergmann 2020-01-14 20:42 ` Arnd Bergmann 2020-01-14 20:42 ` Arnd Bergmann 2020-01-14 21:36 ` Vineet Gupta 2020-01-14 21:36 ` Vineet Gupta 2020-01-14 21:36 ` Vineet Gupta 2020-01-14 21:49 ` Linus Torvalds 2020-01-14 21:49 ` Linus Torvalds 2020-01-14 21:49 ` Linus Torvalds 2020-01-14 22:14 ` Vineet Gupta 2020-01-14 22:14 ` Vineet Gupta 2020-01-14 22:14 ` Vineet Gupta
Reply instructions: You may reply publicly 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='CAHk-=wgoc5DaF6=WxsAcft_Lp4XUYTiRhhCJGcmM5PwEDXY6Gw@mail.gmail.com' \ --to=torvalds@linux-foundation.org \ --cc=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 \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.