Re: [PATCH] hw/misc/npcm_clk: fix buffer-overflow

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 2/25/25 05:41, Peter Maydell wrote:
> On Mon, 24 Feb 2025 at 20:51, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> Regression introduced by cf76c4
>> (hw/misc: Add nr_regs and cold_reset_values to NPCM CLK)
>>
>> cold_reset_values has a different size, depending on device used
>> (NPCM7xx vs NPCM8xx). However, s->regs has a fixed size, which matches
>> NPCM8xx. Thus, when initializing a NPCM7xx, we go past cold_reset_values
>> ending.
> 
> 
>> diff --git a/hw/misc/npcm_clk.c b/hw/misc/npcm_clk.c
>> index d1f29759d59..0e85974cf96 100644
>> --- a/hw/misc/npcm_clk.c
>> +++ b/hw/misc/npcm_clk.c
>> @@ -964,8 +964,9 @@ static void npcm_clk_enter_reset(Object *obj, ResetType type)
>>       NPCMCLKState *s = NPCM_CLK(obj);
>>       NPCMCLKClass *c = NPCM_CLK_GET_CLASS(s);
>>
>> -    g_assert(sizeof(s->regs) >= c->nr_regs * sizeof(uint32_t));
>> -    memcpy(s->regs, c->cold_reset_values, sizeof(s->regs));
>> +    size_t sizeof_regs = c->nr_regs * sizeof(uint32_t);
>> +    g_assert(sizeof(s->regs) >= sizeof_regs);
>> +    memcpy(s->regs, c->cold_reset_values, sizeof_regs);
>>       s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>       npcm7xx_clk_update_all_clocks(s);
>>       /*
> 
> Whoops, thanks for catching this. Applied to target-arm.next, thanks.
> 
> (Looking more closely at the cold_reset_values handling
> in npcm_gcr.c, that looks not quite right in a different
> way; I'll send a reply to that patch email about that.)
>

It may be a hole in our CI right now.
Would that be interesting for CI to run all tests (check-functional + 
check w/o functional) with both ubsan and asan?

> -- PMM