Michael Ellerman <m...@ellerman.id.au> writes: > Breno Leitao <lei...@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).