Hello Peter,
On Friday 06 of November 2020 19:04:38 Peter Maydell wrote:
> On Fri, 6 Nov 2020 at 17:48, Pavel Pisa <p...@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.mayd...@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 <p...@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.
I understand but I see as fully valid to have CTUCAN_CORE_MEM_SIZE
with some reserve and only two buffers populated which would lead
to "spare" space after the end of the area where buffers are mapped
into address-space. Same for CTUCAN_CORE_TXBUFF_SPAN
and CTUCAN_CORE_MSG_MAX_LEN. There could be check
assert(CTUCAN_CORE_MSG_MAX_LEN <= CTUCAN_CORE_TXBUFF_SPAN)
and
assert(CTUCAN_CORE_TXBUFF_SPAN * CTUCAN_CORE_TXBUF_NUM +
CTU_CAN_FD_TXTB1_DATA_1 <= CTUCAN_CORE_MEM_SIZE)
or even some cross checks of sizeof of the filed.
But all other combination are in the fact real, depends on core
synthesis parameters. Yes, for core release 2.x are the most
fixed now. But option to provide variant compatible with actual
driver which has CTUCAN_CORE_TXBUF_NUM > 4 up to hard limit of 8
is left open still.
Best wishes,
Pavel
