From: Michael Ellerman <mpe@ellerman•id.au>
To: Kees Cook <keescook@chromium•org>, linux-kernel@vger•kernel.org
Cc: Kees Cook <keescook@chromium•org>,
Rasmus Villemoes <linux@rasmusvillemoes•dk>,
Greg Kroah-Hartman <gregkh@linuxfoundation•org>,
linux-staging@lists•linux.dev, linux-wireless@vger•kernel.org,
"Gustavo A. R. Silva" <gustavoars@kernel•org>,
linux-hardening@vger•kernel.org, linux-block@vger•kernel.org,
clang-built-linux@googlegroups•com, netdev@vger•kernel.org,
Paul Mackerras <paulus@samba•org>,
dri-devel@lists•freedesktop.org,
Sudeep Holla <sudeep.holla@arm•com>,
Andrew Morton <akpm@linux-foundation•org>,
linuxppc-dev@lists•ozlabs.org, linux-kbuild@vger•kernel.org,
kernel test robot <lkp@intel•com>
Subject: Re: [PATCH v2 57/63] powerpc/signal32: Use struct_group() to zero spe regs
Date: Fri, 20 Aug 2021 17:49:35 +1000 [thread overview]
Message-ID: <877dggeesw.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20210818060533.3569517-58-keescook@chromium.org>
Kees Cook <keescook@chromium•org> writes:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
>
> Add a struct_group() for the spe registers so that memset() can correctly reason
> about the size:
>
> In function 'fortify_memset_chk',
> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>>> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> 195 | __write_overflow_field();
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Cc: Michael Ellerman <mpe@ellerman•id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel•crashing.org>
> Cc: Paul Mackerras <paulus@samba•org>
> Cc: Christophe Leroy <christophe.leroy@csgroup•eu>
> Cc: Sudeep Holla <sudeep.holla@arm•com>
> Cc: linuxppc-dev@lists•ozlabs.org
> Reported-by: kernel test robot <lkp@intel•com>
> Signed-off-by: Kees Cook <keescook@chromium•org>
> ---
> arch/powerpc/include/asm/processor.h | 6 ++++--
> arch/powerpc/kernel/signal_32.c | 6 +++---
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index f348e564f7dd..05dc567cb9a8 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -191,8 +191,10 @@ struct thread_struct {
> int used_vsr; /* set if process has used VSX */
> #endif /* CONFIG_VSX */
> #ifdef CONFIG_SPE
> - unsigned long evr[32]; /* upper 32-bits of SPE regs */
> - u64 acc; /* Accumulator */
> + struct_group(spe,
> + unsigned long evr[32]; /* upper 32-bits of SPE regs */
> + u64 acc; /* Accumulator */
> + );
> unsigned long spefscr; /* SPE & eFP status */
> unsigned long spefscr_last; /* SPEFSCR value on last prctl
> call or trap return */
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 0608581967f0..77b86caf5c51 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -532,11 +532,11 @@ static long restore_user_regs(struct pt_regs *regs,
> regs_set_return_msr(regs, regs->msr & ~MSR_SPE);
> if (msr & MSR_SPE) {
> /* restore spe registers from the stack */
> - unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs,
> - ELF_NEVRREG * sizeof(u32), failed);
> + unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs,
> + sizeof(current->thread.spe), failed);
This makes me nervous, because the ABI is that we copy ELF_NEVRREG *
sizeof(u32) bytes, not whatever sizeof(current->thread.spe) happens to
be.
ie. if we use sizeof an inadvertent change to the fields in
thread_struct could change how many bytes we copy out to userspace,
which would be an ABI break.
And that's not that hard to do, because it's not at all obvious that the
size and layout of fields in thread_struct affects the user ABI.
At the same time we don't want to copy the right number of bytes but
the wrong content, so from that point of view using sizeof is good :)
The way we handle it in ptrace is to have BUILD_BUG_ON()s to verify that
things match up, so maybe we should do that here too.
ie. add:
BUILD_BUG_ON(sizeof(current->thread.spe) == ELF_NEVRREG * sizeof(u32));
Not sure if you are happy doing that as part of this patch. I can always
do it later if not.
cheers
next prev parent reply other threads:[~2021-08-20 7:50 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210818060533.3569517-1-keescook@chromium.org>
2021-08-18 6:05 ` [PATCH v2 36/63] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp Kees Cook
2021-08-18 6:05 ` [PATCH v2 57/63] powerpc/signal32: Use struct_group() to zero spe regs Kees Cook
2021-08-20 7:49 ` Michael Ellerman [this message]
2021-08-20 7:53 ` Christophe Leroy
2021-08-20 12:13 ` Michael Ellerman
2021-08-20 15:55 ` Kees Cook
2021-08-23 4:55 ` Michael Ellerman
2021-08-18 6:05 ` [PATCH v2 61/63] powerpc: Split memset() to avoid multi-field overflow Kees Cook
2021-08-18 6:42 ` Christophe Leroy
2021-08-18 22:30 ` Kees Cook
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=877dggeesw.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=akpm@linux-foundation$(echo .)org \
--cc=clang-built-linux@googlegroups$(echo .)com \
--cc=dri-devel@lists$(echo .)freedesktop.org \
--cc=gregkh@linuxfoundation$(echo .)org \
--cc=gustavoars@kernel$(echo .)org \
--cc=keescook@chromium$(echo .)org \
--cc=linux-block@vger$(echo .)kernel.org \
--cc=linux-hardening@vger$(echo .)kernel.org \
--cc=linux-kbuild@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-staging@lists$(echo .)linux.dev \
--cc=linux-wireless@vger$(echo .)kernel.org \
--cc=linux@rasmusvillemoes$(echo .)dk \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=lkp@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=paulus@samba$(echo .)org \
--cc=sudeep.holla@arm$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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