Re: [PATCH for-5.2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Peter Maydell]

On Fri, 6 Nov 2020 at 17:48, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> thanks much for the catching the problem and investing time into
> fixing. I hope to find time for more review of remarks and Xilinx
> patches next week. I do not find reasonable time slot till Sunday.
> Excuse me. To not block updates, I confirm your changes.
>
> On Friday 06 of November 2020 18:11:50 Peter Maydell wrote:
> > The ctucan device has 4 CAN bus cores, each of which has a set of 20
> > 32-bit registers for writing the transmitted data. The registers are
> > however not contiguous; each core's buffers is 0x100 bytes after
> > the last.
> >
> > We got the checks on the address wrong in the ctucan_mem_write()
> > function:
> >  * the first "is addr in range at all" check allowed
> >    addr == CTUCAN_CORE_MEM_SIZE, which is actually the first
> >    byte off the end of the range
> >  * the decode of addresses into core-number plus offset in the
> >    tx buffer for that core failed to check that the offset was
> >    in range, so the guest could write off the end of the
> >    tx_buffer[] array
> >  * the decode had an explicit check for whether the core-number
> >    was out of range, which is actually impossible given the
> >    CTUCAN_CORE_MEM_SIZE check and the number of cores.
> >
> > Fix the top level check, check the offset, and turn the check
> > on the core-number into an assertion.
> >
> > Fixes: Coverity CID 1432874
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/net/can/ctucan_core.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> > index d20835cd7e9..ea09bf71a0c 100644
> > --- a/hw/net/can/ctucan_core.c
> > +++ b/hw/net/can/ctucan_core.c
> > @@ -303,7 +303,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> > uint64_t val, DPRINTF("write 0x%02llx addr 0x%02x\n",
> >              (unsigned long long)val, (unsigned int)addr);
> >
> > -    if (addr > CTUCAN_CORE_MEM_SIZE) {
> > +    if (addr >= CTUCAN_CORE_MEM_SIZE) {
> >          return;
> >      }
>
> Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

> > @@ -312,7 +312,8 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> > uint64_t val, addr -= CTU_CAN_FD_TXTB1_DATA_1;
> >          buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
> >          addr %= CTUCAN_CORE_TXBUFF_SPAN;
> > -        if (buff_num < CTUCAN_CORE_TXBUF_NUM) {
> > +        assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
>
> Assert is not necessary. If there is not buffer at that location,
> then write has no effect. Assert would check for driver errors,
> but it is not a problem of QEMU and for sure should not lead to its
> crash.

We assert() here as a guide to readers of the code that we know
that buff_num can't possibly be out of range for the array
access we're about to do: the values of CTUCAN_CORE_MEM_SIZE,
CTUCAN_CORE_TXBUFF_SPAN, etc, make it mathematically impossible.
We prefer to assert() that kind of condition rather than having
an if() test for it.

thanks
-- PMM