1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/net: ftgmac100: copy eth_hdr for alignment

Patrick Venture posted 1 patch 1 month ago 
hw/net/ftgmac100.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
[PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
Posted by Patrick Venture 1 month ago 
eth_hdr requires 2 byte alignment

Signed-off-by: Patrick Venture <venture@google.com>
---
 hw/net/ftgmac100.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 1f524d7a01..a33aaa01ee 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -989,12 +989,16 @@ static void ftgmac100_high_write(void *opaque, hwaddr addr,
 static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
 {
     unsigned mcast_idx;
+    struct eth_header eth_hdr = {};
 
     if (s->maccr & FTGMAC100_MACCR_RX_ALL) {
         return 1;
     }
 
-    switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
+    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
+           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
+
+    switch (get_eth_packet_type(&eth_hdr)) {
     case ETH_PKT_BCAST:
         if (!(s->maccr & FTGMAC100_MACCR_RX_BROADPKT)) {
             return 0;
@@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
 {
     FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
     FTGMAC100Desc bd;
+    struct eth_header eth_hdr = {};
     uint32_t flags = 0;
     uint64_t addr;
     uint32_t crc;
@@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
     uint32_t buf_len;
     size_t size = len;
     uint32_t first = FTGMAC100_RXDES0_FRS;
-    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
+    uint16_t proto;
+
+    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
+           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
+    proto = be16_to_cpu(eth_hdr.h_proto);
     int max_frame_size = ftgmac100_max_frame_size(s, proto);
 
     if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN))
@@ -1061,7 +1070,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
         flags |= FTGMAC100_RXDES0_FTL;
     }
 
-    switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
+    switch (get_eth_packet_type(&eth_hdr)) {
     case ETH_PKT_BCAST:
         flags |= FTGMAC100_RXDES0_BROADCAST;
         break;
-- 
2.48.1.658.g4767266eb4-goog
Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
Posted by Philippe Mathieu-Daudé 1 month ago 
Hi Patrick,

On 27/2/25 16:42, Patrick Venture wrote:
> eth_hdr requires 2 byte alignment
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>   hw/net/ftgmac100.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)


> @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>   {
>       FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
>       FTGMAC100Desc bd;
> +    struct eth_header eth_hdr = {};
>       uint32_t flags = 0;
>       uint64_t addr;
>       uint32_t crc;
> @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>       uint32_t buf_len;
>       size_t size = len;
>       uint32_t first = FTGMAC100_RXDES0_FRS;
> -    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);

The LD/ST API deals with unaligned fields, would that help?

     uint16_t proto = lduw_be_p(&PKT_GET_ETH_HDR(buf)->h_proto);

> +    uint16_t proto;
> +
> +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
> +    proto = be16_to_cpu(eth_hdr.h_proto);
>       int max_frame_size = ftgmac100_max_frame_size(s, proto);
Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
Posted by Peter Maydell 1 month ago 
On Mon, 3 Mar 2025 at 11:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Patrick,
>
> On 27/2/25 16:42, Patrick Venture wrote:
> > eth_hdr requires 2 byte alignment
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> >   hw/net/ftgmac100.c | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
>
>
> > @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> >   {
> >       FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> >       FTGMAC100Desc bd;
> > +    struct eth_header eth_hdr = {};
> >       uint32_t flags = 0;
> >       uint64_t addr;
> >       uint32_t crc;
> > @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> >       uint32_t buf_len;
> >       size_t size = len;
> >       uint32_t first = FTGMAC100_RXDES0_FRS;
> > -    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
>
> The LD/ST API deals with unaligned fields, would that help?
>
>      uint16_t proto = lduw_be_p(&PKT_GET_ETH_HDR(buf)->h_proto);

No, it doesn't, unfortunately -- I forget the details, but
if the struct is unaligned but its definition says it is
aligned then you get UB even with our accessor functions.
This is why in commit f8b94b4c520126 I had to fix this for
struct ip_header by marking it as QEMU_PACKED, even though
the actual access there was a uint8_t*.

(See also the other thread where I suggested to Patrick that
the best approach here is to mark eth_hdr as QEMU_PACKED,
and fix up anything we need to do to make that change.)

thanks
-- PMM
Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
Posted by Andrew Jeffery 1 month ago 
Hi Patrick,

On Thu, 2025-02-27 at 15:42 +0000, Patrick Venture wrote:
> eth_hdr requires 2 byte alignment
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>  hw/net/ftgmac100.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 1f524d7a01..a33aaa01ee 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -989,12 +989,16 @@ static void ftgmac100_high_write(void *opaque, hwaddr addr,
>  static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
>  {
>      unsigned mcast_idx;
> +    struct eth_header eth_hdr = {};
>  
>      if (s->maccr & FTGMAC100_MACCR_RX_ALL) {
>          return 1;
>      }
>  
> -    switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));

I don't think truncating the memcpy() in this way is what we want? The
switched value may not be meaningful for small values of len.

Perhaps return an error?

> +
> +    switch (get_eth_packet_type(&eth_hdr)) {
>      case ETH_PKT_BCAST:
>          if (!(s->maccr & FTGMAC100_MACCR_RX_BROADPKT)) {
>              return 0;
> @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>  {
>      FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
>      FTGMAC100Desc bd;
> +    struct eth_header eth_hdr = {};
>      uint32_t flags = 0;
>      uint64_t addr;
>      uint32_t crc;
> @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>      uint32_t buf_len;
>      size_t size = len;
>      uint32_t first = FTGMAC100_RXDES0_FRS;
> -    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
> +    uint16_t proto;
> +
> +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));

Again here.

> +    proto = be16_to_cpu(eth_hdr.h_proto);
>      int max_frame_size = ftgmac100_max_frame_size(s, proto);
>  
>      if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN))
> @@ -1061,7 +1070,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>          flags |= FTGMAC100_RXDES0_FTL;
>      }
>  
> -    switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> +    switch (get_eth_packet_type(&eth_hdr)) {
>      case ETH_PKT_BCAST:
>          flags |= FTGMAC100_RXDES0_BROADCAST;
>          break;
Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
Posted by Patrick Venture 1 month ago 
On Thu, Feb 27, 2025 at 9:54 PM Andrew Jeffery <andrew@codeconstruct.com.au>
wrote:

> Hi Patrick,
>
> On Thu, 2025-02-27 at 15:42 +0000, Patrick Venture wrote:
> > eth_hdr requires 2 byte alignment
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> >  hw/net/ftgmac100.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> > index 1f524d7a01..a33aaa01ee 100644
> > --- a/hw/net/ftgmac100.c
> > +++ b/hw/net/ftgmac100.c
> > @@ -989,12 +989,16 @@ static void ftgmac100_high_write(void *opaque,
> hwaddr addr,
> >  static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf,
> size_t len)
> >  {
> >      unsigned mcast_idx;
> > +    struct eth_header eth_hdr = {};
> >
> >      if (s->maccr & FTGMAC100_MACCR_RX_ALL) {
> >          return 1;
> >      }
> >
> > -    switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>
> I don't think truncating the memcpy() in this way is what we want? The
> switched value may not be meaningful for small values of len.
>
> Perhaps return an error?
>
> > +
> > +    switch (get_eth_packet_type(&eth_hdr)) {
> >      case ETH_PKT_BCAST:
> >          if (!(s->maccr & FTGMAC100_MACCR_RX_BROADPKT)) {
> >              return 0;
> > @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> >  {
> >      FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> >      FTGMAC100Desc bd;
> > +    struct eth_header eth_hdr = {};
> >      uint32_t flags = 0;
> >      uint64_t addr;
> >      uint32_t crc;
> > @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> >      uint32_t buf_len;
> >      size_t size = len;
> >      uint32_t first = FTGMAC100_RXDES0_FRS;
> > -    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
> > +    uint16_t proto;
> > +
> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>
> Again here.
>
> > +    proto = be16_to_cpu(eth_hdr.h_proto);
> >      int max_frame_size = ftgmac100_max_frame_size(s, proto);
> >
> >      if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN |
> FTGMAC100_MACCR_RXMAC_EN))
> > @@ -1061,7 +1070,7 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> >          flags |= FTGMAC100_RXDES0_FTL;
> >      }
> >
> > -    switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> > +    switch (get_eth_packet_type(&eth_hdr)) {
> >      case ETH_PKT_BCAST:
> >          flags |= FTGMAC100_RXDES0_BROADCAST;
> >          break;
>
>
Thanks, I've been asked to fix eth_header to be correctly packed instead
and I'm still fixing the implications of that.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.