Re: [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads

[Niek Linnenbank <nieklinnenbank@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]

On Tue, Mar 24, 2020 at 10:21 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> Coverity points out (CID 1421926) that the read code for
> REG_ADDR_HIGH reads off the end of the buffer, because it does a
> 32-bit read from byte 4 of a 6-byte buffer.
>
> The code also has an endianness issue for both REG_ADDR_HIGH and
> REG_ADDR_LOW, because it will do the wrong thing on a big-endian
> host.
>
> Rewrite the read code to use ldl_le_p() and lduw_le_p() to fix this;
> the write code is not incorrect, but for consistency we make it use
> stl_le_p() and stw_le_p().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

By the way, is the coverity output of master publically available by any
chance?

Regards,
Niek


> ---
>  hw/net/allwinner-sun8i-emac.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
> index 3fc5e346401..fc67a1be70a 100644
> --- a/hw/net/allwinner-sun8i-emac.c
> +++ b/hw/net/allwinner-sun8i-emac.c
> @@ -611,10 +611,10 @@ static uint64_t allwinner_sun8i_emac_read(void
> *opaque, hwaddr offset,
>          value = s->mii_data;
>          break;
>      case REG_ADDR_HIGH:         /* MAC Address High */
> -        value = *(((uint32_t *) (s->conf.macaddr.a)) + 1);
> +        value = lduw_le_p(s->conf.macaddr.a + 4);
>          break;
>      case REG_ADDR_LOW:          /* MAC Address Low */
> -        value = *(uint32_t *) (s->conf.macaddr.a);
> +        value = ldl_le_p(s->conf.macaddr.a);
>          break;
>      case REG_TX_DMA_STA:        /* Transmit DMA Status */
>          break;
> @@ -728,14 +728,10 @@ static void allwinner_sun8i_emac_write(void *opaque,
> hwaddr offset,
>          s->mii_data = value;
>          break;
>      case REG_ADDR_HIGH:         /* MAC Address High */
> -        s->conf.macaddr.a[4] = (value & 0xff);
> -        s->conf.macaddr.a[5] = (value & 0xff00) >> 8;
> +        stw_le_p(s->conf.macaddr.a + 4, value);
>          break;
>      case REG_ADDR_LOW:          /* MAC Address Low */
> -        s->conf.macaddr.a[0] = (value & 0xff);
> -        s->conf.macaddr.a[1] = (value & 0xff00) >> 8;
> -        s->conf.macaddr.a[2] = (value & 0xff0000) >> 16;
> -        s->conf.macaddr.a[3] = (value & 0xff000000) >> 24;
> +        stl_le_p(s->conf.macaddr.a, value);
>          break;
>      case REG_TX_DMA_STA:        /* Transmit DMA Status */
>      case REG_TX_CUR_DESC:       /* Transmit Current Descriptor */
> --
> 2.20.1
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 3858 bytes --]