| Message ID | 1534876926-21849-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/kernel: Fix potential spectre v1 in syscall | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
+Nathan as this is RTAS related. Le 21/08/2018 à 20:42, Breno Leitao a écrit : > The rtas syscall reads a value from a user-provided structure and uses it > to index an array, being a possible area for a potential spectre v1 attack. > This is the code that exposes this problem. > > args.rets = &args.args[nargs]; > > The nargs is an user provided value, and the below code is an example where > the 'nargs' value would be set to XX. > > struct rtas_args ra; > ra.nargs = htobe32(XX); > syscall(__NR_rtas, &ra); This patch has been hanging around in patchwork since 2018 and doesn't apply anymore. Is it still relevant ? If so, can you rebase et resubmit ? Thanks Christophe > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > arch/powerpc/kernel/rtas.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 8afd146bc9c7..5ef3c863003d 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -27,6 +27,7 @@ > #include <linux/slab.h> > #include <linux/reboot.h> > #include <linux/syscalls.h> > +#include <linux/nospec.h> > > #include <asm/prom.h> > #include <asm/rtas.h> > @@ -1056,7 +1057,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > struct rtas_args args; > unsigned long flags; > char *buff_copy, *errbuf = NULL; > - int nargs, nret, token; > + int index, nargs, nret, token; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -1084,7 +1085,8 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > if (token == RTAS_UNKNOWN_SERVICE) > return -EINVAL; > > - args.rets = &args.args[nargs]; > + index = array_index_nospec(nargs, ARRAY_SIZE(args.args)); > + args.rets = &args.args[index]; > memset(args.rets, 0, nret * sizeof(rtas_arg_t)); > > /* Need to handle ibm,suspend_me call specially */
On Tue, Mar 12, 2024 at 08:17:42AM +0000, Christophe Leroy wrote: > +Nathan as this is RTAS related. > > Le 21/08/2018 à 20:42, Breno Leitao a écrit : > > The rtas syscall reads a value from a user-provided structure and uses it > > to index an array, being a possible area for a potential spectre v1 attack. > > This is the code that exposes this problem. > > > > args.rets = &args.args[nargs]; > > > > The nargs is an user provided value, and the below code is an example where > > the 'nargs' value would be set to XX. > > > > struct rtas_args ra; > > ra.nargs = htobe32(XX); > > syscall(__NR_rtas, &ra); > > > This patch has been hanging around in patchwork since 2018 and doesn't > apply anymore. Is it still relevant ? If so, can you rebase et resubmit ? This seems to be important, since nargs is a user-provided value. I can submit it if the maintainers are willing to accept. I do not want to spend my time if no one is willing to review it. Thanks for revamping this one.
Breno Leitao <leitao@debian.org> writes: > On Tue, Mar 12, 2024 at 08:17:42AM +0000, Christophe Leroy wrote: >> +Nathan as this is RTAS related. >> >> Le 21/08/2018 à 20:42, Breno Leitao a écrit : >> > The rtas syscall reads a value from a user-provided structure and uses it >> > to index an array, being a possible area for a potential spectre v1 attack. >> > This is the code that exposes this problem. >> > >> > args.rets = &args.args[nargs]; >> > >> > The nargs is an user provided value, and the below code is an example where >> > the 'nargs' value would be set to XX. >> > >> > struct rtas_args ra; >> > ra.nargs = htobe32(XX); >> > syscall(__NR_rtas, &ra); >> >> >> This patch has been hanging around in patchwork since 2018 and doesn't >> apply anymore. Is it still relevant ? If so, can you rebase et resubmit ? > > This seems to be important, since nargs is a user-provided value. I can > submit it if the maintainers are willing to accept. I do not want to > spend my time if no one is willing to review it. My memory is that I didn't think it was actually a problem, because all we do is memset args.rets to zero. I thought I'd talked to you on Slack about it, but maybe I didn't. Anyway we should probably just fix it to be safe and keep the static checkers happy. I'll rebase it and apply it, I'm sure you've got better things to do :) cheers
On Tue, Mar 12, 2024 at 10:07:54PM +1100, Michael Ellerman wrote: > Breno Leitao <leitao@debian.org> writes: > > On Tue, Mar 12, 2024 at 08:17:42AM +0000, Christophe Leroy wrote: > >> +Nathan as this is RTAS related. > >> > >> Le 21/08/2018 à 20:42, Breno Leitao a écrit : > >> > The rtas syscall reads a value from a user-provided structure and uses it > >> > to index an array, being a possible area for a potential spectre v1 attack. > >> > This is the code that exposes this problem. > >> > > >> > args.rets = &args.args[nargs]; > >> > > >> > The nargs is an user provided value, and the below code is an example where > >> > the 'nargs' value would be set to XX. > >> > > >> > struct rtas_args ra; > >> > ra.nargs = htobe32(XX); > >> > syscall(__NR_rtas, &ra); > >> > >> > >> This patch has been hanging around in patchwork since 2018 and doesn't > >> apply anymore. Is it still relevant ? If so, can you rebase et resubmit ? > > > > This seems to be important, since nargs is a user-provided value. I can > > submit it if the maintainers are willing to accept. I do not want to > > spend my time if no one is willing to review it. > > My memory is that I didn't think it was actually a problem, because all > we do is memset args.rets to zero. I thought I'd talked to you on Slack > about it, but maybe I didn't. > > Anyway we should probably just fix it to be safe and keep the static > checkers happy. > > I'll rebase it and apply it, I'm sure you've got better things to do :) Awesome. Thanks Michael!
Michael Ellerman <mpe@ellerman.id.au> writes: > Breno Leitao <leitao@debian.org> writes: >> On Tue, Mar 12, 2024 at 08:17:42AM +0000, Christophe Leroy wrote: >>> +Nathan as this is RTAS related. Thanks! >>> Le 21/08/2018 à 20:42, Breno Leitao a écrit : >>> > The rtas syscall reads a value from a user-provided structure and uses it >>> > to index an array, being a possible area for a potential spectre v1 attack. >>> > This is the code that exposes this problem. >>> > >>> > args.rets = &args.args[nargs]; >>> > >>> > The nargs is an user provided value, and the below code is an example where >>> > the 'nargs' value would be set to XX. >>> > >>> > struct rtas_args ra; >>> > ra.nargs = htobe32(XX); >>> > syscall(__NR_rtas, &ra); >>> >>> >>> This patch has been hanging around in patchwork since 2018 and doesn't >>> apply anymore. Is it still relevant ? If so, can you rebase et resubmit ? >> >> This seems to be important, since nargs is a user-provided value. I can >> submit it if the maintainers are willing to accept. I do not want to >> spend my time if no one is willing to review it. > > My memory is that I didn't think it was actually a problem, because all > we do is memset args.rets to zero. This is also my initial reaction to this. I suppose if the memset() implementation performs some validation of the destination buffer contents (comparing to a known poison value or something) that could load the CPU cache then there is a more plausible issue? > Anyway we should probably just fix it to be safe and keep the static > checkers happy. Here is the relevant passage in its current state: if (copy_from_user(&args, uargs, 3 * sizeof(u32)) != 0) return -EFAULT; nargs = be32_to_cpu(args.nargs); nret = be32_to_cpu(args.nret); token = be32_to_cpu(args.token); if (nargs >= ARRAY_SIZE(args.args) || nret > ARRAY_SIZE(args.args) || nargs + nret > ARRAY_SIZE(args.args)) return -EINVAL; /* Copy in args. */ if (copy_from_user(args.args, uargs->args, nargs * sizeof(rtas_arg_t)) != 0) return -EFAULT; /* * If this token doesn't correspond to a function the kernel * understands, you're not allowed to call it. */ func = rtas_token_to_function_untrusted(token); if (!func) return -EINVAL; args.rets = &args.args[nargs]; memset(args.rets, 0, nret * sizeof(rtas_arg_t)); Some questions: 1. The patch sanitizes 'nargs' immediately before the call to memset(), but shouldn't that happen before 'nargs' is used as an input to copy_from_user()? 2. If 'nargs' needs this treatment, then why wouldn't the user-supplied 'nret' and 'token' need them as well? 'nret' is used to index the same array as 'nargs'. And at least conceptually, 'token' is used to index a data structure (xarray) with array-like semantics (to be fair, this is a relatively recent development and was not the case when this change was submitted).
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index 8afd146bc9c7..5ef3c863003d 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -27,6 +27,7 @@ #include <linux/slab.h> #include <linux/reboot.h> #include <linux/syscalls.h> +#include <linux/nospec.h> #include <asm/prom.h> #include <asm/rtas.h> @@ -1056,7 +1057,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) struct rtas_args args; unsigned long flags; char *buff_copy, *errbuf = NULL; - int nargs, nret, token; + int index, nargs, nret, token; if (!capable(CAP_SYS_ADMIN)) return -EPERM; @@ -1084,7 +1085,8 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) if (token == RTAS_UNKNOWN_SERVICE) return -EINVAL; - args.rets = &args.args[nargs]; + index = array_index_nospec(nargs, ARRAY_SIZE(args.args)); + args.rets = &args.args[index]; memset(args.rets, 0, nret * sizeof(rtas_arg_t)); /* Need to handle ibm,suspend_me call specially */
The rtas syscall reads a value from a user-provided structure and uses it to index an array, being a possible area for a potential spectre v1 attack. This is the code that exposes this problem. args.rets = &args.args[nargs]; The nargs is an user provided value, and the below code is an example where the 'nargs' value would be set to XX. struct rtas_args ra; ra.nargs = htobe32(XX); syscall(__NR_rtas, &ra); Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/rtas.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)