Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1470413 > unrolled thread
| Started by | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| First post | 2016-08-25 23:00 +0200 |
| Last post | 2016-08-30 20:40 +0200 |
| Articles | 20 on this page of 23 — 5 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 23:00 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-26 04:30 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 06:00 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-26 15:50 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 16:00 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 23:10 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 23:10 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-27 02:40 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 16:50 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-29 17:40 +0200
[PATCH v2] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 19:10 +0200
Re: [PATCH v2] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 20:10 +0200
[PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-30 15:10 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-30 19:10 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-30 20:20 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-30 21:10 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-30 21:30 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-30 22:20 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-31 00:30 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Mark Rutland <mark.rutland@arm.com> - 2016-08-31 11:50 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-30 20:20 +0200
Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-30 20:20 +0200
Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-30 20:40 +0200
Page 1 of 2 [1] 2 Next page →
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-25 23:00 +0200 |
| Subject | Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc |
| Message-ID | <sa9UK-619-13@gated-at.bofh.it> |
On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote:
> On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > This is a revert of:
> >
> > 2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")
> >
> > The goal of that commit was to silence the "provably correct" gcc
> > warnings. But it went too far: it also disabled the runtime warnings.
> >
> > Now that the pretty much useless gcc warnings have been properly
> > disposed of with the previous patch, re-enable this checking on modern
> > versions of gcc so we can get the runtime warnings again.
>
> As far as I know, this will still be broken since it's
> __builtin_object_size() that is buggy. Maybe I'm misunderstanding
> which piece is busted, though?
What specifically is buggy with __builtin_object_size()? Looking at the
generated code for a few of the "provably correct" warning sites, the
values generated by __builtin_object_size() are correct.
I think the problem is really related to the compile-time warning
function attribute used by __copy_to_user_overflow(). The warning is
printed when gcc *can* determine the object size but it *can't*
determine the copy size. The warning just means that, even though the
object has a const size, gcc isn't able to prove that the overflow won't
happen.
As an example, here's one of the warnings:
In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0,
from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9,
from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246,
from /home/jpoimboe/git/linux/include/linux/perf_event.h:24,
from /home/jpoimboe/git/linux/kernel/sys.c:16:
In function ‘copy_to_user.part.10’,
inlined from ‘copy_to_user’,
inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9:
/home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to ‘__copy_to_user_overflow’ declared with attribute warning: copy_to_user() buffer size is not provably correct
#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
^~~~~~~~~~~~~~~~~~~~~~~~~
/home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’
__copy_to_user_overflow(sz, n);
^~~~~~~~~~~~~~~~~~~~~~~
This is from override_release()'s use of copy_to_user(). The object
code shows that __builtin_object_size() correctly reports 65 bytes for
the 'buf' object size. But the copy size ('copy + 1') isn't known at
compile-time. Thus the (bogus) warning.
Maybe I'm missing something but I don't even see a gcc bug. To me it
looks like a mismatch in expectations between the code and the compiler.
--
Josh
[toc] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-26 04:30 +0200 |
| Subject | Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc |
| Message-ID | <saf46-Zl-9@gated-at.bofh.it> |
| In reply to | #1470413 |
On Thu, Aug 25, 2016 at 4:47 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote:
>> On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > This is a revert of:
>> >
>> > 2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")
>> >
>> > The goal of that commit was to silence the "provably correct" gcc
>> > warnings. But it went too far: it also disabled the runtime warnings.
>> >
>> > Now that the pretty much useless gcc warnings have been properly
>> > disposed of with the previous patch, re-enable this checking on modern
>> > versions of gcc so we can get the runtime warnings again.
>>
>> As far as I know, this will still be broken since it's
>> __builtin_object_size() that is buggy. Maybe I'm misunderstanding
>> which piece is busted, though?
>
> What specifically is buggy with __builtin_object_size()? Looking at the
> generated code for a few of the "provably correct" warning sites, the
> values generated by __builtin_object_size() are correct.
>
> I think the problem is really related to the compile-time warning
> function attribute used by __copy_to_user_overflow(). The warning is
> printed when gcc *can* determine the object size but it *can't*
> determine the copy size. The warning just means that, even though the
> object has a const size, gcc isn't able to prove that the overflow won't
> happen.
>
> As an example, here's one of the warnings:
>
> In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0,
> from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9,
> from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246,
> from /home/jpoimboe/git/linux/include/linux/perf_event.h:24,
> from /home/jpoimboe/git/linux/kernel/sys.c:16:
> In function ‘copy_to_user.part.10’,
> inlined from ‘copy_to_user’,
> inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9:
> /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to ‘__copy_to_user_overflow’ declared with attribute warning: copy_to_user() buffer size is not provably correct
> #define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’
> __copy_to_user_overflow(sz, n);
> ^~~~~~~~~~~~~~~~~~~~~~~
>
> This is from override_release()'s use of copy_to_user(). The object
> code shows that __builtin_object_size() correctly reports 65 bytes for
> the 'buf' object size. But the copy size ('copy + 1') isn't known at
> compile-time. Thus the (bogus) warning.
>
> Maybe I'm missing something but I don't even see a gcc bug. To me it
> looks like a mismatch in expectations between the code and the compiler.
Ah, yes, I had a total brain failure. This is what I get trying to do
email between sessions at a conference. :)
Okay, right. __builtin_object_size() is totally fine, I absolutely
misspoke: it's the resolution of const value ranges. I wouldn't expect
gcc to warn here, though, since "copy + 1" isn't a const value...
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-26 06:00 +0200 |
| Message-ID | <sagtc-1KQ-9@gated-at.bofh.it> |
| In reply to | #1470513 |
On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote:
> On Thu, Aug 25, 2016 at 4:47 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote:
> >> On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > This is a revert of:
> >> >
> >> > 2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")
> >> >
> >> > The goal of that commit was to silence the "provably correct" gcc
> >> > warnings. But it went too far: it also disabled the runtime warnings.
> >> >
> >> > Now that the pretty much useless gcc warnings have been properly
> >> > disposed of with the previous patch, re-enable this checking on modern
> >> > versions of gcc so we can get the runtime warnings again.
> >>
> >> As far as I know, this will still be broken since it's
> >> __builtin_object_size() that is buggy. Maybe I'm misunderstanding
> >> which piece is busted, though?
> >
> > What specifically is buggy with __builtin_object_size()? Looking at the
> > generated code for a few of the "provably correct" warning sites, the
> > values generated by __builtin_object_size() are correct.
> >
> > I think the problem is really related to the compile-time warning
> > function attribute used by __copy_to_user_overflow(). The warning is
> > printed when gcc *can* determine the object size but it *can't*
> > determine the copy size. The warning just means that, even though the
> > object has a const size, gcc isn't able to prove that the overflow won't
> > happen.
> >
> > As an example, here's one of the warnings:
> >
> > In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0,
> > from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9,
> > from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246,
> > from /home/jpoimboe/git/linux/include/linux/perf_event.h:24,
> > from /home/jpoimboe/git/linux/kernel/sys.c:16:
> > In function ‘copy_to_user.part.10’,
> > inlined from ‘copy_to_user’,
> > inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9:
> > /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to ‘__copy_to_user_overflow’ declared with attribute warning: copy_to_user() buffer size is not provably correct
> > #define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
> > ^~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’
> > __copy_to_user_overflow(sz, n);
> > ^~~~~~~~~~~~~~~~~~~~~~~
> >
> > This is from override_release()'s use of copy_to_user(). The object
> > code shows that __builtin_object_size() correctly reports 65 bytes for
> > the 'buf' object size. But the copy size ('copy + 1') isn't known at
> > compile-time. Thus the (bogus) warning.
> >
> > Maybe I'm missing something but I don't even see a gcc bug. To me it
> > looks like a mismatch in expectations between the code and the compiler.
>
> Ah, yes, I had a total brain failure. This is what I get trying to do
> email between sessions at a conference. :)
>
> Okay, right. __builtin_object_size() is totally fine, I absolutely
> misspoke: it's the resolution of const value ranges. I wouldn't expect
> gcc to warn here, though, since "copy + 1" isn't a const value...
Look at the code again :-)
__copy_to_user_overflow(), which does the "provably correct" warning, is
"called" when the copy size is non-const (and the object size is const).
So "copy + 1" being non-const is consistent with the warning.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-26 15:50 +0200 |
| Subject | Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc |
| Message-ID | <sapG9-7Ij-9@gated-at.bofh.it> |
| In reply to | #1470534 |
On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote: >> Okay, right. __builtin_object_size() is totally fine, I absolutely >> misspoke: it's the resolution of const value ranges. I wouldn't expect >> gcc to warn here, though, since "copy + 1" isn't a const value... > > Look at the code again :-) > > __copy_to_user_overflow(), which does the "provably correct" warning, is > "called" when the copy size is non-const (and the object size is const). > So "copy + 1" being non-const is consistent with the warning. Right, yes. Man, this is hard to read. All the names are the same. ;) So this will trigger when the object size is known but the copy length is non-const? When I played with re-enabling this in the past, I didn't hit very many false positives. I sent a bunch of patches a few months back for legitimate problems that this warning pointed out, so I'm a bit cautious to just entirely drop it. -Kees -- Kees Cook Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-26 16:00 +0200 |
| Message-ID | <sapPQ-7Lt-15@gated-at.bofh.it> |
| In reply to | #1470729 |
On Fri, Aug 26, 2016 at 09:42:42AM -0400, Kees Cook wrote: > On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote: > >> Okay, right. __builtin_object_size() is totally fine, I absolutely > >> misspoke: it's the resolution of const value ranges. I wouldn't expect > >> gcc to warn here, though, since "copy + 1" isn't a const value... > > > > Look at the code again :-) > > > > __copy_to_user_overflow(), which does the "provably correct" warning, is > > "called" when the copy size is non-const (and the object size is const). > > So "copy + 1" being non-const is consistent with the warning. > > Right, yes. Man, this is hard to read. All the names are the same. ;) Yeah, agreed. The code is way too cryptic. > So this will trigger when the object size is known but the copy length > is non-const? Right. > When I played with re-enabling this in the past, I didn't hit very > many false positives. I sent a bunch of patches a few months back for > legitimate problems that this warning pointed out, so I'm a bit > cautious to just entirely drop it. Ah, I didn't realize that. We should definitely keep DEBUG_STRICT_USER_COPY_CHECKS then. Though it would be *really* nice to find a way to associate some kind of whitelist with it to separate the wheat from all the chaff. -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-26 23:10 +0200 |
| Message-ID | <sawxX-3Rz-3@gated-at.bofh.it> |
| In reply to | #1470733 |
On Fri, Aug 26, 2016 at 08:55:33AM -0500, Josh Poimboeuf wrote: > On Fri, Aug 26, 2016 at 09:42:42AM -0400, Kees Cook wrote: > > On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote: > > >> Okay, right. __builtin_object_size() is totally fine, I absolutely > > >> misspoke: it's the resolution of const value ranges. I wouldn't expect > > >> gcc to warn here, though, since "copy + 1" isn't a const value... > > > > > > Look at the code again :-) > > > > > > __copy_to_user_overflow(), which does the "provably correct" warning, is > > > "called" when the copy size is non-const (and the object size is const). > > > So "copy + 1" being non-const is consistent with the warning. > > > > Right, yes. Man, this is hard to read. All the names are the same. ;) > > Yeah, agreed. The code is way too cryptic. > > > So this will trigger when the object size is known but the copy length > > is non-const? > > Right. > > > When I played with re-enabling this in the past, I didn't hit very > > many false positives. I sent a bunch of patches a few months back for > > legitimate problems that this warning pointed out, so I'm a bit > > cautious to just entirely drop it. > > Ah, I didn't realize that. We should definitely keep > DEBUG_STRICT_USER_COPY_CHECKS then. Though it would be *really* nice to > find a way to associate some kind of whitelist with it to separate the > wheat from all the chaff. Ok, so I could drop patch 1/2 and then resubmit 2/2 with an updated patch header. There's one problem with that though. It's going to annoy a lot of people who do allyesconfig/allmodconfig builds because DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings. Anybody know if there's a way to disable an option for allyesconfig/allmodconfig? -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-26 23:10 +0200 |
| Message-ID | <sawxX-3Rz-15@gated-at.bofh.it> |
| In reply to | #1470992 |
On Fri, Aug 26, 2016 at 03:56:27PM -0500, Josh Poimboeuf wrote: > On Fri, Aug 26, 2016 at 08:55:33AM -0500, Josh Poimboeuf wrote: > > On Fri, Aug 26, 2016 at 09:42:42AM -0400, Kees Cook wrote: > > > On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote: > > > >> Okay, right. __builtin_object_size() is totally fine, I absolutely > > > >> misspoke: it's the resolution of const value ranges. I wouldn't expect > > > >> gcc to warn here, though, since "copy + 1" isn't a const value... > > > > > > > > Look at the code again :-) > > > > > > > > __copy_to_user_overflow(), which does the "provably correct" warning, is > > > > "called" when the copy size is non-const (and the object size is const). > > > > So "copy + 1" being non-const is consistent with the warning. > > > > > > Right, yes. Man, this is hard to read. All the names are the same. ;) > > > > Yeah, agreed. The code is way too cryptic. > > > > > So this will trigger when the object size is known but the copy length > > > is non-const? > > > > Right. > > > > > When I played with re-enabling this in the past, I didn't hit very > > > many false positives. I sent a bunch of patches a few months back for > > > legitimate problems that this warning pointed out, so I'm a bit > > > cautious to just entirely drop it. > > > > Ah, I didn't realize that. We should definitely keep > > DEBUG_STRICT_USER_COPY_CHECKS then. Though it would be *really* nice to > > find a way to associate some kind of whitelist with it to separate the > > wheat from all the chaff. > > Ok, so I could drop patch 1/2 and then resubmit 2/2 with an updated > patch header. > > There's one problem with that though. It's going to annoy a lot of > people who do allyesconfig/allmodconfig builds because > DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings. > > Anybody know if there's a way to disable an option for > allyesconfig/allmodconfig? Hm, I guess that wouldn't be good enough anyway because the build bot randconfig builds woudn't be happy with the warnings either. Not sure how to keep the feature around without littering the landscape with false positives... -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-27 02:40 +0200 |
| Subject | Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc |
| Message-ID | <sazPb-5US-7@gated-at.bofh.it> |
| In reply to | #1470992 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Aug 26, 2016 at 1:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> There's one problem with that though. It's going to annoy a lot of
> people who do allyesconfig/allmodconfig builds because
> DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings.
How bad is it?
In particular, we've definitely had issues with the "warning"
attribute before. Because as you pointed out somewhere elsewhere, the
warrning can happen before the call is actually optimized away by a
later compiler phase.
In particular, we _have_ occasionally fixed this by turning it into a
link-time error instead (that's in fact the really traditional model).
That makes the errior happen much later, and the error message isn't
nearly as nice (you get something like "undefined reference to unknown
symbol '__copy_to_user_failed' in function xyz" without line numbers
etc nice things). But it cuts down on the false positives that come
from warnings triggering before the final code has actually been
generated.
So one option *might* be to make the copy_to_user checks do an
explicitly constant and static check like
if (__builtin_constant_p(n) && sz >= 0 && n > sz)
__copy_to_user_failed();
with the "__copy_to_user_failed()" function declared but never
defined. That way, at link time, if something still references it, you
get a link error and you'll know it's bad.
So something like the attached patch *might* work. As mentioned, it
makes the error messages much less legible if they happen, and it
delays them to link time, so it's not perfect. But it certainly has
the potential of avoiding bogus warnings.
It *seemed* to work in my quick allmodconfig build test, but that may
be because I screwed something up. So take that with a large pinch of
salt.
What do people think? The static built-time errors - if they happen -
really should be pretty exceptional and unusual. So maybe it's ok that
they then would be somewhat cryptic, and you'd have to maybe hunt
(possibly through several layers of inline functions) where the actual
offending user copy then ends up being..
So I'm not happy with this patch, but I also think that the false
positives make the *current* code simply unworkable with current gcc
versions.
Of course, somebody might be able to come up with a better approach
that still gets the nice error messages and avoids the false
positives.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-29 16:50 +0200 |
| Message-ID | <sbw2S-62-31@gated-at.bofh.it> |
| In reply to | #1471055 |
On Fri, Aug 26, 2016 at 05:37:20PM -0700, Linus Torvalds wrote: > On Fri, Aug 26, 2016 at 1:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > There's one problem with that though. It's going to annoy a lot of > > people who do allyesconfig/allmodconfig builds because > > DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings. > > How bad is it? > > In particular, we've definitely had issues with the "warning" > attribute before. Because as you pointed out somewhere elsewhere, the > warrning can happen before the call is actually optimized away by a > later compiler phase. So I *think* your patch fixes the wrong problem. That's probably at least somewhat my fault because I misunderstood the issue before and may have described it wrong at some point. AFAICT, gcc isn't doing anything wrong, and the false positives are "intentional". There are in fact two static warnings (which are being silenced for new versions of gcc): 1) "copy_from_user() buffer size is too small" This happens when object size and copy size are both const, and copy size > object size. I didn't see any false positives for this one. So the function warning attribute seems to be working fine here. Your patch "fixed" this warning, but it didn't need fixing. Note this scenario is always a bug and so I think it should be changed to *always* be an error, regardless of DEBUG_STRICT_USER_COPY_CHECKS. 2) "copy_from_user() buffer size is not provably correct" This is the (cryptic) false positive warning which happens when I enable __compiletime_object_size() for new compilers (and DEBUG_STRICT_USER_COPY_CHECKS). It happens when object size is const, but copy size is *not*. In this case there's no way to compare the two at build time, so it gives the warning. (Note the warning is a byproduct of the fact that gcc has no way of knowing whether the overflow function will be called, so the call isn't dead code and the warning attribute is activated.) So this warning seems to only indicate "this is an unusual pattern, maybe you should check it out" rather than "this is a bug". It seems to be working "as designed": it has nothing to do with gcc compiler phases AFAICT. (Which begs the question: why didn't these warnings appear with older versions of gcc? I have no idea...) I get 102(!) of these warnings with allyesconfig and the __compiletime_object_size() gcc check removed. I don't know if there are any real bugs hiding in there, but from looking at a small sample, I didn't see any. So warning 2 seems to be intentional for some reason. I suggested removing it, while keeping the corresponding runtime check. But according to Kees it sometimes finds real bugs. (Kees, can you confirm that at least some of the recent bugs you found were from warning 2?) Anyway I don't currently see any doable option other than just removing warning 2 (yet still keeping the corresponding copy_user_overflow() runtime check). -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-29 17:40 +0200 |
| Subject | Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc |
| Message-ID | <sbwPg-BA-39@gated-at.bofh.it> |
| In reply to | #1471896 |
On Mon, Aug 29, 2016 at 7:48 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> So I *think* your patch fixes the wrong problem. That's probably at
> least somewhat my fault because I misunderstood the issue before and may
> have described it wrong at some point.
>
> AFAICT, gcc isn't doing anything wrong, and the false positives are
> "intentional".
>
> There are in fact two static warnings (which are being silenced for new
> versions of gcc):
[ snip snip details ]
Ok.
Color me convinced, I never even looked at the two different cases, I
thought it was just one issue.
Let's just remove the spurious false positive warning then, in order
to re-instate the *actual* warning that right now is disabled entirely
due to the unrelated false positives.
Thanks for looking into this. Would you happen to also have a patch
that can be applied? Hint hint..
Linus
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-29 19:10 +0200 |
| Subject | [PATCH v2] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbyel-1BC-11@gated-at.bofh.it> |
| In reply to | #1471936 |
On Mon, Aug 29, 2016 at 08:36:46AM -0700, Linus Torvalds wrote:
> On Mon, Aug 29, 2016 at 7:48 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > So I *think* your patch fixes the wrong problem. That's probably at
> > least somewhat my fault because I misunderstood the issue before and may
> > have described it wrong at some point.
> >
> > AFAICT, gcc isn't doing anything wrong, and the false positives are
> > "intentional".
> >
> > There are in fact two static warnings (which are being silenced for new
> > versions of gcc):
>
> [ snip snip details ]
>
> Ok.
>
> Color me convinced, I never even looked at the two different cases, I
> thought it was just one issue.
>
> Let's just remove the spurious false positive warning then, in order
> to re-instate the *actual* warning that right now is disabled entirely
> due to the unrelated false positives.
>
> Thanks for looking into this. Would you happen to also have a patch
> that can be applied? Hint hint..
How about something like this? I can split it up if needed...
---
From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH v2] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
There are three usercopy warnings which are currently being silenced for
gcc 4.6 and newer:
1) "copy_from_user() buffer size is too small" compile warning/error
This is a static warning which happens when object size and copy size
are both const, and copy size > object size. I didn't see any false
positives for this one. So the function warning attribute seems to
be working fine here.
Note this scenario is always a bug and so I think it should be
changed to *always* be an error, regardless of
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS.
2) "copy_from_user() buffer size is not provably correct" compile warning
This is another static warning which happens when I enable
__compiletime_object_size() for new compilers (and
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS). It happens when object size
is const, but copy size is *not*. In this case there's no way to
compare the two at build time, so it gives the warning. (Note the
warning is a byproduct of the fact that gcc has no way of knowing
whether the overflow function will be called, so the call isn't dead
code and the warning attribute is activated.)
So this warning seems to only indicate "this is an unusual pattern,
maybe you should check it out" rather than "this is a bug".
I get 102(!) of these warnings with allyesconfig and the
__compiletime_object_size() gcc check removed. I don't know if there
are any real bugs hiding in there, but from looking at a small
sample, I didn't see any. According to Kees, it does sometimes find
real bugs. But the false positive rate seems high.
3) "Buffer overflow detected" runtime warning
This is a runtime warning where object size is const, and copy size >
object size.
All three warnings (both static and runtime) were completely disabled
for gcc 4.6 with the following commit:
2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")
That commit mistakenly assumed that the false positives were caused by a
gcc bug in __compiletime_object_size(). But in fact,
__compiletime_object_size() seems to be working fine. The false
positives were instead triggered by #2 above. (Though I don't have an
explanation for why the warnings supposedly only started showing up in
gcc 4.6.)
So remove warning #2 to get rid of all the false positives, and re-enable
warnings #1 and #3 by reverting the above commit.
Furthermore, since #1 is a real bug which is detected at compile time,
upgrade it to always be an error.
Having done all that, CONFIG_DEBUG_STRICT_USER_COPY_CHECKS is no longer
needed.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/parisc/Kconfig | 1 -
arch/parisc/configs/c8000_defconfig | 1 -
arch/parisc/configs/generic-64bit_defconfig | 1 -
arch/parisc/include/asm/uaccess.h | 22 ++++-----
arch/s390/Kconfig | 1 -
arch/s390/configs/default_defconfig | 1 -
arch/s390/configs/gcov_defconfig | 1 -
arch/s390/configs/performance_defconfig | 1 -
arch/s390/defconfig | 1 -
arch/s390/include/asm/uaccess.h | 19 +++++---
arch/tile/Kconfig | 1 -
arch/tile/include/asm/uaccess.h | 19 ++++----
arch/x86/Kconfig | 1 -
arch/x86/include/asm/uaccess.h | 69 ++++-------------------------
include/asm-generic/uaccess.h | 1 +
include/linux/compiler-gcc.h | 2 +-
lib/Kconfig.debug | 18 --------
lib/Makefile | 1 -
lib/usercopy.c | 9 ----
19 files changed, 45 insertions(+), 125 deletions(-)
delete mode 100644 lib/usercopy.c
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index cd87781..af12c2d 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -1,6 +1,5 @@
config PARISC
def_bool y
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_MIGHT_HAVE_PC_PARPORT
select HAVE_IDE
select HAVE_OPROFILE
diff --git a/arch/parisc/configs/c8000_defconfig b/arch/parisc/configs/c8000_defconfig
index 1a8f6f95..f6a4c01 100644
--- a/arch/parisc/configs/c8000_defconfig
+++ b/arch/parisc/configs/c8000_defconfig
@@ -245,7 +245,6 @@ CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_PROVE_RCU_DELAY=y
CONFIG_DEBUG_BLOCK_EXT_DEVT=y
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_KEYS=y
# CONFIG_CRYPTO_HW is not set
CONFIG_FONTS=y
diff --git a/arch/parisc/configs/generic-64bit_defconfig b/arch/parisc/configs/generic-64bit_defconfig
index 7e07926..c564e6e 100644
--- a/arch/parisc/configs/generic-64bit_defconfig
+++ b/arch/parisc/configs/generic-64bit_defconfig
@@ -291,7 +291,6 @@ CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC=y
CONFIG_BOOTPARAM_HUNG_TASK_PANIC=y
# CONFIG_SCHED_DEBUG is not set
CONFIG_TIMER_STATS=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_CRYPTO_MANAGER=y
CONFIG_CRYPTO_ECB=m
CONFIG_CRYPTO_PCBC=m
diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
index 0f59fd9..736c0c1 100644
--- a/arch/parisc/include/asm/uaccess.h
+++ b/arch/parisc/include/asm/uaccess.h
@@ -208,13 +208,13 @@ unsigned long copy_in_user(void __user *dst, const void __user *src, unsigned lo
#define __copy_to_user_inatomic __copy_to_user
#define __copy_from_user_inatomic __copy_from_user
-extern void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
- __compiletime_error("copy_from_user() buffer size is not provably correct")
-#else
- __compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
+
+static inline void copy_user_overflow(int size, unsigned long count)
+{
+ WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
static inline unsigned long __must_check copy_from_user(void *to,
const void __user *from,
@@ -223,10 +223,12 @@ static inline unsigned long __must_check copy_from_user(void *to,
int sz = __compiletime_object_size(to);
int ret = -EFAULT;
- if (likely(sz == -1 || !__builtin_constant_p(n) || sz >= n))
+ if (likely(sz == -1 || sz >= n))
ret = __copy_from_user(to, from, n);
- else
- copy_from_user_overflow();
+ else (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
+ else
+ __bad_copy_user();
return ret;
}
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index e751fe2..c109f07 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -68,7 +68,6 @@ config DEBUG_RODATA
config S390
def_bool y
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_GCOV_PROFILE_ALL
diff --git a/arch/s390/configs/default_defconfig b/arch/s390/configs/default_defconfig
index 26e0c7f..412b1bd 100644
--- a/arch/s390/configs/default_defconfig
+++ b/arch/s390/configs/default_defconfig
@@ -602,7 +602,6 @@ CONFIG_FAIL_FUTEX=y
CONFIG_FAULT_INJECTION_DEBUG_FS=y
CONFIG_FAULT_INJECTION_STACKTRACE_FILTER=y
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_IRQSOFF_TRACER=y
CONFIG_PREEMPT_TRACER=y
CONFIG_SCHED_TRACER=y
diff --git a/arch/s390/configs/gcov_defconfig b/arch/s390/configs/gcov_defconfig
index 24879da..bec279e 100644
--- a/arch/s390/configs/gcov_defconfig
+++ b/arch/s390/configs/gcov_defconfig
@@ -552,7 +552,6 @@ CONFIG_NOTIFIER_ERROR_INJECTION=m
CONFIG_CPU_NOTIFIER_ERROR_INJECT=m
CONFIG_PM_NOTIFIER_ERROR_INJECT=m
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_BLK_DEV_IO_TRACE=y
# CONFIG_KPROBE_EVENT is not set
CONFIG_TRACE_ENUM_MAP_FILE=y
diff --git a/arch/s390/configs/performance_defconfig b/arch/s390/configs/performance_defconfig
index a5c1e5f..1751446 100644
--- a/arch/s390/configs/performance_defconfig
+++ b/arch/s390/configs/performance_defconfig
@@ -549,7 +549,6 @@ CONFIG_TIMER_STATS=y
CONFIG_RCU_TORTURE_TEST=m
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_SCHED_TRACER=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_STACK_TRACER=y
diff --git a/arch/s390/defconfig b/arch/s390/defconfig
index 73610f2..2d40ef0 100644
--- a/arch/s390/defconfig
+++ b/arch/s390/defconfig
@@ -172,7 +172,6 @@ CONFIG_DEBUG_NOTIFIERS=y
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_RCU_TRACE=y
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_SCHED_TRACER=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_TRACER_SNAPSHOT_PER_CPU_SWAP=y
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index 9b49cf1..95aefdb 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -311,6 +311,14 @@ int __get_user_bad(void) __attribute__((noreturn));
#define __put_user_unaligned __put_user
#define __get_user_unaligned __get_user
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
+
+static inline void copy_user_overflow(int size, unsigned long count)
+{
+ WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
+
/**
* copy_to_user: - Copy a block of data into user space.
* @to: Destination address, in user space.
@@ -332,12 +340,6 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
return __copy_to_user(to, from, n);
}
-void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
-
/**
* copy_from_user: - Copy a block of data from user space.
* @to: Destination address, in kernel space.
@@ -362,7 +364,10 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
might_fault();
if (unlikely(sz != -1 && sz < n)) {
- copy_from_user_overflow();
+ if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
+ else
+ __bad_copy_user();
return n;
}
return __copy_from_user(to, from, n);
diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 4820a02..78da75b 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -4,7 +4,6 @@
config TILE
def_bool y
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAVE_NMI_SAFE_CMPXCHG
select ARCH_WANT_FRAME_POINTERS
diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
index 0a9c4265..c664300 100644
--- a/arch/tile/include/asm/uaccess.h
+++ b/arch/tile/include/asm/uaccess.h
@@ -416,14 +416,13 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
return n;
}
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-/*
- * There are still unprovable places in the generic code as of 2.6.34, so this
- * option is not really compatible with -Werror, which is more useful in
- * general.
- */
-extern void copy_from_user_overflow(void)
- __compiletime_warning("copy_from_user() size is not provably correct");
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
+
+static inline void copy_user_overflow(int size, unsigned long count)
+{
+ WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
static inline unsigned long __must_check copy_from_user(void *to,
const void __user *from,
@@ -433,8 +432,10 @@ static inline unsigned long __must_check copy_from_user(void *to,
if (likely(sz == -1 || sz >= n))
n = _copy_from_user(to, from, n);
+ else if (!__builtin_constant_p(n))
+ copy_user_overflow();
else
- copy_from_user_overflow();
+ __bad_copy_user();
return n;
}
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c580d8c..2a1f0ce 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -24,7 +24,6 @@ config X86
select ARCH_DISCARD_MEMBLOCK
select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FAST_MULTIPLIER
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a0ae610..c3f2911 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -697,43 +697,14 @@ unsigned long __must_check _copy_from_user(void *to, const void __user *from,
unsigned long __must_check _copy_to_user(void __user *to, const void *from,
unsigned n);
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-# define copy_user_diag __compiletime_error
-#else
-# define copy_user_diag __compiletime_warning
-#endif
-
-extern void copy_user_diag("copy_from_user() buffer size is too small")
-copy_from_user_overflow(void);
-extern void copy_user_diag("copy_to_user() buffer size is too small")
-copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-
-#undef copy_user_diag
-
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-
-extern void
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-__copy_from_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_from_user_overflow(size, count) __copy_from_user_overflow()
-
-extern void
-__compiletime_warning("copy_to_user() buffer size is not provably correct")
-__copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
-
-#else
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
-static inline void
-__copy_from_user_overflow(int size, unsigned long count)
+static inline void copy_user_overflow(int size, unsigned long count)
{
WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
}
-#define __copy_to_user_overflow __copy_from_user_overflow
-
-#endif
-
static inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long n)
{
@@ -743,31 +714,13 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
kasan_check_write(to, n);
- /*
- * While we would like to have the compiler do the checking for us
- * even in the non-constant size case, any false positives there are
- * a problem (especially when DEBUG_STRICT_USER_COPY_CHECKS, but even
- * without - the [hopefully] dangerous looking nature of the warning
- * would make people go look at the respecitive call sites over and
- * over again just to find that there's no problem).
- *
- * And there are cases where it's just not realistic for the compiler
- * to prove the count to be in range. For example when multiple call
- * sites of a helper function - perhaps in different source files -
- * all doing proper range checking, yet the helper function not doing
- * so again.
- *
- * Therefore limit the compile time checking to the constant size
- * case, and do only runtime checking for non-constant sizes.
- */
-
if (likely(sz < 0 || sz >= n)) {
check_object_size(to, n, false);
n = _copy_from_user(to, from, n);
- } else if (__builtin_constant_p(n))
- copy_from_user_overflow();
+ } else if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
else
- __copy_from_user_overflow(sz, n);
+ __bad_copy_user();
return n;
}
@@ -781,21 +734,17 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
might_fault();
- /* See the comment in copy_from_user() above. */
if (likely(sz < 0 || sz >= n)) {
check_object_size(from, n, true);
n = _copy_to_user(to, from, n);
- } else if (__builtin_constant_p(n))
- copy_to_user_overflow();
+ } else if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
else
- __copy_to_user_overflow(sz, n);
+ __bad_copy_user();
return n;
}
-#undef __copy_from_user_overflow
-#undef __copy_to_user_overflow
-
/*
* We rely on the nested NMI work to allow atomic faults from the NMI path; the
* nested NMI paths are careful to preserve CR2.
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index 1bfa602..5dea1fb 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -72,6 +72,7 @@ struct exception_table_entry
/* Returns 0 if exception not found and fixup otherwise. */
extern unsigned long search_exception_table(unsigned long);
+
/*
* architectures with an MMU should override these two
*/
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 8dbc892..573c5a1 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -158,7 +158,7 @@
#define __compiler_offsetof(a, b) \
__builtin_offsetof(a, b)
-#if GCC_VERSION >= 40100 && GCC_VERSION < 40600
+#if GCC_VERSION >= 40100
# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
#endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2307d7c..2e2cca5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1686,24 +1686,6 @@ config LATENCYTOP
Enable this option if you want to use the LatencyTOP tool
to find out which userspace is blocking on what kernel operations.
-config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
- bool
-
-config DEBUG_STRICT_USER_COPY_CHECKS
- bool "Strict user copy size checks"
- depends on ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
- depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING
- help
- Enabling this option turns a certain set of sanity checks for user
- copy operations into compile time failures.
-
- The copy_from_user() etc checks are there to help test if there
- are sufficient security checks on the length argument of
- the copy operation, by having gcc prove that the argument is
- within bounds.
-
- If unsure, say N.
-
source kernel/trace/Kconfig
menu "Runtime Testing"
diff --git a/lib/Makefile b/lib/Makefile
index cfa68eb..5dc77a8 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -24,7 +24,6 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
earlycpio.o seq_buf.o nmi_backtrace.o nodemask.o
-obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
lib-$(CONFIG_MMU) += ioremap.o
lib-$(CONFIG_SMP) += cpumask.o
lib-$(CONFIG_HAS_DMA) += dma-noop.o
diff --git a/lib/usercopy.c b/lib/usercopy.c
deleted file mode 100644
index 4f5b1dd..0000000
--- a/lib/usercopy.c
+++ /dev/null
@@ -1,9 +0,0 @@
-#include <linux/export.h>
-#include <linux/bug.h>
-#include <linux/uaccess.h>
-
-void copy_from_user_overflow(void)
-{
- WARN(1, "Buffer overflow detected!\n");
-}
-EXPORT_SYMBOL(copy_from_user_overflow);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-29 20:10 +0200 |
| Subject | Re: [PATCH v2] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbzap-2aw-15@gated-at.bofh.it> |
| In reply to | #1471993 |
On Mon, Aug 29, 2016 at 12:08:13PM -0500, Josh Poimboeuf wrote:
> On Mon, Aug 29, 2016 at 08:36:46AM -0700, Linus Torvalds wrote:
> > On Mon, Aug 29, 2016 at 7:48 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > So I *think* your patch fixes the wrong problem. That's probably at
> > > least somewhat my fault because I misunderstood the issue before and may
> > > have described it wrong at some point.
> > >
> > > AFAICT, gcc isn't doing anything wrong, and the false positives are
> > > "intentional".
> > >
> > > There are in fact two static warnings (which are being silenced for new
> > > versions of gcc):
> >
> > [ snip snip details ]
> >
> > Ok.
> >
> > Color me convinced, I never even looked at the two different cases, I
> > thought it was just one issue.
> >
> > Let's just remove the spurious false positive warning then, in order
> > to re-instate the *actual* warning that right now is disabled entirely
> > due to the unrelated false positives.
> >
> > Thanks for looking into this. Would you happen to also have a patch
> > that can be applied? Hint hint..
>
> How about something like this? I can split it up if needed...
>
> ---
>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH v2] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
...and that failed to build on tile and parisc. Here's the fix:
----
diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
index 736c0c1..e915048 100644
--- a/arch/parisc/include/asm/uaccess.h
+++ b/arch/parisc/include/asm/uaccess.h
@@ -225,7 +225,7 @@ static inline unsigned long __must_check copy_from_user(void *to,
if (likely(sz == -1 || sz >= n))
ret = __copy_from_user(to, from, n);
- else (!__builtin_constant_p(n))
+ else if (!__builtin_constant_p(n))
copy_user_overflow(sz, n);
else
__bad_copy_user();
diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
index c664300..4416f09 100644
--- a/arch/tile/include/asm/uaccess.h
+++ b/arch/tile/include/asm/uaccess.h
@@ -433,7 +433,7 @@ static inline unsigned long __must_check copy_from_user(void *to,
if (likely(sz == -1 || sz >= n))
n = _copy_from_user(to, from, n);
else if (!__builtin_constant_p(n))
- copy_user_overflow();
+ copy_user_overflow(sz, n);
else
__bad_copy_user();
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-30 15:10 +0200 |
| Subject | [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbQXD-5kV-23@gated-at.bofh.it> |
| In reply to | #1471993 |
There are three usercopy warnings which are currently being silenced for
gcc 4.6 and newer:
1) "copy_from_user() buffer size is too small" compile warning/error
This is a static warning which happens when object size and copy size
are both const, and copy size > object size. I didn't see any false
positives for this one. So the function warning attribute seems to
be working fine here.
Note this scenario is always a bug and so I think it should be
changed to *always* be an error, regardless of
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS.
2) "copy_from_user() buffer size is not provably correct" compile warning
This is another static warning which happens when I enable
__compiletime_object_size() for new compilers (and
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS). It happens when object size
is const, but copy size is *not*. In this case there's no way to
compare the two at build time, so it gives the warning. (Note the
warning is a byproduct of the fact that gcc has no way of knowing
whether the overflow function will be called, so the call isn't dead
code and the warning attribute is activated.)
So this warning seems to only indicate "this is an unusual pattern,
maybe you should check it out" rather than "this is a bug".
I get 102(!) of these warnings with allyesconfig and the
__compiletime_object_size() gcc check removed. I don't know if there
are any real bugs hiding in there, but from looking at a small
sample, I didn't see any. According to Kees, it does sometimes find
real bugs. But the false positive rate seems high.
3) "Buffer overflow detected" runtime warning
This is a runtime warning where object size is const, and copy size >
object size.
All three warnings (both static and runtime) were completely disabled
for gcc 4.6 with the following commit:
2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")
That commit mistakenly assumed that the false positives were caused by a
gcc bug in __compiletime_object_size(). But in fact,
__compiletime_object_size() seems to be working fine. The false
positives were instead triggered by #2 above. (Though I don't have an
explanation for why the warnings supposedly only started showing up in
gcc 4.6.)
So remove warning #2 to get rid of all the false positives, and re-enable
warnings #1 and #3 by reverting the above commit.
Furthermore, since #1 is a real bug which is detected at compile time,
upgrade it to always be an error.
Having done all that, CONFIG_DEBUG_STRICT_USER_COPY_CHECKS is no longer
needed.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
v3: fixed compile errors on parisc and tile
arch/parisc/Kconfig | 1 -
arch/parisc/configs/c8000_defconfig | 1 -
arch/parisc/configs/generic-64bit_defconfig | 1 -
arch/parisc/include/asm/uaccess.h | 22 ++++-----
arch/s390/Kconfig | 1 -
arch/s390/configs/default_defconfig | 1 -
arch/s390/configs/gcov_defconfig | 1 -
arch/s390/configs/performance_defconfig | 1 -
arch/s390/defconfig | 1 -
arch/s390/include/asm/uaccess.h | 19 +++++---
arch/tile/Kconfig | 1 -
arch/tile/include/asm/uaccess.h | 22 +++++----
arch/x86/Kconfig | 1 -
arch/x86/include/asm/uaccess.h | 69 ++++-------------------------
include/asm-generic/uaccess.h | 1 +
include/linux/compiler-gcc.h | 2 +-
lib/Kconfig.debug | 18 --------
lib/Makefile | 1 -
lib/usercopy.c | 9 ----
19 files changed, 45 insertions(+), 128 deletions(-)
delete mode 100644 lib/usercopy.c
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index cd87781..af12c2d 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -1,6 +1,5 @@
config PARISC
def_bool y
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_MIGHT_HAVE_PC_PARPORT
select HAVE_IDE
select HAVE_OPROFILE
diff --git a/arch/parisc/configs/c8000_defconfig b/arch/parisc/configs/c8000_defconfig
index 1a8f6f95..f6a4c01 100644
--- a/arch/parisc/configs/c8000_defconfig
+++ b/arch/parisc/configs/c8000_defconfig
@@ -245,7 +245,6 @@ CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_PROVE_RCU_DELAY=y
CONFIG_DEBUG_BLOCK_EXT_DEVT=y
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_KEYS=y
# CONFIG_CRYPTO_HW is not set
CONFIG_FONTS=y
diff --git a/arch/parisc/configs/generic-64bit_defconfig b/arch/parisc/configs/generic-64bit_defconfig
index 7e07926..c564e6e 100644
--- a/arch/parisc/configs/generic-64bit_defconfig
+++ b/arch/parisc/configs/generic-64bit_defconfig
@@ -291,7 +291,6 @@ CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC=y
CONFIG_BOOTPARAM_HUNG_TASK_PANIC=y
# CONFIG_SCHED_DEBUG is not set
CONFIG_TIMER_STATS=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_CRYPTO_MANAGER=y
CONFIG_CRYPTO_ECB=m
CONFIG_CRYPTO_PCBC=m
diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
index 0f59fd9..e915048 100644
--- a/arch/parisc/include/asm/uaccess.h
+++ b/arch/parisc/include/asm/uaccess.h
@@ -208,13 +208,13 @@ unsigned long copy_in_user(void __user *dst, const void __user *src, unsigned lo
#define __copy_to_user_inatomic __copy_to_user
#define __copy_from_user_inatomic __copy_from_user
-extern void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
- __compiletime_error("copy_from_user() buffer size is not provably correct")
-#else
- __compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
+
+static inline void copy_user_overflow(int size, unsigned long count)
+{
+ WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
static inline unsigned long __must_check copy_from_user(void *to,
const void __user *from,
@@ -223,10 +223,12 @@ static inline unsigned long __must_check copy_from_user(void *to,
int sz = __compiletime_object_size(to);
int ret = -EFAULT;
- if (likely(sz == -1 || !__builtin_constant_p(n) || sz >= n))
+ if (likely(sz == -1 || sz >= n))
ret = __copy_from_user(to, from, n);
- else
- copy_from_user_overflow();
+ else if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
+ else
+ __bad_copy_user();
return ret;
}
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index e751fe2..c109f07 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -68,7 +68,6 @@ config DEBUG_RODATA
config S390
def_bool y
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_GCOV_PROFILE_ALL
diff --git a/arch/s390/configs/default_defconfig b/arch/s390/configs/default_defconfig
index 26e0c7f..412b1bd 100644
--- a/arch/s390/configs/default_defconfig
+++ b/arch/s390/configs/default_defconfig
@@ -602,7 +602,6 @@ CONFIG_FAIL_FUTEX=y
CONFIG_FAULT_INJECTION_DEBUG_FS=y
CONFIG_FAULT_INJECTION_STACKTRACE_FILTER=y
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_IRQSOFF_TRACER=y
CONFIG_PREEMPT_TRACER=y
CONFIG_SCHED_TRACER=y
diff --git a/arch/s390/configs/gcov_defconfig b/arch/s390/configs/gcov_defconfig
index 24879da..bec279e 100644
--- a/arch/s390/configs/gcov_defconfig
+++ b/arch/s390/configs/gcov_defconfig
@@ -552,7 +552,6 @@ CONFIG_NOTIFIER_ERROR_INJECTION=m
CONFIG_CPU_NOTIFIER_ERROR_INJECT=m
CONFIG_PM_NOTIFIER_ERROR_INJECT=m
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_BLK_DEV_IO_TRACE=y
# CONFIG_KPROBE_EVENT is not set
CONFIG_TRACE_ENUM_MAP_FILE=y
diff --git a/arch/s390/configs/performance_defconfig b/arch/s390/configs/performance_defconfig
index a5c1e5f..1751446 100644
--- a/arch/s390/configs/performance_defconfig
+++ b/arch/s390/configs/performance_defconfig
@@ -549,7 +549,6 @@ CONFIG_TIMER_STATS=y
CONFIG_RCU_TORTURE_TEST=m
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_SCHED_TRACER=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_STACK_TRACER=y
diff --git a/arch/s390/defconfig b/arch/s390/defconfig
index 73610f2..2d40ef0 100644
--- a/arch/s390/defconfig
+++ b/arch/s390/defconfig
@@ -172,7 +172,6 @@ CONFIG_DEBUG_NOTIFIERS=y
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_RCU_TRACE=y
CONFIG_LATENCYTOP=y
-CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
CONFIG_SCHED_TRACER=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_TRACER_SNAPSHOT_PER_CPU_SWAP=y
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index 9b49cf1..95aefdb 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -311,6 +311,14 @@ int __get_user_bad(void) __attribute__((noreturn));
#define __put_user_unaligned __put_user
#define __get_user_unaligned __get_user
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
+
+static inline void copy_user_overflow(int size, unsigned long count)
+{
+ WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
+
/**
* copy_to_user: - Copy a block of data into user space.
* @to: Destination address, in user space.
@@ -332,12 +340,6 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
return __copy_to_user(to, from, n);
}
-void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
-
/**
* copy_from_user: - Copy a block of data from user space.
* @to: Destination address, in kernel space.
@@ -362,7 +364,10 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
might_fault();
if (unlikely(sz != -1 && sz < n)) {
- copy_from_user_overflow();
+ if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
+ else
+ __bad_copy_user();
return n;
}
return __copy_from_user(to, from, n);
diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 4820a02..78da75b 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -4,7 +4,6 @@
config TILE
def_bool y
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAVE_NMI_SAFE_CMPXCHG
select ARCH_WANT_FRAME_POINTERS
diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
index 0a9c4265..a77369e 100644
--- a/arch/tile/include/asm/uaccess.h
+++ b/arch/tile/include/asm/uaccess.h
@@ -416,14 +416,13 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
return n;
}
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-/*
- * There are still unprovable places in the generic code as of 2.6.34, so this
- * option is not really compatible with -Werror, which is more useful in
- * general.
- */
-extern void copy_from_user_overflow(void)
- __compiletime_warning("copy_from_user() size is not provably correct");
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
+
+static inline void copy_user_overflow(int size, unsigned long count)
+{
+ WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
+}
static inline unsigned long __must_check copy_from_user(void *to,
const void __user *from,
@@ -433,14 +432,13 @@ static inline unsigned long __must_check copy_from_user(void *to,
if (likely(sz == -1 || sz >= n))
n = _copy_from_user(to, from, n);
+ else if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
else
- copy_from_user_overflow();
+ __bad_copy_user();
return n;
}
-#else
-#define copy_from_user _copy_from_user
-#endif
#ifdef __tilegx__
/**
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c580d8c..2a1f0ce 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -24,7 +24,6 @@ config X86
select ARCH_DISCARD_MEMBLOCK
select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
- select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FAST_MULTIPLIER
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a0ae610..c3f2911 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -697,43 +697,14 @@ unsigned long __must_check _copy_from_user(void *to, const void __user *from,
unsigned long __must_check _copy_to_user(void __user *to, const void *from,
unsigned n);
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-# define copy_user_diag __compiletime_error
-#else
-# define copy_user_diag __compiletime_warning
-#endif
-
-extern void copy_user_diag("copy_from_user() buffer size is too small")
-copy_from_user_overflow(void);
-extern void copy_user_diag("copy_to_user() buffer size is too small")
-copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-
-#undef copy_user_diag
-
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-
-extern void
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-__copy_from_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_from_user_overflow(size, count) __copy_from_user_overflow()
-
-extern void
-__compiletime_warning("copy_to_user() buffer size is not provably correct")
-__copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
-
-#else
+extern void __compiletime_error("usercopy buffer size is too small")
+__bad_copy_user(void);
-static inline void
-__copy_from_user_overflow(int size, unsigned long count)
+static inline void copy_user_overflow(int size, unsigned long count)
{
WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
}
-#define __copy_to_user_overflow __copy_from_user_overflow
-
-#endif
-
static inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long n)
{
@@ -743,31 +714,13 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
kasan_check_write(to, n);
- /*
- * While we would like to have the compiler do the checking for us
- * even in the non-constant size case, any false positives there are
- * a problem (especially when DEBUG_STRICT_USER_COPY_CHECKS, but even
- * without - the [hopefully] dangerous looking nature of the warning
- * would make people go look at the respecitive call sites over and
- * over again just to find that there's no problem).
- *
- * And there are cases where it's just not realistic for the compiler
- * to prove the count to be in range. For example when multiple call
- * sites of a helper function - perhaps in different source files -
- * all doing proper range checking, yet the helper function not doing
- * so again.
- *
- * Therefore limit the compile time checking to the constant size
- * case, and do only runtime checking for non-constant sizes.
- */
-
if (likely(sz < 0 || sz >= n)) {
check_object_size(to, n, false);
n = _copy_from_user(to, from, n);
- } else if (__builtin_constant_p(n))
- copy_from_user_overflow();
+ } else if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
else
- __copy_from_user_overflow(sz, n);
+ __bad_copy_user();
return n;
}
@@ -781,21 +734,17 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
might_fault();
- /* See the comment in copy_from_user() above. */
if (likely(sz < 0 || sz >= n)) {
check_object_size(from, n, true);
n = _copy_to_user(to, from, n);
- } else if (__builtin_constant_p(n))
- copy_to_user_overflow();
+ } else if (!__builtin_constant_p(n))
+ copy_user_overflow(sz, n);
else
- __copy_to_user_overflow(sz, n);
+ __bad_copy_user();
return n;
}
-#undef __copy_from_user_overflow
-#undef __copy_to_user_overflow
-
/*
* We rely on the nested NMI work to allow atomic faults from the NMI path; the
* nested NMI paths are careful to preserve CR2.
diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index 1bfa602..5dea1fb 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -72,6 +72,7 @@ struct exception_table_entry
/* Returns 0 if exception not found and fixup otherwise. */
extern unsigned long search_exception_table(unsigned long);
+
/*
* architectures with an MMU should override these two
*/
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 8dbc892..573c5a1 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -158,7 +158,7 @@
#define __compiler_offsetof(a, b) \
__builtin_offsetof(a, b)
-#if GCC_VERSION >= 40100 && GCC_VERSION < 40600
+#if GCC_VERSION >= 40100
# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
#endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2307d7c..2e2cca5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1686,24 +1686,6 @@ config LATENCYTOP
Enable this option if you want to use the LatencyTOP tool
to find out which userspace is blocking on what kernel operations.
-config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
- bool
-
-config DEBUG_STRICT_USER_COPY_CHECKS
- bool "Strict user copy size checks"
- depends on ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
- depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING
- help
- Enabling this option turns a certain set of sanity checks for user
- copy operations into compile time failures.
-
- The copy_from_user() etc checks are there to help test if there
- are sufficient security checks on the length argument of
- the copy operation, by having gcc prove that the argument is
- within bounds.
-
- If unsure, say N.
-
source kernel/trace/Kconfig
menu "Runtime Testing"
diff --git a/lib/Makefile b/lib/Makefile
index cfa68eb..5dc77a8 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -24,7 +24,6 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
earlycpio.o seq_buf.o nmi_backtrace.o nodemask.o
-obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
lib-$(CONFIG_MMU) += ioremap.o
lib-$(CONFIG_SMP) += cpumask.o
lib-$(CONFIG_HAS_DMA) += dma-noop.o
diff --git a/lib/usercopy.c b/lib/usercopy.c
deleted file mode 100644
index 4f5b1dd..0000000
--- a/lib/usercopy.c
+++ /dev/null
@@ -1,9 +0,0 @@
-#include <linux/export.h>
-#include <linux/bug.h>
-#include <linux/uaccess.h>
-
-void copy_from_user_overflow(void)
-{
- WARN(1, "Buffer overflow detected!\n");
-}
-EXPORT_SYMBOL(copy_from_user_overflow);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-30 19:10 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbUHT-7Jg-1@gated-at.bofh.it> |
| In reply to | #1472477 |
On Tue, Aug 30, 2016 at 6:04 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> There are three usercopy warnings which are currently being silenced for
> gcc 4.6 and newer:
[.. snip snip ..]
Ok, I'm not entirely happy with the timing, but I think the problem
counts as a regression since it effectively made all the checks go
away in practice for most people, so I'm going to apply this patch.
I know Al Viro is working on some uaccess cleanups and trying to make
a lot of this be generic, so there's hopefully cleanups coming in the
not too distant future (I say "hopefully", because I worry that
looking at the mess will make Al dig his eyes out), but this seems to
be a clear improvement.
I still do wish we'd move the x86 __builtin_constant_p(n) check
around, so that x86 wouldn't do the run-time check_object_size() for
the trivially statically correct case, but I guess that's a separate
issue from this patch anyway.
If somebody has objections to this patch, holler quickly, because it's
about to get applied. 3.. 2.. 1..
Linus
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-30 20:20 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbVND-8n7-9@gated-at.bofh.it> |
| In reply to | #1472647 |
On Tue, Aug 30, 2016 at 1:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Aug 30, 2016 at 6:04 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> There are three usercopy warnings which are currently being silenced for
>> gcc 4.6 and newer:
>
> [.. snip snip ..]
>
> Ok, I'm not entirely happy with the timing, but I think the problem
> counts as a regression since it effectively made all the checks go
> away in practice for most people, so I'm going to apply this patch.
Yeah, for pragmatism, I'm a fan of this patch since it restores the
const checks. What gets lost here are the gcc dead-code optimization
situations where gcc can figure out the value range for a non-const
size, but that's currently broken anyway, so there's no point in
keeping it. We can add it back when gcc fixes their regression.
> I know Al Viro is working on some uaccess cleanups and trying to make
> a lot of this be generic, so there's hopefully cleanups coming in the
> not too distant future (I say "hopefully", because I worry that
> looking at the mess will make Al dig his eyes out), but this seems to
> be a clear improvement.
Yeah. Mark Rutland is also looking at this too.
> I still do wish we'd move the x86 __builtin_constant_p(n) check
> around, so that x86 wouldn't do the run-time check_object_size() for
> the trivially statically correct case, but I guess that's a separate
> issue from this patch anyway.
Yeah, I'm going to wait a bit for the dust to settle here, but it's
worth documenting the situation as I'd like to see it.
First, some current API usage which we'll need to maintain at least
for now: __copy_*_user() is just copy_*_user() without the access_ok()
checks. Unfortunately, some arch implement different copying methods
depending on if the entry is via copy...() or __copy..() (e.g. see
x86's use of _copy...() -- single underscore??) There doesn't seem to
be a good reason for this, and I think it would make sense to extract
the actual per-arch implementation that performs the real copy into
something like arm64's __arch_copy_*_user(), which only does the copy
itself and nothing else.
Once that's in place, we can do sanity-checking in __copy_*_user(),
leaving the access_ok() only in copy_*_user(). The logic should be
something like:
if const destination object size is known:
if copy size is too large:
if copy size is const:
abort build
else:
runtime BUG
else:
perform copy
else:
perform runtime object size sanity checks
perform copy
For example, totally untested, put together based on Josh's updates,
and the arm64 code, and some variable name clarity changes:
static inline __must_check unsigned long __copy_from_user(void *to,
const void __user *from, unsigned long n)
{
int dest_size = __compiletime_object_size(to);
might_fault();
/* KASan seems to want pre-check arguments, so run it first. */
kasan_check_write(to, n);
if (likely(dest_size != -1)) {
/* Destination object size is known at compile time. */
if (n > dest_size) {
/* Copy size is too large for destination object. */
if (__builtin_constant_p(n)) {
/* Copy size is known at compile time: abort the build. */
copy_user_compile_time_overflow(dest_size, n);
} else {
/* Copy size only known at runtime, abort copy with BUG. */
__bad_user_copy();
}
} else {
/* Copy size within size of destination object, perform copy. */
n = __arch_copy_from_user(to, from, n);
}
} else {
/* Destination object size needs runtime checking. */
check_runtime_object_size(to, from, n);
/* If we got here, runtime checks passed, perform copy. */
n = __arch_copy_from_user(to, from, n);
}
return n;
}
static inline __must_check unsigned long copy_from_user(void *to,
const void __user * from, unsigned long n)
{
if (access_ok(VERIFY_READ, from, n)) {
n = __copy_from_user(to, from, n);
} else
memset(to, 0, n); /* This is needed to avoid memory
content leaks. */
return n;
}
Some notes, here: the __bad_user_copy() should be a BUG, not a WARN
since we've landed on a provably bad situation.
check_object_size() should probably be renamed
"check_runtime_obj_size" or something to clarify its purpose, since
it's intended to be called only when we have to go off and examine
runtime object metadata to figure out how to correctly perform bounds
checking.
> If somebody has objections to this patch, holler quickly, because it's
> about to get applied. 3.. 2.. 1..
Go for it! :)
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-30 21:10 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbWA2-tT-31@gated-at.bofh.it> |
| In reply to | #1472745 |
On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote:
> static inline __must_check unsigned long __copy_from_user(void *to,
> const void __user *from, unsigned long n)
> {
> int dest_size = __compiletime_object_size(to);
>
> might_fault();
> /* KASan seems to want pre-check arguments, so run it first. */
> kasan_check_write(to, n);
>
> if (likely(dest_size != -1)) {
> /* Destination object size is known at compile time. */
> if (n > dest_size) {
> /* Copy size is too large for destination object. */
> if (__builtin_constant_p(n)) {
> /* Copy size is known at compile time: abort the build. */
> copy_user_compile_time_overflow(dest_size, n);
> } else {
> /* Copy size only known at runtime, abort copy with BUG. */
> __bad_user_copy();
> }
> } else {
> /* Copy size within size of destination object, perform copy. */
> n = __arch_copy_from_user(to, from, n);
> }
> } else {
> /* Destination object size needs runtime checking. */
> check_runtime_object_size(to, from, n);
> /* If we got here, runtime checks passed, perform copy. */
> n = __arch_copy_from_user(to, from, n);
> }
> return n;
> }
>
> static inline __must_check unsigned long copy_from_user(void *to,
> const void __user * from, unsigned long n)
> {
> if (access_ok(VERIFY_READ, from, n)) {
> n = __copy_from_user(to, from, n);
> } else
> memset(to, 0, n); /* This is needed to avoid memory
> content leaks. */
> return n;
> }
>
> Some notes, here: the __bad_user_copy() should be a BUG, not a WARN
> since we've landed on a provably bad situation.
Looks good to me. One nit: I think the "likely" check for "dest_size !=
-1" isn't needed. dest_size is known at compile-time, so gcc should be
able to optimize it accordingly.
> check_object_size() should probably be renamed
> "check_runtime_obj_size" or something to clarify its purpose, since
> it's intended to be called only when we have to go off and examine
> runtime object metadata to figure out how to correctly perform bounds
> checking.
Personally I find having "size" in the name to be misleading, since the
function actually looks at much more than just size. Especially
considering the fact that we already have the other static and runtime
checks which do only check the size.
I also don't really care for "runtime", since most functions are indeed
called at runtime. If anything I'd prefer the reverse, where any
built-in compile-time "functions" are specially named or annotated.
My vote would be something like check_usercopy_object().
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-30 21:30 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbWTn-Dd-9@gated-at.bofh.it> |
| In reply to | #1472793 |
On Tue, Aug 30, 2016 at 3:09 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote:
>> static inline __must_check unsigned long __copy_from_user(void *to,
>> const void __user *from, unsigned long n)
>> {
>> int dest_size = __compiletime_object_size(to);
>>
>> might_fault();
>> /* KASan seems to want pre-check arguments, so run it first. */
>> kasan_check_write(to, n);
>>
>> if (likely(dest_size != -1)) {
>> /* Destination object size is known at compile time. */
>> if (n > dest_size) {
>> /* Copy size is too large for destination object. */
>> if (__builtin_constant_p(n)) {
>> /* Copy size is known at compile time: abort the build. */
>> copy_user_compile_time_overflow(dest_size, n);
>> } else {
>> /* Copy size only known at runtime, abort copy with BUG. */
>> __bad_user_copy();
>> }
>> } else {
>> /* Copy size within size of destination object, perform copy. */
>> n = __arch_copy_from_user(to, from, n);
>> }
>> } else {
>> /* Destination object size needs runtime checking. */
>> check_runtime_object_size(to, from, n);
>> /* If we got here, runtime checks passed, perform copy. */
>> n = __arch_copy_from_user(to, from, n);
>> }
>> return n;
>> }
>>
>> static inline __must_check unsigned long copy_from_user(void *to,
>> const void __user * from, unsigned long n)
>> {
>> if (access_ok(VERIFY_READ, from, n)) {
>> n = __copy_from_user(to, from, n);
>> } else
>> memset(to, 0, n); /* This is needed to avoid memory
>> content leaks. */
>> return n;
>> }
>>
>> Some notes, here: the __bad_user_copy() should be a BUG, not a WARN
>> since we've landed on a provably bad situation.
>
> Looks good to me. One nit: I think the "likely" check for "dest_size !=
> -1" isn't needed. dest_size is known at compile-time, so gcc should be
> able to optimize it accordingly.
Yeah, good point.
>> check_object_size() should probably be renamed
>> "check_runtime_obj_size" or something to clarify its purpose, since
>> it's intended to be called only when we have to go off and examine
>> runtime object metadata to figure out how to correctly perform bounds
>> checking.
>
> Personally I find having "size" in the name to be misleading, since the
> function actually looks at much more than just size. Especially
> considering the fact that we already have the other static and runtime
> checks which do only check the size.
>
> I also don't really care for "runtime", since most functions are indeed
> called at runtime. If anything I'd prefer the reverse, where any
> built-in compile-time "functions" are specially named or annotated.
>
> My vote would be something like check_usercopy_object().
Sounds good to me. :)
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-08-30 22:20 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbXFL-1b9-1@gated-at.bofh.it> |
| In reply to | #1472745 |
On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote:
> First, some current API usage which we'll need to maintain at least
> for now: __copy_*_user() is just copy_*_user() without the access_ok()
> checks. Unfortunately, some arch implement different copying methods
> depending on if the entry is via copy...() or __copy..() (e.g. see
> x86's use of _copy...() -- single underscore??) There doesn't seem to
> be a good reason for this, and I think it would make sense to extract
> the actual per-arch implementation that performs the real copy into
> something like arm64's __arch_copy_*_user(), which only does the copy
> itself and nothing else.
No. __arch_copy_from_user() is a bloody bad idea; the real primitive
is what's currently called __copy_from_user_inatomic(), and I'm planning
to rename it to raw_copy_from_user(). Note that _this_ should not
zero anything on fault; "inatomic" part is a misnomer. I'm not sure
if __copy_from_user() will survive long-term, actually; copy_from_user()
should (size checks aside) be equivalent to
size_t res = size;
might_fault();
if (likely(access_ok(...)))
res = __copy_from_user_inatomic(...);
if (unlikely(res))
memset(to + (size - res), 0, res);
return res;
Linus asked to take that to lib/* - at least the memset() part.
* get_user()/put_user()/clear_user()/copy_{from,to,in}_user() should
check access_ok() (if non-degenerate on the architecture in question).
* failing get_user(x, p)/__get_user(x, p) should zero x
* short copy (for any reason, including access_ok() failure) in
copy_from_user() should return the amount of bytes *not* copied and zero them.
In no circumstances should it return -E...
* __copy_from_user_inatomic(to, from, size) should return exactly
size - amount of bytes stored. It does *not* need to copy as much as possible
in case of fault. It should not zero anything; as the matter of fact, zeroing
does not belong in assembler part at all.
* iov_iter_copy_from_user_atomic(), copy_page_from_iter()
and copy_page_from_iter() will not modify anything past the amount they
return. In particular, they will not zero anything at all. Right now it's
arch-dependent.
* iov_iter_fault_in_readable() will merge with
iov_iter_fault_in_multipages_readable(), with the semantics of the latter.
As the matter of fact, the same ought to happen to fault_in_pages_readable()
and fault_in_multipages_readable().
* ->write_end() instances on short copy into uptodate page should
not zero anything whatsoever; when page is not uptodate, they should only
zero an area if readpage should've done the same (e.g. if it's something like
ramfs, or if we know that we'd allocated new on-disk blocks and hadn't
copied them over, etc. Returning 0 and leaving a page !uptodate is always
OK on a short copy; we might do something more intelligent, but that's
up to specific ->write_end() instance.
* includes of asm/uaccess.h are going away. That's obviously not
something we can afford as a prereq for fixes to be backported, but for
the next window we definitely want a one-time tree-wide switch to
linux/uaccess.h. For *.c (and local .h) it's trivial, for general-purpose
headers it'll take some massage. Once we have linux/uaccess.h use, we
can move duplicates over there.
The above obviously doesn't go into exception/longjmp/asm-goto/etc.
pile of joy; that needs more experiments and frankly, I want to finish
separating the -stable fodder first.
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-31 00:30 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <sbZHF-2pw-65@gated-at.bofh.it> |
| In reply to | #1472832 |
On Tue, Aug 30, 2016 at 4:13 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote:
>
>> First, some current API usage which we'll need to maintain at least
>> for now: __copy_*_user() is just copy_*_user() without the access_ok()
>> checks. Unfortunately, some arch implement different copying methods
>> depending on if the entry is via copy...() or __copy..() (e.g. see
>> x86's use of _copy...() -- single underscore??) There doesn't seem to
>> be a good reason for this, and I think it would make sense to extract
>> the actual per-arch implementation that performs the real copy into
>> something like arm64's __arch_copy_*_user(), which only does the copy
>> itself and nothing else.
>
> No. __arch_copy_from_user() is a bloody bad idea; the real primitive
> is what's currently called __copy_from_user_inatomic(), and I'm planning
> to rename it to raw_copy_from_user(). Note that _this_ should not
I don't think the name is important, just as long as it's clear. We
both seem to agree: the arch-specific stuff should be separate from
the common API that has the sanity checking, etc, which it sounds like
you're already doing.
-Kees
> zero anything on fault; "inatomic" part is a misnomer. I'm not sure
> if __copy_from_user() will survive long-term, actually; copy_from_user()
> should (size checks aside) be equivalent to
> size_t res = size;
> might_fault();
> if (likely(access_ok(...)))
> res = __copy_from_user_inatomic(...);
> if (unlikely(res))
> memset(to + (size - res), 0, res);
> return res;
>
> Linus asked to take that to lib/* - at least the memset() part.
>
> * get_user()/put_user()/clear_user()/copy_{from,to,in}_user() should
> check access_ok() (if non-degenerate on the architecture in question).
> * failing get_user(x, p)/__get_user(x, p) should zero x
> * short copy (for any reason, including access_ok() failure) in
> copy_from_user() should return the amount of bytes *not* copied and zero them.
> In no circumstances should it return -E...
> * __copy_from_user_inatomic(to, from, size) should return exactly
> size - amount of bytes stored. It does *not* need to copy as much as possible
> in case of fault. It should not zero anything; as the matter of fact, zeroing
> does not belong in assembler part at all.
> * iov_iter_copy_from_user_atomic(), copy_page_from_iter()
> and copy_page_from_iter() will not modify anything past the amount they
> return. In particular, they will not zero anything at all. Right now it's
> arch-dependent.
> * iov_iter_fault_in_readable() will merge with
> iov_iter_fault_in_multipages_readable(), with the semantics of the latter.
> As the matter of fact, the same ought to happen to fault_in_pages_readable()
> and fault_in_multipages_readable().
> * ->write_end() instances on short copy into uptodate page should
> not zero anything whatsoever; when page is not uptodate, they should only
> zero an area if readpage should've done the same (e.g. if it's something like
> ramfs, or if we know that we'd allocated new on-disk blocks and hadn't
> copied them over, etc. Returning 0 and leaving a page !uptodate is always
> OK on a short copy; we might do something more intelligent, but that's
> up to specific ->write_end() instance.
Agreed on all these; and getting that documented in the final
uaccess.h seems like a very good idea too.
> * includes of asm/uaccess.h are going away. That's obviously not
> something we can afford as a prereq for fixes to be backported, but for
> the next window we definitely want a one-time tree-wide switch to
> linux/uaccess.h. For *.c (and local .h) it's trivial, for general-purpose
> headers it'll take some massage. Once we have linux/uaccess.h use, we
> can move duplicates over there.
How do you envision architectures gluing their copy implementation to
raw_copy_from_user()?
> The above obviously doesn't go into exception/longjmp/asm-goto/etc.
> pile of joy; that needs more experiments and frankly, I want to finish
> separating the -stable fodder first.
Yup, cool.
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-08-31 11:50 +0200 |
| Subject | Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS |
| Message-ID | <scajE-HB-27@gated-at.bofh.it> |
| In reply to | #1472832 |
On Tue, Aug 30, 2016 at 09:13:32PM +0100, Al Viro wrote: > On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote: > > > First, some current API usage which we'll need to maintain at least > > for now: __copy_*_user() is just copy_*_user() without the access_ok() > > checks. Unfortunately, some arch implement different copying methods > > depending on if the entry is via copy...() or __copy..() (e.g. see > > x86's use of _copy...() -- single underscore??) There doesn't seem to > > be a good reason for this, and I think it would make sense to extract > > the actual per-arch implementation that performs the real copy into > > something like arm64's __arch_copy_*_user(), which only does the copy > > itself and nothing else. > > No. __arch_copy_from_user() is a bloody bad idea; the real primitive > is what's currently called __copy_from_user_inatomic(), and I'm planning > to rename it to raw_copy_from_user(). Great! FWIW, my plan with the arch_* forms was to follow the convention set by the spinlock code and have raw_* forms build atop of these, where common debug and/or hardening checks would live. From my PoV, anything to make this more consistent cross-architecture is good, especially if we can pull the duplicated logic into common code. Thanks, Mark.
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web