From: Kees Cook <keescook@chromium.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Bill Metzenthen <billm@melbpc.org.au>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] x86: math-emu: check __copy_from_user result
Date: Tue, 1 Oct 2019 16:39:41 -0700 [thread overview]
Message-ID: <201910011638.F2F70EF@keescook> (raw)
In-Reply-To: <20191001142344.1274185-1-arnd@arndb.de>
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
next prev parent reply other threads:[~2019-10-01 23:39 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-01 14:23 [PATCH 1/2] x86: math-emu: check __copy_from_user result Arnd Bergmann
2019-10-01 14:23 ` [PATCH 2/2] x86: math-emu: limit MATH_EMULATION to 486SX compatibles Arnd Bergmann
2019-10-01 21:54 ` Kees Cook
2019-10-03 9:33 ` [tip: x86/cpu] x86/math-emu: Limit " tip-bot2 for Arnd Bergmann
2019-10-01 23:39 ` Kees Cook [this message]
2019-10-02 7:11 ` [PATCH 1/2] x86: math-emu: check __copy_from_user result Arnd Bergmann
2019-10-03 6:26 ` Kees Cook
2019-10-03 9:33 ` [tip: x86/cpu] x86/math-emu: Check __copy_from_user() result tip-bot2 for Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201910011638.F2F70EF@keescook \
--to=keescook@chromium.org \
--cc=arnd@arndb.de \
--cc=billm@melbpc.org.au \
--cc=bp@alien8.de \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).