Re: [PATCH] hw/intc: sifive_plic: Fix heap-buffer-overflow in SiFive PLIC read operation

[Alistair Francis]

On Thu, Jul 4, 2024 at 7:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 3 Jul 2024 at 22:32, Zheyu Ma <zheyuma97@gmail.com> wrote:
> >
> > The sifive_plic_read function in hw/intc/sifive_plic.c had a potential
> > heap-buffer-overflow issue when reading from the pending_base region.
> > This occurred because the code did not check if the calculated word index
> > was within valid bounds before accessing the pending array.
> >
> > This fix prevents out-of-bounds memory access, ensuring safer and more
> > robust handling of PLIC reads.
> >
> > ASAN log:
> > ==78800==ERROR: AddressSanitizer: heap-buffer-overflow on address 
> > 0x602000038a14 at pc 0x5baf49d0d6cb bp 0x7ffc2ea4e180 sp 0x7ffc2ea4e178
> > READ of size 4 at 0x602000038a14 thread T0
> >     #0 0x5baf49d0d6ca in sifive_plic_read hw/intc/sifive_plic.c:151:16
> >     #1 0x5baf49f7f3bb in memory_region_read_accessor system/memory.c:445:11
> >
> > Reproducer:
> > cat << EOF | qemu-system-riscv64  -display \
> > none -machine accel=qtest, -m 512M -machine shakti_c -m 2G -qtest stdio
> > readl 0xc001004
> > EOF
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >  hw/intc/sifive_plic.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> > index e559f11805..d2a90dfd3a 100644
> > --- a/hw/intc/sifive_plic.c
> > +++ b/hw/intc/sifive_plic.c
> > @@ -147,7 +147,14 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr 
> > addr, unsigned size)
> >                              (plic->num_sources + 31) >> 3)) {
> >          uint32_t word = (addr - plic->pending_base) >> 2;
> >
> > -        return plic->pending[word];
> > +        if (word < plic->bitfield_words) {
> > +            return plic->pending[word];
> > +        } else {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "sifive_plic_read: Word out of bounds for 
> > pending_base read: word=%u\n",
> > +                          word);
> > +            return 0;
> > +        }
>
> This seems a bit odd. This part of the code is guarded by
>
>     } else if (addr_between(addr, plic->pending_base,
>                             (plic->num_sources + 31) >> 3)) {
>
> and we calculate plic->bitfield_words in realize based on
> plic->num_sources:
>     s->bitfield_words = (s->num_sources + 31) >> 5;
>
> so presumably the intention was that we put enough words
> in the bitfield for the number of sources we have, so that
> the array access wouldn't overrun. Maybe we got the
> calculation wrong?

Yeah, the calculation is wrong here.

We have

s->bitfield_words = (s->num_sources + 31) >> 5;
...
s->pending = g_new0(uint32_t, s->bitfield_words);

But then the check is

    } else if (addr_between(addr, plic->pending_base,
                            (plic->num_sources + 31) >> 3)) {

So when we allocate we shift by 5, but then when we check we only shift by 3.

So that's the bug. It's not immediately obvious what the correct fix
is though, do you mind having a look?

Alistair

>
> thanks
> -- PMM
>