While working on some additional copy_to_user() checks for sparse, I
noticed that sparse's current copy_to_user() checks are not triggered. This
is because copy_to_user() is declared as __always_inline, and sparse
specifically looks for a call instruction to copy_to_user() when it tries
to apply the checks.
A quick fix is to explicitly not inline when __CHECKER__ is defined, so
that sparse will be able to analyze all the copy_{to,from}_user calls.
There may be some refactoring in sparse that we can do to fix this,
although it's not immediately obvious to me how, hence the RFC-ness of this
patch.
Signed-off-by: Tycho Andersen <[email protected]>
---
include/linux/uaccess.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index efe79c1cdd47..f20a2d173e1f 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -140,7 +140,13 @@ extern unsigned long
_copy_to_user(void __user *, const void *, unsigned long);
#endif
-static __always_inline unsigned long __must_check
+#ifdef __CHECKER__
+#define uaccess_inline __attribute__((__noinline__))
+#else
+#define uaccess_inline __always_inline
+#endif
+
+static uaccess_inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long n)
{
if (likely(check_copy_size(to, n, false)))
@@ -148,7 +154,7 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
return n;
}
-static __always_inline unsigned long __must_check
+static uaccess_inline unsigned long __must_check
copy_to_user(void __user *to, const void *from, unsigned long n)
{
if (likely(check_copy_size(from, n, true)))
--
2.19.1
On Sun, Dec 09, 2018 at 01:44:49PM -0700, Tycho Andersen wrote:
> While working on some additional copy_to_user() checks for sparse, I
> noticed that sparse's current copy_to_user() checks are not triggered. This
> is because copy_to_user() is declared as __always_inline, and sparse
> specifically looks for a call instruction to copy_to_user() when it tries
> to apply the checks.
>
> A quick fix is to explicitly not inline when __CHECKER__ is defined, so
> that sparse will be able to analyze all the copy_{to,from}_user calls.
> There may be some refactoring in sparse that we can do to fix this,
> although it's not immediately obvious to me how, hence the RFC-ness of this
> patch.
Which sparse checks do not trigger? Explain, please - as it is, I had been
unable to guess what could "specifically looks for a call instruction" refer
to.
Hi Al,
On Sun, Dec 09, 2018 at 09:02:21PM +0000, Al Viro wrote:
> On Sun, Dec 09, 2018 at 01:44:49PM -0700, Tycho Andersen wrote:
> > While working on some additional copy_to_user() checks for sparse, I
> > noticed that sparse's current copy_to_user() checks are not triggered. This
> > is because copy_to_user() is declared as __always_inline, and sparse
> > specifically looks for a call instruction to copy_to_user() when it tries
> > to apply the checks.
> >
> > A quick fix is to explicitly not inline when __CHECKER__ is defined, so
> > that sparse will be able to analyze all the copy_{to,from}_user calls.
> > There may be some refactoring in sparse that we can do to fix this,
> > although it's not immediately obvious to me how, hence the RFC-ness of this
> > patch.
>
> Which sparse checks do not trigger? Explain, please - as it is, I had been
> unable to guess what could "specifically looks for a call instruction" refer
> to.
In sparse.c there's check_call_instruction(), which is triggered when
there's an instruction of OP_CALL type in the basic block. This simply
compares against the name of the call target to determine whether or
not to call check_ctu().
I think what's happening here is that the call is getting inlined, and
so the OP_CALL goes away, and check_call_instruction() never gets
called.
Tycho
On Sun, Dec 09, 2018 at 02:25:23PM -0700, Tycho Andersen wrote:
> Hi Al,
>
> On Sun, Dec 09, 2018 at 09:02:21PM +0000, Al Viro wrote:
> > On Sun, Dec 09, 2018 at 01:44:49PM -0700, Tycho Andersen wrote:
> > > While working on some additional copy_to_user() checks for sparse, I
> > > noticed that sparse's current copy_to_user() checks are not triggered. This
> > > is because copy_to_user() is declared as __always_inline, and sparse
> > > specifically looks for a call instruction to copy_to_user() when it tries
> > > to apply the checks.
> > >
> > > A quick fix is to explicitly not inline when __CHECKER__ is defined, so
> > > that sparse will be able to analyze all the copy_{to,from}_user calls.
> > > There may be some refactoring in sparse that we can do to fix this,
> > > although it's not immediately obvious to me how, hence the RFC-ness of this
> > > patch.
> >
> > Which sparse checks do not trigger? Explain, please - as it is, I had been
> > unable to guess what could "specifically looks for a call instruction" refer
> > to.
>
> In sparse.c there's check_call_instruction(), which is triggered when
> there's an instruction of OP_CALL type in the basic block. This simply
> compares against the name of the call target to determine whether or
> not to call check_ctu().
>
> I think what's happening here is that the call is getting inlined, and
> so the OP_CALL goes away, and check_call_instruction() never gets
> called.
Yes, it's what's happening.
There are several more or less bad/good solutions, like:
* add raw_copy_{to,from}_user() in the list of checked function
(not inlined in most archs).
* add a new annotation to force sparse to check the byte count
(I'm thinking about __range__/OP_RANGE or something similar).
* do these checks before functions are inlined (but then some
constant count could not yet be seen as constant).
* ...
Wasn't there some plan to remove all these __always_inline
because of the future 'asm inline'?
-- Luc
On Sun, Dec 09, 2018 at 02:25:23PM -0700, Tycho Andersen wrote:
> > Which sparse checks do not trigger? Explain, please - as it is, I had been
> > unable to guess what could "specifically looks for a call instruction" refer
> > to.
>
> In sparse.c there's check_call_instruction(), which is triggered when
> there's an instruction of OP_CALL type in the basic block. This simply
> compares against the name of the call target to determine whether or
> not to call check_ctu().
Oh, that Linus' experiment with "look for huge constant size argument
to memcpy() et.al."? Frankly, it's not only the wrong place to put the
checks, but breaking inlining loses the _real_ "known constant size"
checks in there.
I don't know if the check_ctu thing has ever caught a bug... What kind of
checks do you want to add? Because this place is almost certainly wrong
for anything useful...
If anything, I would suggest simulating this behaviour with something like
if (__builtin_constant_p(size) && size > something)
/* something that would trigger a warning */
_inside_ copy_from_user()/copy_to_user() and to hell with name-recognizing
magic...
On Sun, Dec 09, 2018 at 10:39:52PM +0100, Luc Van Oostenryck wrote:
> On Sun, Dec 09, 2018 at 02:25:23PM -0700, Tycho Andersen wrote:
> > Hi Al,
> >
> > On Sun, Dec 09, 2018 at 09:02:21PM +0000, Al Viro wrote:
> > > On Sun, Dec 09, 2018 at 01:44:49PM -0700, Tycho Andersen wrote:
> > > > While working on some additional copy_to_user() checks for sparse, I
> > > > noticed that sparse's current copy_to_user() checks are not triggered. This
> > > > is because copy_to_user() is declared as __always_inline, and sparse
> > > > specifically looks for a call instruction to copy_to_user() when it tries
> > > > to apply the checks.
> > > >
> > > > A quick fix is to explicitly not inline when __CHECKER__ is defined, so
> > > > that sparse will be able to analyze all the copy_{to,from}_user calls.
> > > > There may be some refactoring in sparse that we can do to fix this,
> > > > although it's not immediately obvious to me how, hence the RFC-ness of this
> > > > patch.
> > >
> > > Which sparse checks do not trigger? Explain, please - as it is, I had been
> > > unable to guess what could "specifically looks for a call instruction" refer
> > > to.
> >
> > In sparse.c there's check_call_instruction(), which is triggered when
> > there's an instruction of OP_CALL type in the basic block. This simply
> > compares against the name of the call target to determine whether or
> > not to call check_ctu().
> >
> > I think what's happening here is that the call is getting inlined, and
> > so the OP_CALL goes away, and check_call_instruction() never gets
> > called.
>
> Yes, it's what's happening.
>
> There are several more or less bad/good solutions, like:
> * add raw_copy_{to,from}_user() in the list of checked function
> (not inlined in most archs).
But they are inlined on x86 :\
> * add a new annotation to force sparse to check the byte count
> (I'm thinking about __range__/OP_RANGE or something similar).
Yes, I was playing around with inventing some check like this without
the need for an annotation. It's not clear to me if it's going to work
or not yet, though :). Top two patches here are what I was playing
with:
https://github.com/tych0/sparse/commits/check-as-infoleaks
> * do these checks before functions are inlined (but then some
> constant count could not yet be seen as constant).
Yeah, I guess I was wondering if there was some more clever location
in sparse itself we could move these to.
Tycho
On Sun, Dec 09, 2018 at 10:39:52PM +0100, Luc Van Oostenryck wrote:
> There are several more or less bad/good solutions, like:
> * add raw_copy_{to,from}_user() in the list of checked function
> (not inlined in most archs).
> * add a new annotation to force sparse to check the byte count
> (I'm thinking about __range__/OP_RANGE or something similar).
> * do these checks before functions are inlined (but then some
> constant count could not yet be seen as constant).
* just spell it out in copy_to_user() itself - as in
#ifdef C_T_U_SIZE_LIMIT
if (__builtin_constant_p(count) && count > C_T_U_SIZE_LIMIT)
/* something warning-triggering */
#endif
in the beginning of copy_from_user(). Or simply
#ifdef C_T_U_SIZE_LIMIT
BUILD_BUG_ON(__builtin_constant_p(count) && count > C_T_U_SIZE_LIMIT);
#endif
in there...
On Sun, Dec 09, 2018 at 09:46:00PM +0000, Al Viro wrote:
> On Sun, Dec 09, 2018 at 02:25:23PM -0700, Tycho Andersen wrote:
>
> > > Which sparse checks do not trigger? Explain, please - as it is, I had been
> > > unable to guess what could "specifically looks for a call instruction" refer
> > > to.
> >
> > In sparse.c there's check_call_instruction(), which is triggered when
> > there's an instruction of OP_CALL type in the basic block. This simply
> > compares against the name of the call target to determine whether or
> > not to call check_ctu().
>
> Oh, that Linus' experiment with "look for huge constant size argument
> to memcpy() et.al."? Frankly, it's not only the wrong place to put the
> checks, but breaking inlining loses the _real_ "known constant size"
> checks in there.
>
> I don't know if the check_ctu thing has ever caught a bug... What kind of
> checks do you want to add? Because this place is almost certainly wrong
> for anything useful...
Yeah, agreed that the static size check doesn't seem particularly
useful. I linked to these in the other mail, but the top two patches
here are what I was playing with:
https://github.com/tych0/sparse/commits/check-as-infoleaks
> If anything, I would suggest simulating this behaviour with something like
> if (__builtin_constant_p(size) && size > something)
> /* something that would trigger a warning */
> _inside_ copy_from_user()/copy_to_user() and to hell with name-recognizing
> magic...
Hmm. I wonder if we couldn't do some size checking with the argument
like this instead. Thanks for the idea, I'll play around with it.
Tycho
On Sun, Dec 09, 2018 at 09:56:51PM +0000, Al Viro wrote:
> On Sun, Dec 09, 2018 at 10:39:52PM +0100, Luc Van Oostenryck wrote:
>
> > There are several more or less bad/good solutions, like:
> > * add raw_copy_{to,from}_user() in the list of checked function
> > (not inlined in most archs).
> > * add a new annotation to force sparse to check the byte count
> > (I'm thinking about __range__/OP_RANGE or something similar).
> > * do these checks before functions are inlined (but then some
> > constant count could not yet be seen as constant).
> * just spell it out in copy_to_user() itself - as in
> #ifdef C_T_U_SIZE_LIMIT
> if (__builtin_constant_p(count) && count > C_T_U_SIZE_LIMIT)
> /* something warning-triggering */
> #endif
> in the beginning of copy_from_user(). Or simply
> #ifdef C_T_U_SIZE_LIMIT
> BUILD_BUG_ON(__builtin_constant_p(count) && count > C_T_U_SIZE_LIMIT);
> #endif
> in there...
Yes, I agree.
-- Luc