Re: [PATCH] riscv: hwprobe: Avoid uninitialized read in hwprobe_get_cpus()

[Michael Ellerman <mpe@kernel.org>]

On 16/6/26 1:53 pm, Mark Harris wrote:
> Michael Ellerman wrote:
>>
>> On 12/6/26 2:55 pm, Mark Harris wrote:
>>> When cpusetsize < cpumask_size(), hwprobe_get_cpus() did not fully
>>> initialize its copy of the cpu mask, which could cause non-deterministic
>>> results from the riscv_hwprobe syscall on a system with more than 8 CPUs
>>> when the supplied cpu mask is empty.  Address this by fully initializing
>>> the cpu mask.
>>>
>>> Signed-off-by: Mark Harris <mark.hsj@gmail.com>
>>> ---
>>>    arch/riscv/kernel/sys_hwprobe.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>
>> This should have a fixes tag, I think it's:
>>
>> Fixes: e178bf146e4b ("RISC-V: hwprobe: Introduce which-cpus flag")
> 
> Yes, that looks correct.
> 
>>
>>> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
>>> index 1659d31fd288..caf6762427c8 100644
>>> --- a/arch/riscv/kernel/sys_hwprobe.c
>>> +++ b/arch/riscv/kernel/sys_hwprobe.c
>>> @@ -450,6 +450,7 @@ static int hwprobe_get_cpus(struct riscv_hwprobe __user *pairs,
>>>        if (cpusetsize > cpumask_size())
>>>                cpusetsize = cpumask_size();
>>>
>>> +     cpumask_clear(&cpus);
>>>        ret = copy_from_user(&cpus, cpus_user, cpusetsize);
>>>        if (ret)
>>>                return -EFAULT;
>>
>> cpus is on the stack, and is copied back out at the end of the function,
>> so this looks like it could be a stack info leak.
>>
>> But the copy back is also bounded by cpusetsize, so in fact there is not
>> any leak of uninitialised stack out to userspace:
>>
>>           ret = copy_to_user(cpus_user, &cpus, cpusetsize);
>>           if (ret)
>>                   return -EFAULT;
> 
> It can leak 1 bit of information from the kernel stack.  For example
> with 16 CPUs, all online, if a 1-byte 0x00 mask is supplied, the
> caller can determine whether the second (uninitialized) mask byte is
> zero or non-zero due to the cpumask_empty(&cpus) check.
Oh yep, because the caller can observe if cpus was/wasn't overwritten 
with the cpus_online_mask.

I guess that counts as an info leak, it doesn't let an attacker 
reconstruct actual values from the stack, but a zero/non-zero check 
could be useful in theory.

cheers