[v1] hw/net/can/ctucan: fix Coverity and other issues

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

Posted by Peter Maydell 5 years, 3 months ago

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 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

Posted by Pavel Pisa 5 years, 3 months ago


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 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers

Posted by Peter Maydell 5 years, 3 months ago

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

Posted by Philippe Mathieu-Daudé 5 years, 3 months ago

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

Posted by Philippe Mathieu-Daudé 5 years, 3 months ago

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

Posted by Peter Maydell 5 years, 3 months ago

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