| Message ID | 20181209204449.18906-1-tycho@tycho.ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] copy_{to,from}_user(): only inline when !__CHECKER__ | expand |
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
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)))
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(-)