| Message ID | 20220216201449.2087956-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usercopy: Check valid lifetime via stack depth | expand |
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on hnaz-mm/master]
[also build test ERROR on kees/for-next/pstore v5.17-rc4 next-20220216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/usercopy-Check-valid-lifetime-via-stack-depth/20220217-041611
base: https://github.com/hnaz/linux-mm master
config: openrisc-randconfig-r002-20220216 (https://download.01.org/0day-ci/archive/20220217/202202170844.jnpFFEmh-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/77944e5fa0cf5a29903b72466a22152c6a5d41ac
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Kees-Cook/usercopy-Check-valid-lifetime-via-stack-depth/20220217-041611
git checkout 77944e5fa0cf5a29903b72466a22152c6a5d41ac
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=openrisc SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
mm/usercopy.c: In function 'check_stack_object':
>> mm/usercopy.c:41:42: error: 'current_stack_pointer' undeclared (first use in this function); did you mean 'user_stack_pointer'?
41 | const void * const low = (void *)current_stack_pointer;
| ^~~~~~~~~~~~~~~~~~~~~
| user_stack_pointer
mm/usercopy.c:41:42: note: each undeclared identifier is reported only once for each function it appears in
mm/usercopy.c: In function '__check_object_size':
mm/usercopy.c:287:41: error: 'current_stack_pointer' undeclared (first use in this function); did you mean 'user_stack_pointer'?
287 | (void *)current_stack_pointer - ptr,
| ^~~~~~~~~~~~~~~~~~~~~
| user_stack_pointer
vim +41 mm/usercopy.c
24
25 /*
26 * Checks if a given pointer and length is contained by the current
27 * stack frame (if possible).
28 *
29 * Returns:
30 * NOT_STACK: not at all on the stack
31 * GOOD_FRAME: fully within a valid stack frame
32 * GOOD_STACK: within the current stack (when can't frame-check exactly)
33 * BAD_STACK: error condition (invalid stack position or bad stack frame)
34 */
35 static noinline int check_stack_object(const void *obj, unsigned long len)
36 {
37 const void * const stack = task_stack_page(current);
38 const void * const stackend = stack + THREAD_SIZE;
39 #ifndef CONFIG_STACK_GROWSUP
40 const void * const high = stackend;
> 41 const void * const low = (void *)current_stack_pointer;
42 #else
43 const void * const high = (void *)current_stack_pointer;
44 const void * const low = stack;
45 #endif
46 int ret;
47
48 /* Object is not on the stack at all. */
49 if (obj + len <= stack || stackend <= obj)
50 return NOT_STACK;
51
52 /*
53 * Reject: object partially overlaps the stack (passing the
54 * check above means at least one end is within the stack,
55 * so if this check fails, the other end is outside the stack).
56 */
57 if (obj < stack || stackend < obj + len)
58 return BAD_STACK;
59
60 /* Check if object is safely within a valid frame. */
61 ret = arch_within_stack_frames(stack, stackend, obj, len);
62 if (ret)
63 return ret;
64
65 /*
66 * Reject: object not within current stack depth.
67 */
68 if (obj < low || high < obj + len)
69 return BAD_STACK;
70
71 return GOOD_STACK;
72 }
73
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/mm/usercopy.c b/mm/usercopy.c index d0d268135d96..3846c1634dca 100644 --- a/mm/usercopy.c +++ b/mm/usercopy.c @@ -29,13 +29,20 @@ * Returns: * NOT_STACK: not at all on the stack * GOOD_FRAME: fully within a valid stack frame - * GOOD_STACK: fully on the stack (when can't do frame-checking) + * GOOD_STACK: within the current stack (when can't frame-check exactly) * BAD_STACK: error condition (invalid stack position or bad stack frame) */ static noinline int check_stack_object(const void *obj, unsigned long len) { const void * const stack = task_stack_page(current); const void * const stackend = stack + THREAD_SIZE; +#ifndef CONFIG_STACK_GROWSUP + const void * const high = stackend; + const void * const low = (void *)current_stack_pointer; +#else + const void * const high = (void *)current_stack_pointer; + const void * const low = stack; +#endif int ret; /* Object is not on the stack at all. */ @@ -55,6 +62,12 @@ static noinline int check_stack_object(const void *obj, unsigned long len) if (ret) return ret; + /* + * Reject: object not within current stack depth. + */ + if (obj < low || high < obj + len) + return BAD_STACK; + return GOOD_STACK; } @@ -280,7 +293,13 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user) */ return; default: - usercopy_abort("process stack", NULL, to_user, 0, n); + usercopy_abort("process stack", NULL, to_user, +#ifndef CONFIG_STACK_GROWSUP + (void *)current_stack_pointer - ptr, +#else + ptr - (void *)current_stack_pointer, +#endif + n); } /* Check for bad heap object. */
Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking is not available (i.e. everything except x86 with FRAME_POINTER), check a stack object as being at least "current depth valid", in the sense that any object within the stack region but not between start-of-stack and current_stack_pointer should be considered unavailable (i.e. its lifetime is from a call no longer present on the stack). Additionally report usercopy bounds checking failures with an offset from current_stack_pointer, which may assist with diagnosing failures. The LKDTM USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests (once slightly adjusted in a separate patch) will pass again with this fixed. Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> Cc: Matthew Wilcox (Oracle) <willy@infradead.org> Cc: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org Signed-off-by: Kees Cook <keescook@chromium.org> --- mm/usercopy.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)