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

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

[Mark Harris]

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(+)

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;
-- 
2.54.0

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

[Nam Cao]

Mark Harris <mark.hsj@gmail.com> writes:
> 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>

Reviewed-by: Nam Cao <namcao@linutronix.de>

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

[Michael Ellerman]

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")

> 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;

Reviewed-by: Michael Ellerman <mpe@kernel.org>

cheers

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

[Mark Harris]

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.

 - Mark

>
> Reviewed-by: Michael Ellerman <mpe@kernel.org>
>
> cheers

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

[Michael Ellerman]

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