diff mbox series

[1/2] x86: math-emu: check __copy_from_user result

Message ID 20191001142344.1274185-1-arnd@arndb.de
State Accepted
Commit e6b44ce1925a8329a937c57f0d60ba0d9bb5d226
Headers show
Series [1/2] x86: math-emu: check __copy_from_user result | expand

Commit Message

Arnd Bergmann Oct. 1, 2019, 2:23 p.m. UTC 
The new __must_check annotation on __copy_from_user successfully
identified some code that has lacked the check since at least
linux-2.1.73:

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);
                ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In addition, the get_user/put_user helpers do not enforce a return value
check, but actually still require one. These have been missing
for even longer.

Change the internal wrappers around get_user/put_user to force
a signal and add a corresponding wrapper around __copy_from_user
to check all such cases.

Fixes: 257e458057e5 ("Import 2.1.73")
Fixes: 9dd819a15162 ("uaccess: add missing __must_check attributes")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 arch/x86/math-emu/fpu_system.h | 6 ++++--
 arch/x86/math-emu/reg_ld_str.c | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.20.0

Comments

Kees Cook Oct. 1, 2019, 11:39 p.m. UTC | #1 
On Tue, Oct 01, 2019 at 04:23:34PM +0200, Arnd Bergmann wrote:
> The new __must_check annotation on __copy_from_user successfully

> identified some code that has lacked the check since at least

> linux-2.1.73:

> 

> 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);

>                 ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> 

> In addition, the get_user/put_user helpers do not enforce a return value

> check, but actually still require one. These have been missing

> for even longer.

> 

> Change the internal wrappers around get_user/put_user to force

> a signal and add a corresponding wrapper around __copy_from_user

> to check all such cases.

> 

> Fixes: 257e458057e5 ("Import 2.1.73")

> Fixes: 9dd819a15162 ("uaccess: add missing __must_check attributes")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Reviewed-by: Kees Cook <keescook@chromium.org>


Notes below...

> ---

>  arch/x86/math-emu/fpu_system.h | 6 ++++--

>  arch/x86/math-emu/reg_ld_str.c | 6 +++---

>  2 files changed, 7 insertions(+), 5 deletions(-)

> 

> diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h

> index f98a0c956764..9b41391867dc 100644

> --- a/arch/x86/math-emu/fpu_system.h

> +++ b/arch/x86/math-emu/fpu_system.h

> @@ -107,6 +107,8 @@ static inline bool seg_writable(struct desc_struct *d)

>  #define FPU_access_ok(y,z)	if ( !access_ok(y,z) ) \

>  				math_abort(FPU_info,SIGSEGV)

>  #define FPU_abort		math_abort(FPU_info, SIGSEGV)

> +#define FPU_copy_from_user(to, from, n)	\

> +		do { if (copy_from_user(to, from, n)) FPU_abort; } while (0)

>  

>  #undef FPU_IGNORE_CODE_SEGV

>  #ifdef FPU_IGNORE_CODE_SEGV

> @@ -122,7 +124,7 @@ static inline bool seg_writable(struct desc_struct *d)

>  #define	FPU_code_access_ok(z) FPU_access_ok((void __user *)FPU_EIP,z)

>  #endif

>  

> -#define FPU_get_user(x,y)       get_user((x),(y))

> -#define FPU_put_user(x,y)       put_user((x),(y))

> +#define FPU_get_user(x,y) do { if (get_user((x),(y))) FPU_abort; } while (0)

> +#define FPU_put_user(x,y) do { if (put_user((x),(y))) FPU_abort; } while (0)

>  

>  #endif

> diff --git a/arch/x86/math-emu/reg_ld_str.c b/arch/x86/math-emu/reg_ld_str.c

> index f3779743d15e..fe6246ff9887 100644

> --- a/arch/x86/math-emu/reg_ld_str.c

> +++ b/arch/x86/math-emu/reg_ld_str.c

> @@ -85,7 +85,7 @@ int FPU_load_extended(long double __user *s, int stnr)

>  

>  	RE_ENTRANT_CHECK_OFF;

>  	FPU_access_ok(s, 10);

> -	__copy_from_user(sti_ptr, s, 10);

> +	FPU_copy_from_user(sti_ptr, s, 10);


These access_ok() checks seem redundant everywhere in this file (after
your switch from __copy* to copy*. I mean, I guess, just leave them, but
*shrug*

-Kees

>  	RE_ENTRANT_CHECK_ON;

>  

>  	return FPU_tagof(sti_ptr);

> @@ -1126,9 +1126,9 @@ void frstor(fpu_addr_modes addr_modes, u_char __user *data_address)

>  	/* Copy all registers in stack order. */

>  	RE_ENTRANT_CHECK_OFF;

>  	FPU_access_ok(s, 80);

> -	__copy_from_user(register_base + offset, s, other);

> +	FPU_copy_from_user(register_base + offset, s, other);

>  	if (offset)

> -		__copy_from_user(register_base, s + other, offset);

> +		FPU_copy_from_user(register_base, s + other, offset);

>  	RE_ENTRANT_CHECK_ON;

>  

>  	for (i = 0; i < 8; i++) {

> -- 

> 2.20.0

> 


-- 
Kees Cook
Arnd Bergmann Oct. 2, 2019, 7:11 a.m. UTC | #2 
On Wed, Oct 2, 2019 at 1:39 AM Kees Cook <keescook@chromium.org> wrote:

> > diff --git a/arch/x86/math-emu/reg_ld_str.c b/arch/x86/math-emu/reg_ld_str.c

> > index f3779743d15e..fe6246ff9887 100644

> > --- a/arch/x86/math-emu/reg_ld_str.c

> > +++ b/arch/x86/math-emu/reg_ld_str.c

> > @@ -85,7 +85,7 @@ int FPU_load_extended(long double __user *s, int stnr)

> >

> >       RE_ENTRANT_CHECK_OFF;

> >       FPU_access_ok(s, 10);

> > -     __copy_from_user(sti_ptr, s, 10);

> > +     FPU_copy_from_user(sti_ptr, s, 10);

>

> These access_ok() checks seem redundant everywhere in this file (after

> your switch from __copy* to copy*. I mean, I guess, just leave them, but

> *shrug*


There have always been duplicate/inconsistent for the get_user/put_user
case. I considered cleaning it all up but then decided to touch it as little
as possible.

       Arnd
Kees Cook Oct. 3, 2019, 6:26 a.m. UTC | #3 
On Wed, Oct 02, 2019 at 09:11:23AM +0200, Arnd Bergmann wrote:
> On Wed, Oct 2, 2019 at 1:39 AM Kees Cook <keescook@chromium.org> wrote:

> 

> > > diff --git a/arch/x86/math-emu/reg_ld_str.c b/arch/x86/math-emu/reg_ld_str.c

> > > index f3779743d15e..fe6246ff9887 100644

> > > --- a/arch/x86/math-emu/reg_ld_str.c

> > > +++ b/arch/x86/math-emu/reg_ld_str.c

> > > @@ -85,7 +85,7 @@ int FPU_load_extended(long double __user *s, int stnr)

> > >

> > >       RE_ENTRANT_CHECK_OFF;

> > >       FPU_access_ok(s, 10);

> > > -     __copy_from_user(sti_ptr, s, 10);

> > > +     FPU_copy_from_user(sti_ptr, s, 10);

> >

> > These access_ok() checks seem redundant everywhere in this file (after

> > your switch from __copy* to copy*. I mean, I guess, just leave them, but

> > *shrug*

> 

> There have always been duplicate/inconsistent for the get_user/put_user

> case. I considered cleaning it all up but then decided to touch it as little

> as possible.


Yeah, at this point, I'd agree. :)

-- 
Kees Cook
diff mbox series

Patch

diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
index f98a0c956764..9b41391867dc 100644
--- a/arch/x86/math-emu/fpu_system.h
+++ b/arch/x86/math-emu/fpu_system.h
@@ -107,6 +107,8 @@  static inline bool seg_writable(struct desc_struct *d)
 #define FPU_access_ok(y,z)	if ( !access_ok(y,z) ) \
 				math_abort(FPU_info,SIGSEGV)
 #define FPU_abort		math_abort(FPU_info, SIGSEGV)
+#define FPU_copy_from_user(to, from, n)	\
+		do { if (copy_from_user(to, from, n)) FPU_abort; } while (0)
 
 #undef FPU_IGNORE_CODE_SEGV
 #ifdef FPU_IGNORE_CODE_SEGV
@@ -122,7 +124,7 @@  static inline bool seg_writable(struct desc_struct *d)
 #define	FPU_code_access_ok(z) FPU_access_ok((void __user *)FPU_EIP,z)
 #endif
 
-#define FPU_get_user(x,y)       get_user((x),(y))
-#define FPU_put_user(x,y)       put_user((x),(y))
+#define FPU_get_user(x,y) do { if (get_user((x),(y))) FPU_abort; } while (0)
+#define FPU_put_user(x,y) do { if (put_user((x),(y))) FPU_abort; } while (0)
 
 #endif
diff --git a/arch/x86/math-emu/reg_ld_str.c b/arch/x86/math-emu/reg_ld_str.c
index f3779743d15e..fe6246ff9887 100644
--- a/arch/x86/math-emu/reg_ld_str.c
+++ b/arch/x86/math-emu/reg_ld_str.c
@@ -85,7 +85,7 @@  int FPU_load_extended(long double __user *s, int stnr)
 
 	RE_ENTRANT_CHECK_OFF;
 	FPU_access_ok(s, 10);
-	__copy_from_user(sti_ptr, s, 10);
+	FPU_copy_from_user(sti_ptr, s, 10);
 	RE_ENTRANT_CHECK_ON;
 
 	return FPU_tagof(sti_ptr);
@@ -1126,9 +1126,9 @@  void frstor(fpu_addr_modes addr_modes, u_char __user *data_address)
 	/* Copy all registers in stack order. */
 	RE_ENTRANT_CHECK_OFF;
 	FPU_access_ok(s, 80);
-	__copy_from_user(register_base + offset, s, other);
+	FPU_copy_from_user(register_base + offset, s, other);
 	if (offset)
-		__copy_from_user(register_base, s + other, offset);
+		FPU_copy_from_user(register_base, s + other, offset);
 	RE_ENTRANT_CHECK_ON;
 
 	for (i = 0; i < 8; i++) {