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

[Peter Maydell]

On Tue, 10 Nov 2020 at 18:02, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> On Tuesday 10 of November 2020 18:06:01 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
> >
> > NB: currently the values of CTUCAN_CORE_MEM_SIZE, CTUCAN_CORE_TXBUF_NUM,
> > etc, make "buff_num >= CTUCAN_CORE_TXBUF_NUM" impossible, but we
> > retain this as a runtime check rather than an assertion to permit
> > those values to be changed in future (in hardware they are
> > configurable synthesis parameters).
> >
> > Fix the top level check, and check the offset is within the buffer.
> >
> > 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..538270e62f9 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;
> >      }
>
> Ack
>
> > @@ -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) {
> > +        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
> > +            (addr < sizeof(s->tx_buffer[buff_num].data))) {
>
> should be &&

Whoops, that's a silly mistake on my part.

> I would use
>
> +        if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
> +            addr < CTUCAN_CORE_MSG_MAX_LEN) {
>
> But that is equal. There can be problem that last three bytes of the uint32_t
> type can fall after the end. The correct changes to fully support
> unaligned writes is not so easy an dis unnecessary for actual drivers
> and use. So suggest

>> +        addr &= ~3;
> +        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) &&
> +            (addr < sizeof(s->tx_buffer[buff_num].data))) {

Hmm, yeah, the code is currently doing a 32-bit read regardless.

> You can consider that as Acked by me

OK, let's go with your version for 5.2.

For unaligned accesses, for 6.0, I think the code for doing
them to the txbuff at least is straightforward:

   if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
       (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) {
      stn_le_p(s->tx_buffer[buff_num].data + addr, size, val);
   }

(stn_le_p takes care of doing an appropriate-width write.)

thanks
-- PMM