[RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Tycho Andersen]

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 <tycho@tycho.ws>
---
 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

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Al Viro]

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.

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Tycho Andersen]

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

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Luc Van Oostenryck]

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

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Tycho Andersen]

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

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Al Viro]

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...

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Luc Van Oostenryck]

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

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Al Viro]

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...

Re: [RFC v1] copy_{to,from}_user(): only inline when !__CHECKER__

[Tycho Andersen]

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