Re: [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

On 22/03/2021 10:12, Andrew Jones wrote:
> On Mon, Mar 22, 2021 at 09:45:09AM +0000, Nikos Nikoleris wrote:
>> Hi Drew,
>>
>> On 22/03/2021 09:31, Andrew Jones wrote:
>>> On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote:
>>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>>>> ---
>>>>    lib/arm/asm/cpumask.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
>>>> index 6683bb6..02124de 100644
>>>> --- a/lib/arm/asm/cpumask.h
>>>> +++ b/lib/arm/asm/cpumask.h
>>>> @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src)
>>>>    static inline int cpumask_next(int cpu, const cpumask_t *mask)
>>>>    {
>>>> -  while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask))
>>>> +  while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask))
>>>>                    ;
>>>>            return cpu;
>>>
>>
>> Thanks for reviewing this!
>>
>>
>>> This looks like the right thing to do, but I'm surprised that
>>> I've never seen an assert in cpumask_test_cpu, even though
>>> it looks like we call cpumask_next with cpu == nr_cpus - 1
>>> in several places.
>>>
>>
>> cpumask_next() would trigger one of the assertions in the 4th patch in this
>> series without this fix. The 4th patch is a way to demonstrate (if we apply
>> it without the rest) the problem of using cpu0's thread_info->cpu
>> uninitialized.
>
> Ah, I see my error. I had already applied your 4th patch but hadn't
> reviewed it yet, so I didn't realize it was new code. Now it makes
> sense that we didn't hit that assert before (it didn't exist
> before :-)
>
>>
>>> Can you please add a commit message explaining how you found
>>> this bug?
>>>
>>
>> Yes I'll do that.
>
> If you just write one here then I'll add it while applying. The rest of
> the patches look good to me. So no need to respin.
>

Sounds good! Maybe we can add something along the lines of:

Prior to this change, a call of cpumask_next(cpu, mask) where cpu=nr_cpu
- 1 (assuming all cpus are enumerated in the range 0..nr_cpus - 1) would
make an out-of-bounds access to the mask. In many cases, this is still a
valid memory location due the implementation of cpumask_t, however, in
certain configurations (for example, nr_cpus == sizeof(long)) this would
cause an access outside the bounds of the mask too.

This patch changes the way we guard calls to cpumask_test_cpu() in
cpumask_next() to avoid the above condition. A following change adds
assertions to catch out-of-bounds accesses to cpumask_t.

Thanks,

Nikos

> Thanks,
> drew
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.