[PATCH] uaccess: Add missing __must_check attributes

[PATCH] uaccess: Add missing __must_check attributes

[Kees Cook]

The usercopy implementation comments describe that callers of the
copy_*_user() family of functions must always have their return values
checked. This can be enforced at compile time with __must_check, so add
it where needed.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/thread_info.h |  2 +-
 include/linux/uaccess.h     | 21 +++++++++++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 8d8821b3689a..659a4400517b 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -134,7 +134,7 @@ static inline void copy_overflow(int size, unsigned long count)
 	WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
 }
 
-static __always_inline bool
+static __always_inline __must_check bool
 check_copy_size(const void *addr, size_t bytes, bool is_source)
 {
 	int sz = __compiletime_object_size(addr);
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 34a038563d97..70bbdc38dc37 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -55,7 +55,7 @@
  * as usual) and both source and destination can trigger faults.
  */
 
-static __always_inline unsigned long
+static __always_inline __must_check unsigned long
 __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
 {
 	kasan_check_write(to, n);
@@ -63,7 +63,7 @@ __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
 	return raw_copy_from_user(to, from, n);
 }
 
-static __always_inline unsigned long
+static __always_inline __must_check unsigned long
 __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	might_fault();
@@ -85,7 +85,7 @@ __copy_from_user(void *to, const void __user *from, unsigned long n)
  * The caller should also make sure he pins the user space address
  * so that we don't result in page fault and sleep.
  */
-static __always_inline unsigned long
+static __always_inline __must_check unsigned long
 __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n)
 {
 	kasan_check_read(from, n);
@@ -93,7 +93,7 @@ __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n)
 	return raw_copy_to_user(to, from, n);
 }
 
-static __always_inline unsigned long
+static __always_inline __must_check unsigned long
 __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	might_fault();
@@ -103,7 +103,7 @@ __copy_to_user(void __user *to, const void *from, unsigned long n)
 }
 
 #ifdef INLINE_COPY_FROM_USER
-static inline unsigned long
+static inline __must_check unsigned long
 _copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	unsigned long res = n;
@@ -117,12 +117,12 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
 	return res;
 }
 #else
-extern unsigned long
+extern __must_check unsigned long
 _copy_from_user(void *, const void __user *, unsigned long);
 #endif
 
 #ifdef INLINE_COPY_TO_USER
-static inline unsigned long
+static inline __must_check unsigned long
 _copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	might_fault();
@@ -133,7 +133,7 @@ _copy_to_user(void __user *to, const void *from, unsigned long n)
 	return n;
 }
 #else
-extern unsigned long
+extern __must_check unsigned long
 _copy_to_user(void __user *, const void *, unsigned long);
 #endif
 
@@ -222,8 +222,9 @@ static inline bool pagefault_disabled(void)
 
 #ifndef ARCH_HAS_NOCACHE_UACCESS
 
-static inline unsigned long __copy_from_user_inatomic_nocache(void *to,
-				const void __user *from, unsigned long n)
+static inline __must_check unsigned long
+__copy_from_user_inatomic_nocache(void *to, const void __user *from,
+				  unsigned long n)
 {
 	return __copy_from_user_inatomic(to, from, n);
 }
-- 
2.17.1


-- 
Kees Cook

Re: [PATCH] uaccess: Add missing __must_check attributes

[Arnd Bergmann]

On Wed, Aug 28, 2019 at 7:38 PM Kees Cook <keescook@chromium.org> wrote:
>
> The usercopy implementation comments describe that callers of the
> copy_*_user() family of functions must always have their return values
> checked. This can be enforced at compile time with __must_check, so add
> it where needed.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

I can't find any other reports, so I'd point out here that this found what
looks like a bug in the x86 math-emu code:

arch/x86/math-emu/reg_ld_str.c:88:2: error: ignoring return value of
function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
        __copy_from_user(sti_ptr, s, 10);
        ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~
arch/x86/math-emu/reg_ld_str.c:1129:2: error: ignoring return value of
function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
        __copy_from_user(register_base + offset, s, other);
        ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/math-emu/reg_ld_str.c:1131:3: error: ignoring return value of
function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
                __copy_from_user(register_base, s + other, offset);
                ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Moreover, the same code also ignores the return value from most
get_user()/put_user()/FPU_get_user()/FPU_put_user() calls,
which have no warn_unused_result annotation (they are macros,
but I think something could be done if we want to have that
annotation to catch some of the other such users).

      Arnd

Re: [PATCH] uaccess: Add missing __must_check attributes

[Kees Cook]

On Mon, Sep 30, 2019 at 12:33:19PM +0200, Arnd Bergmann wrote:
> On Wed, Aug 28, 2019 at 7:38 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > The usercopy implementation comments describe that callers of the
> > copy_*_user() family of functions must always have their return values
> > checked. This can be enforced at compile time with __must_check, so add
> > it where needed.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> I can't find any other reports, so I'd point out here that this found what
> looks like a bug in the x86 math-emu code:

Oh interesting!

> arch/x86/math-emu/reg_ld_str.c:88:2: error: ignoring return value of
> function declared with 'warn_unused_result' attribute
> [-Werror,-Wunused-result]
>         __copy_from_user(sti_ptr, s, 10);
>         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~
> arch/x86/math-emu/reg_ld_str.c:1129:2: error: ignoring return value of
> function declared with 'warn_unused_result' attribute
> [-Werror,-Wunused-result]
>         __copy_from_user(register_base + offset, s, other);
>         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/math-emu/reg_ld_str.c:1131:3: error: ignoring return value of
> function declared with 'warn_unused_result' attribute
> [-Werror,-Wunused-result]
>                 __copy_from_user(register_base, s + other, offset);
>                 ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

What was the CONFIG for this? I didn't hit these in my build tests.

> Moreover, the same code also ignores the return value from most
> get_user()/put_user()/FPU_get_user()/FPU_put_user() calls,
> which have no warn_unused_result annotation (they are macros,
> but I think something could be done if we want to have that
> annotation to catch some of the other such users).

It would certainly make sense to mark those as __must_check too... now
tracking this here for anyone that wants to take a stab at it:
https://github.com/KSPP/linux/issues/16

-- 
Kees Cook

Re: [PATCH] uaccess: Add missing __must_check attributes

[Arnd Bergmann]

On Tue, Oct 1, 2019 at 1:17 AM Kees Cook <keescook@chromium.org> wrote:
> On Mon, Sep 30, 2019 at 12:33:19PM +0200, Arnd Bergmann wrote:
> > On Wed, Aug 28, 2019 at 7:38 PM Kees Cook <keescook@chromium.org> wrote:
> > arch/x86/math-emu/reg_ld_str.c:88:2: error: ignoring return value of
> > function declared with 'warn_unused_result' attribute
> > [-Werror,-Wunused-result]
> >         __copy_from_user(sti_ptr, s, 10);
> >         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~
> > arch/x86/math-emu/reg_ld_str.c:1129:2: error: ignoring return value of
> > function declared with 'warn_unused_result' attribute
> > [-Werror,-Wunused-result]
> >         __copy_from_user(register_base + offset, s, other);
> >         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > arch/x86/math-emu/reg_ld_str.c:1131:3: error: ignoring return value of
> > function declared with 'warn_unused_result' attribute
> > [-Werror,-Wunused-result]
> >                 __copy_from_user(register_base, s + other, offset);
> >                 ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> What was the CONFIG for this? I didn't hit these in my build tests.

I saw three randconfig builds trigger it in one day, so not that common.
https://pastebin.com/mUhbNEVR is one of them.

> > Moreover, the same code also ignores the return value from most
> > get_user()/put_user()/FPU_get_user()/FPU_put_user() calls,
> > which have no warn_unused_result annotation (they are macros,
> > but I think something could be done if we want to have that
> > annotation to catch some of the other such users).
>
> It would certainly make sense to mark those as __must_check too... now
> tracking this here for anyone that wants to take a stab at it:
> https://github.com/KSPP/linux/issues/16

FWIW, I have not come up with a way to add the warning, but I
did send a fix for the instances in the math-emu code:
https://lore.kernel.org/lkml/20191001142344.1274185-1-arnd@arndb.de/

      Arnd