QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads
@ 2020-03-24 21:21 Peter Maydell
  2020-03-25 15:17 ` Richard Henderson
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Peter Maydell @ 2020-03-24 21:21 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Beniamino Galvani, Jason Wang, Niek Linnenbank

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads
  2020-03-24 21:21 [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads Peter Maydell
@ 2020-03-25 15:17 ` Richard Henderson
  2020-03-25 21:03 ` Niek Linnenbank
  2020-03-27  2:00 ` Jason Wang
  2 siblings, 0 replies; 5+ messages in thread
From: Richard Henderson @ 2020-03-25 15:17 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel
  Cc: Beniamino Galvani, Jason Wang, Niek Linnenbank

On 3/24/20 2:21 PM, Peter Maydell 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>
> ---
>  hw/net/allwinner-sun8i-emac.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads
  2020-03-24 21:21 [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads Peter Maydell
  2020-03-25 15:17 ` Richard Henderson
@ 2020-03-25 21:03 ` Niek Linnenbank
  2020-03-25 21:11   ` Peter Maydell
  2020-03-27  2:00 ` Jason Wang
  2 siblings, 1 reply; 5+ messages in thread
From: Niek Linnenbank @ 2020-03-25 21:03 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Beniamino Galvani, Jason Wang, qemu-arm, QEMU Developers


[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads
  2020-03-25 21:03 ` Niek Linnenbank
@ 2020-03-25 21:11   ` Peter Maydell
  0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2020-03-25 21:11 UTC (permalink / raw)
  To: Niek Linnenbank; +Cc: Beniamino Galvani, Jason Wang, qemu-arm, QEMU Developers

On Wed, 25 Mar 2020 at 21:03, Niek Linnenbank <nieklinnenbank@gmail.com> wrote:
> By the way, is the coverity output of master publically available by any chance?

We use the public 'coverity scan' service:
https://scan.coverity.com/projects/qemu
You can create an account and look at the defects if you
like, but we don't generally expect everybody to do that.
Some of us tend to triage new issues as they come in and
report the non-false-positives to the list.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads
  2020-03-24 21:21 [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads Peter Maydell
  2020-03-25 15:17 ` Richard Henderson
  2020-03-25 21:03 ` Niek Linnenbank
@ 2020-03-27  2:00 ` Jason Wang
  2 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2020-03-27  2:00 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Beniamino Galvani, Niek Linnenbank


On 2020/3/25 上午5:21, Peter Maydell 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>
> ---
>   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 */


Applied.

Thanks




^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 21:21 [PATCH] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads Peter Maydell
2020-03-25 15:17 ` Richard Henderson
2020-03-25 21:03 ` Niek Linnenbank
2020-03-25 21:11   ` Peter Maydell
2020-03-27  2:00 ` Jason Wang

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git