[PATCH] powerpc/kernel: Fix potential spectre v1 in syscall

[Nathan Lynch]

Michael Ellerman <mpe at ellerman.id.au> writes:

> Breno Leitao <leitao at 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).