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

[Pavel Pisa]

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>

There is another mistake

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index d20835cd7e..b90a8e3b76 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -418,7 +418,7 @@ uint64_t ctucan_mem_read(CtuCanCoreState *s, hwaddr addr, 
unsigned size)

     DPRINTF("read addr 0x%02x ...\n", (unsigned int)addr);

-    if (addr > CTUCAN_CORE_MEM_SIZE) {
+    if (addr >= CTUCAN_CORE_MEM_SIZE) {
         return 0;
     }
But is should be really harmless.

> @@ -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.

> +        if (addr < sizeof(s->tx_buffer[buff_num].data)) {
>              uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data +
> addr); *bufp = cpu_to_le32(val);
>          }

So my proposed solution is

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index d20835cd7e..af30d57cfd 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -312,7 +312,9 @@ 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) {
+        addr &= ~3;
+        if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
+            addr < CTUCAN_CORE_MSG_MAX_LEN) {
             uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + addr);
             *bufp = cpu_to_le32(val);
         }

It ignores writes to unimplemented locations.

There is fix, workaround to make safe writes to unaligned addresses.
They are not supported by actual QEMU CTU CAN FD model.
Real HW supports them but driver never uses unaligned writes
nor any other size than 32-bits.

Reads supports at least accesses by smaller size correctly
but do not support unaligned reads which cross 32-bit boundaries.
Again, actual driver code never uses other size than 32-bits
for read. Byte access for debug dumps by rdwrmem by bytes
has been tested and works OK as well.

The following proposed correction right solution too

diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h
index f21cb1c5ec..ad701c4764 100644
--- a/hw/net/can/ctucan_core.h
+++ b/hw/net/can/ctucan_core.h
@@ -31,10 +31,11 @@
 #include "exec/hwaddr.h"
 #include "net/can_emu.h"

-
+#ifndef HOST_WORDS_BIGENDIAN
 #ifndef __LITTLE_ENDIAN_BITFIELD
 #define __LITTLE_ENDIAN_BITFIELD 1
 #endif
+#endif

 #include "ctu_can_fd_frame.h"
 #include "ctu_can_fd_regs.h"

As for bitfields use, there is plan to add additional mode to generate
registers header file from IPXACT register definition sources which
would be based only on defines to follow Linux kernel registers
definition style.

In the longer run, if there is time we can switch QEMU model
to use this additional style and headers.

Thanks much for time again,

Pavel