[PATCH for-5.2 0/4] hw/net/can/ctucan: fix Coverity and other issues

[PATCH for-5.2 0/4] hw/net/can/ctucan: fix Coverity and other issues

[Peter Maydell]

This patchset fixes a couple of issues spotted by Coverity:
 * incorrect address checks meant the guest could write off the
   end of the tx_buffer arrays
 * we had an unused value in ctucan_send_ready_buffers()
and also some I noticed while reading the code:
 * we don't adjust the device's non-portable use of bitfields
   on bigendian hosts
 * we should use stl_le_p() rather than casting uint_t* to
   uint32_t*

Tested with "make check" only.

thanks
 -- PMM

Peter Maydell (4):
  hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer
  hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()
  hw/net/can/ctucan_core: Handle big-endian hosts
  hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

 hw/net/can/ctucan_core.h |  3 +--
 hw/net/can/ctucan_core.c | 23 +++++++----------------
 2 files changed, 8 insertions(+), 18 deletions(-)

-- 
2.20.1

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

[Peter Maydell]

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;
     }
 
@@ -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);
+        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);
         }
-- 
2.20.1

[PATCH for-5.2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Peter Maydell]

Coverity points out that in ctucan_send_ready_buffers() we
set buff_st_mask = 0xf << (i * 4) inside the loop, but then
we never use it before overwriting it later.

The only thing we use the mask for is as part of the code that is
inserting the new buff_st field into tx_status.  That is more
comprehensibly written using deposit32(), so do that and drop the
mask variable entirely.

We also update the buff_st local variable at multiple points
during this function, but nothing can ever see these
intermediate values, so just drop those, write the final
TXT_TOK as a fixed constant value, and collapse the only
remaining set/use of buff_st down into an extract32().

Fixes: Coverity CID 1432869
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/can/ctucan_core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index ea09bf71a0c..f2ce978e5ec 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
     uint8_t *pf;
     int buff2tx_idx;
     uint32_t tx_prio_max;
-    unsigned int buff_st;
-    uint32_t buff_st_mask;
 
     if (!s->mode_settings.s.ena) {
         return;
@@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
         for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
             uint32_t prio;
 
-            buff_st_mask = 0xf << (i * 4);
-            buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
-
-            if (buff_st != TXT_RDY) {
+            if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
                 continue;
             }
             prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
@@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
         if (buff2tx_idx == -1) {
             break;
         }
-        buff_st_mask = 0xf << (buff2tx_idx * 4);
-        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
         int_stat.u32 = 0;
-        buff_st = TXT_RDY;
         pf = s->tx_buffer[buff2tx_idx].data;
         ctucan_buff2frame(pf, &frame);
         s->status.s.idle = 0;
@@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
         s->status.s.idle = 1;
         s->status.s.txs = 0;
         s->tx_fr_ctr.s.tx_fr_ctr_val++;
-        buff_st = TXT_TOK;
         int_stat.s.txi = 1;
         int_stat.s.txbhci = 1;
         s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
-        s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
-                        (buff_st << (buff2tx_idx * 4));
+        s->tx_status.u32 = deposit32(s->tx_status.u32,
+                                     buff2tx_idx * 4, 4, TXT_TOK);
     } while (1);
 }
 
-- 
2.20.1

[PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts

[Peter Maydell]

The ctucan driver defines types for its registers which are a union
of a uint32_t with a struct with bitfields for the individual
fields within that register. This is a bad idea, because bitfields
aren't portable. The ctu_can_fd_regs.h header works around the
most glaring of the portability issues by defining the
fields in two different orders depending on the setting of the
__LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this
is unconditionally set to 1, which is wrong for big-endian hosts.

Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need
for a "have we defined it already" guard, because the only place
that should set it is ctucan_core.h, which has the usual
double-inclusion guard.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Ideally all that bitfield-using code would be rewritten to use
extract32 and deposit32 instead, IMHO.
---
 hw/net/can/ctucan_core.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h
index f21cb1c5ec3..bbc09ae0678 100644
--- a/hw/net/can/ctucan_core.h
+++ b/hw/net/can/ctucan_core.h
@@ -31,8 +31,7 @@
 #include "exec/hwaddr.h"
 #include "net/can_emu.h"
 
-
-#ifndef __LITTLE_ENDIAN_BITFIELD
+#ifndef HOST_WORDS_BIGENDIAN
 #define __LITTLE_ENDIAN_BITFIELD 1
 #endif
 
-- 
2.20.1

[PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

[Peter Maydell]

Instead of casting an address within a uint8_t array to a
uint32_t*, use stl_le_p(). This handles possibly misaligned
addresses which would otherwise crash on some hosts.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/can/ctucan_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index f2ce978e5ec..e66526efa83 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
         addr %= CTUCAN_CORE_TXBUFF_SPAN;
         assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
         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);
+            stl_le_p(s->tx_buffer[buff_num].data + addr, val);
         }
     } else {
         switch (addr & ~3) {
-- 
2.20.1

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

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

Re: [PATCH for-5.2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Pavel Pisa]

Hello Peter,

this one is a little problematic. I understand that you want
to have clean code and no warnings reports from coverity.

On Friday 06 of November 2020 18:11:51 Peter Maydell wrote:
> Coverity points out that in ctucan_send_ready_buffers() we
> set buff_st_mask = 0xf << (i * 4) inside the loop, but then
> we never use it before overwriting it later.
>
> The only thing we use the mask for is as part of the code that is
> inserting the new buff_st field into tx_status.  That is more
> comprehensibly written using deposit32(), so do that and drop the
> mask variable entirely.
>
> We also update the buff_st local variable at multiple points
> during this function, but nothing can ever see these
> intermediate values, so just drop those, write the final
> TXT_TOK as a fixed constant value, and collapse the only
> remaining set/use of buff_st down into an extract32().
>
> Fixes: Coverity CID 1432869
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/can/ctucan_core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index ea09bf71a0c..f2ce978e5ec 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) uint8_t *pf;
>      int buff2tx_idx;
>      uint32_t tx_prio_max;
> -    unsigned int buff_st;
> -    uint32_t buff_st_mask;
>
>      if (!s->mode_settings.s.ena) {
>          return;
> @@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
>              uint32_t prio;
>
> -            buff_st_mask = 0xf << (i * 4);
> -            buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
> -
> -            if (buff_st != TXT_RDY) {
> +            if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
>                  continue;
>              }

It is right replacement. Initially buff_st kept location of bits of the buffer 
found to be processed later. But after priorization of Tx this cannot
be used without recompute.

>              prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
> @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) if (buff2tx_idx == -1) {
>              break;
>          }
> -        buff_st_mask = 0xf << (buff2tx_idx * 4);
> -        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;

There I would kept extracted state in the variable. Actual model is really
simplified to real hardware. Tx succeeds in zero time.

>          int_stat.u32 = 0;
> -        buff_st = TXT_RDY;

This is why the TXT_RDY state immediately changes to TXT_TOK. It works well
for actual simple CAN subsystem implementation. But if we want to implement
priorization of messages on emulated bus and even simulate real bus latency
by delay to state change and interrut delivery, then we need to proceed
through TXT_RDY state. If it is a problem for release, that your want to have
coverity clean source tree, then please left the line as a comment there
or use some trick with
           (void)buff_st;

Or if you prefer, use

  +        s->tx_status.u32 = deposit32(s->tx_status.u32,
  +                                     buff2tx_idx * 4, 4, TXT_RDY);

if it silent the coverity.

>          pf = s->tx_buffer[buff2tx_idx].data;
>          ctucan_buff2frame(pf, &frame);
>          s->status.s.idle = 0;
> @@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) s->status.s.idle = 1;
>          s->status.s.txs = 0;
>          s->tx_fr_ctr.s.tx_fr_ctr_val++;
> -        buff_st = TXT_TOK;
>          int_stat.s.txi = 1;
>          int_stat.s.txbhci = 1;
>          s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
> -        s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
> -                        (buff_st << (buff2tx_idx * 4));
> +        s->tx_status.u32 = deposit32(s->tx_status.u32,
> +                                     buff2tx_idx * 4, 4, TXT_TOK);
>      } while (1);
>  }


Thanks for your time, I have planned to look and these and attempt
to provide solution which is acceptable but documents our intentions,
but it is on my tasks queue still,

Pavel

Re: [PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

[Pavel Pisa]

On Friday 06 of November 2020 18:11:53 Peter Maydell wrote:
> Instead of casting an address within a uint8_t array to a
> uint32_t*, use stl_le_p(). This handles possibly misaligned
> addresses which would otherwise crash on some hosts.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/can/ctucan_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index f2ce978e5ec..e66526efa83 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> uint64_t val, addr %= CTUCAN_CORE_TXBUFF_SPAN;
>          assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
>          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);
> +            stl_le_p(s->tx_buffer[buff_num].data + addr, val);
>          }
>      } else {
>          switch (addr & ~3) {

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

even that I do not like stl_le_p name, because it differs from the Linux
kernel one. cpu_to_le32 matches. The pointer variant is cpu_to_le32p
on Linux kernel side, I think. stl is strange name and l for long
is problematic as well, if it is st32_le_p or st_le32_p I would recognize
that much easier.

Best wishes,

Pavel Pisa

Re: [PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts

[Philippe Mathieu-Daudé]

On 11/6/20 6:11 PM, Peter Maydell wrote:
> The ctucan driver defines types for its registers which are a union
> of a uint32_t with a struct with bitfields for the individual
> fields within that register. This is a bad idea, because bitfields
> aren't portable. The ctu_can_fd_regs.h header works around the
> most glaring of the portability issues by defining the
> fields in two different orders depending on the setting of the
> __LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this
> is unconditionally set to 1, which is wrong for big-endian hosts.
> 
> Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need
> for a "have we defined it already" guard, because the only place
> that should set it is ctucan_core.h, which has the usual
> double-inclusion guard.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Ideally all that bitfield-using code would be rewritten to use
> extract32 and deposit32 instead, IMHO.
> ---
>  hw/net/can/ctucan_core.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h
> index f21cb1c5ec3..bbc09ae0678 100644
> --- a/hw/net/can/ctucan_core.h
> +++ b/hw/net/can/ctucan_core.h
> @@ -31,8 +31,7 @@
>  #include "exec/hwaddr.h"
>  #include "net/can_emu.h"
>  
> -
> -#ifndef __LITTLE_ENDIAN_BITFIELD
> +#ifndef HOST_WORDS_BIGENDIAN
>  #define __LITTLE_ENDIAN_BITFIELD 1
>  #endif

Alternatively s/#ifdef __LITTLE_ENDIAN_BITFIELD/#ifndef
HOST_WORDS_BIGENDIAN/ the codebase and remove this here...

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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,

On Friday 06 of November 2020 19:04:38 Peter Maydell wrote:
> 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.

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


 

Re: [PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

[Philippe Mathieu-Daudé]

On 11/6/20 6:11 PM, Peter Maydell wrote:
> Instead of casting an address within a uint8_t array to a
> uint32_t*, use stl_le_p(). This handles possibly misaligned
> addresses which would otherwise crash on some hosts.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/can/ctucan_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index f2ce978e5ec..e66526efa83 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
>          addr %= CTUCAN_CORE_TXBUFF_SPAN;
>          assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
>          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);
> +            stl_le_p(s->tx_buffer[buff_num].data + addr, val);

Out of curiosity, how did you notice? Passing by while reviewing?

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

[Peter Maydell]

On Fri, 6 Nov 2020 at 18:19, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> On Friday 06 of November 2020 18:11:53 Peter Maydell wrote:
> > Instead of casting an address within a uint8_t array to a
> > uint32_t*, use stl_le_p(). This handles possibly misaligned
> > addresses which would otherwise crash on some hosts.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---

> even that I do not like stl_le_p name, because it differs from the Linux
> kernel one. cpu_to_le32 matches. The pointer variant is cpu_to_le32p
> on Linux kernel side, I think. stl is strange name and l for long
> is problematic as well, if it is st32_le_p or st_le32_p I would recognize
> that much easier.

QEMU is not the kernel. We have our own naming conventions
and our own APIs. I agree that the b/w/l/q suffixing
is less intuitive than 8/16/32/64, but we have a lot of
functions using that convention, and the API is what it is.

thanks
-- PMM

Re: [PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

[Philippe Mathieu-Daudé]

On 11/6/20 7:31 PM, Philippe Mathieu-Daudé wrote:
> On 11/6/20 6:11 PM, Peter Maydell wrote:
>> Instead of casting an address within a uint8_t array to a
>> uint32_t*, use stl_le_p(). This handles possibly misaligned
>> addresses which would otherwise crash on some hosts.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/net/can/ctucan_core.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
>> index f2ce978e5ec..e66526efa83 100644
>> --- a/hw/net/can/ctucan_core.c
>> +++ b/hw/net/can/ctucan_core.c
>> @@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
>>          addr %= CTUCAN_CORE_TXBUFF_SPAN;
>>          assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
>>          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);
>> +            stl_le_p(s->tx_buffer[buff_num].data + addr, val);
> 
> Out of curiosity, how did you notice? Passing by while reviewing?

$ git grep -P '^\s+\*.*=.*(cpu_to.*|to_cpu)\('|wc -l
82

Re: [PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

[Peter Maydell]

On Fri, 6 Nov 2020 at 18:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 11/6/20 6:11 PM, Peter Maydell wrote:
> > Instead of casting an address within a uint8_t array to a
> > uint32_t*, use stl_le_p(). This handles possibly misaligned
> > addresses which would otherwise crash on some hosts.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/net/can/ctucan_core.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> > index f2ce978e5ec..e66526efa83 100644
> > --- a/hw/net/can/ctucan_core.c
> > +++ b/hw/net/can/ctucan_core.c
> > @@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
> >          addr %= CTUCAN_CORE_TXBUFF_SPAN;
> >          assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
> >          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);
> > +            stl_le_p(s->tx_buffer[buff_num].data + addr, val);
>
> Out of curiosity, how did you notice? Passing by while reviewing?

I saw it while I was fixing the Coverity issue from
patch 1, yes.

thanks
-- PMM

Re: [PATCH for-5.2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts

[Pavel Pisa]

On Friday 06 of November 2020 19:29:27 Philippe Mathieu-Daudé wrote:
> On 11/6/20 6:11 PM, Peter Maydell wrote:
> > The ctucan driver defines types for its registers which are a union
> > of a uint32_t with a struct with bitfields for the individual
> > fields within that register. This is a bad idea, because bitfields
> > aren't portable. The ctu_can_fd_regs.h header works around the
> > most glaring of the portability issues by defining the
> > fields in two different orders depending on the setting of the
> > __LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this
> > is unconditionally set to 1, which is wrong for big-endian hosts.
> >
> > Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need
> > for a "have we defined it already" guard, because the only place
> > that should set it is ctucan_core.h, which has the usual
> > double-inclusion guard.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Ideally all that bitfield-using code would be rewritten to use
> > extract32 and deposit32 instead, IMHO.
> > ---
> >  hw/net/can/ctucan_core.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h
> > index f21cb1c5ec3..bbc09ae0678 100644
> > --- a/hw/net/can/ctucan_core.h
> > +++ b/hw/net/can/ctucan_core.h
> > @@ -31,8 +31,7 @@
> >  #include "exec/hwaddr.h"
> >  #include "net/can_emu.h"
> >
> > -
> > -#ifndef __LITTLE_ENDIAN_BITFIELD
> > +#ifndef HOST_WORDS_BIGENDIAN
> >  #define __LITTLE_ENDIAN_BITFIELD 1
> >  #endif
>
> Alternatively s/#ifdef __LITTLE_ENDIAN_BITFIELD/#ifndef
> HOST_WORDS_BIGENDIAN/ the codebase and remove this here...

But then we cannot have same generated, untouch header file
common for Linux kernel and QEMU. Each uses different defines.
Or at least it was the goal, but after mainline Linux review
we switch in longer run to defines with use of BIT, GENMASK
FIELD_GET and FIELD_PREP.

But even GENMASK does not seem to be defined in QEMU headers
even that it is referenced in include/hw/usb/dwc2-regs.h
and include/standard-headers/asm-x86/kvm_para.h

So idea to have nice common generated headers which can
be checked for consistency and right version by diff
for Linux kernel, QEMU and even userspace tests
and other OSes (there with Linux defines substitution)
seems to be only dream.

Anyway, we switch to solution which is matching requirements
of each project if it is suggested. But it takes time.

Best wishes,

Pavel

Re: [PATCH for-5.2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Peter Maydell]

On Fri, 6 Nov 2020 at 18:12, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> this one is a little problematic. I understand that you want
> to have clean code and no warnings reports from coverity.
>
> On Friday 06 of November 2020 18:11:51 Peter Maydell wrote:

> > @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> > *s) if (buff2tx_idx == -1) {
> >              break;
> >          }
> > -        buff_st_mask = 0xf << (buff2tx_idx * 4);
> > -        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
>
> There I would kept extracted state in the variable. Actual model is really
> simplified to real hardware. Tx succeeds in zero time.
>
> >          int_stat.u32 = 0;
> > -        buff_st = TXT_RDY;
>
> This is why the TXT_RDY state immediately changes to TXT_TOK. It works well
> for actual simple CAN subsystem implementation. But if we want to implement
> priorization of messages on emulated bus and even simulate real bus latency
> by delay to state change and interrut delivery, then we need to proceed
> through TXT_RDY state. If it is a problem for release, that your want to have
> coverity clean source tree, then please left the line as a comment there
> or use some trick with
>            (void)buff_st;
>
> Or if you prefer, use
>
>   +        s->tx_status.u32 = deposit32(s->tx_status.u32,
>   +                                     buff2tx_idx * 4, 4, TXT_RDY);
>
> if it silent the coverity.

I was going to put a comment in v2 of this patch series to
document that this is where the status goes to TXT_RDY,
but looking at the code, at this point the buffer status field
is *already* TXT_RDY -- the preceding loop does not allow
us to get to this point for an entry which is in any other
state. So while I agree with your suggestion that it's worth
having at least a documentation comment to indicate where the
state is changing, I think there is no intermediate state
transition to document here.

thanks
-- PMM