[PATCH 0/2] net: Make ip_header struct QEMU_PACKED

[PATCH 0/2] net: Make ip_header struct QEMU_PACKED

[Peter Maydell]

This patchset aims to fix some clang undefined-behavior
sanitizer warnings that you see if you run the arm
functional-tests:

         Stopping network: ../../net/checksum.c:106:9: runtime error: member access within misaligned address 0x556aad9b502e for type 'struct ip_header', which requires 4 byte alignment
        0x556aad9b502e: note: pointer points here
         34 56 08 00 45 00  01 48 a5 09 40 00 40 11  7c 8b 0a 00 02 0f 0a 00  02 02 00 44 00 43 01 34  19 56
                     ^
        SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../net/checksum.c:106:9 in
        ../../net/checksum.c:106:9: runtime error: load of misaligned address 0x556aad9b502e for type 'uint8_t' (aka 'unsigned char'), which requires 4 byte alignment
        0x556aad9b502e: note: pointer points here
         34 56 08 00 45 00  01 48 a5 09 40 00 40 11  7c 8b 0a 00 02 0f 0a 00  02 02 00 44 00 43 01 34  19 56
                     ^

These result from the net_checksum_calculate() function
creating a 'struct ip_header *' from an unaligned address.
We try to handle the non-alignment by using functions like
lduw_be_p(), but this is insufficient because even accessing
a byte member within an unaligned struct is UB.

This patchset fixes this by marking 'struct ip_header'
as QEMU_PACKED, which tells the compiler that it might
be at an unaligned address.

For that to work, we need to first fix the places in virtio-net.c
which take the address of a field within an ip_header:
the compiler will warn/error if we try to do that for a
field in a struct which is marked QEMU_PACKED.

Probably other structs in eth.h should be marked packed
too, but I am here only addressing the case that produces
warnings that I have seen.

thanks
-- PMM

Peter Maydell (2):
  hw/net/virtio-net.c: Don't assume IP length field is aligned
  net: mark struct ip_header as QEMU_PACKED

 include/hw/virtio/virtio-net.h |  2 +-
 include/net/eth.h              |  2 +-
 hw/net/virtio-net.c            | 23 +++++++++++++++++++----
 3 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/net/virtio-net.c: Don't assume IP length field is aligned

[Peter Maydell]

In virtio-net.c we assume that the IP length field in the packet is
aligned, and we copy its address into a uint16_t* in the
VirtioNetRscUnit struct which we then dereference later.  This isn't
a safe assumption; it will also result in compilation failures if we
mark the ip_header struct as QEMU_PACKED because the compiler will
not let you take the address of an unaligned struct field.

Make the ip_plen field in VirtioNetRscUnit a void*, and make all the
places where we read or write through that pointer instead use some
new accessor functions read_unit_ip_len() and write_unit_ip_len()
which account for the pointer being potentially unaligned and also do
the network-byte-order conversion we were previously using htons() to
perform.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/virtio/virtio-net.h |  2 +-
 hw/net/virtio-net.c            | 23 +++++++++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 060c23c04d2..b9ea9e824e3 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -102,7 +102,7 @@ typedef struct VirtioNetRscStat {
 /* Rsc unit general info used to checking if can coalescing */
 typedef struct VirtioNetRscUnit {
     void *ip;   /* ip header */
-    uint16_t *ip_plen;      /* data len pointer in ip header field */
+    void *ip_plen; /* pointer to unaligned uint16_t data len in ip header */
     struct tcp_header *tcp; /* tcp header */
     uint16_t tcp_hdrlen;    /* tcp header len */
     uint16_t payload;       /* pure payload without virtio/eth/ip/tcp */
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f2104ed364a..11cf462180d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2049,6 +2049,21 @@ static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,
     return virtio_net_receive_rcu(nc, buf, size, false);
 }
 
+/*
+ * Accessors to read and write the IP packet data length field. This
+ * is a potentially unaligned network-byte-order 16 bit unsigned integer
+ * pointed to by unit->ip_len.
+ */
+static uint16_t read_unit_ip_len(VirtioNetRscUnit *unit)
+{
+    return ldl_be_p(unit->ip_plen);
+}
+
+static void write_unit_ip_len(VirtioNetRscUnit *unit, uint16_t l)
+{
+    stl_be_p(unit->ip_plen, l);
+}
+
 static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain,
                                          const uint8_t *buf,
                                          VirtioNetRscUnit *unit)
@@ -2063,7 +2078,7 @@ static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain,
     unit->ip_plen = &ip->ip_len;
     unit->tcp = (struct tcp_header *)(((uint8_t *)unit->ip) + ip_hdrlen);
     unit->tcp_hdrlen = (htons(unit->tcp->th_offset_flags) & 0xF000) >> 10;
-    unit->payload = htons(*unit->ip_plen) - ip_hdrlen - unit->tcp_hdrlen;
+    unit->payload = read_unit_ip_len(unit) - ip_hdrlen - unit->tcp_hdrlen;
 }
 
 static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain,
@@ -2082,7 +2097,7 @@ static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain,
 
     /* There is a difference between payload length in ipv4 and v6,
        ip header is excluded in ipv6 */
-    unit->payload = htons(*unit->ip_plen) - unit->tcp_hdrlen;
+    unit->payload = read_unit_ip_len(unit) - unit->tcp_hdrlen;
 }
 
 static size_t virtio_net_rsc_drain_seg(VirtioNetRscChain *chain,
@@ -2231,7 +2246,7 @@ static int32_t virtio_net_rsc_coalesce_data(VirtioNetRscChain *chain,
     VirtioNetRscUnit *o_unit;
 
     o_unit = &seg->unit;
-    o_ip_len = htons(*o_unit->ip_plen);
+    o_ip_len = read_unit_ip_len(o_unit);
     nseq = htonl(n_unit->tcp->th_seq);
     oseq = htonl(o_unit->tcp->th_seq);
 
@@ -2267,7 +2282,7 @@ coalesce:
         o_unit->payload += n_unit->payload; /* update new data len */
 
         /* update field in ip header */
-        *o_unit->ip_plen = htons(o_ip_len + n_unit->payload);
+        write_unit_ip_len(o_unit, o_ip_len + n_unit->payload);
 
         /* Bring 'PUSH' big, the whql test guide says 'PUSH' can be coalesced
            for windows guest, while this may change the behavior for linux
-- 
2.34.1

Re: [PATCH 1/2] hw/net/virtio-net.c: Don't assume IP length field is aligned

[Peter Maydell]

On Thu, 7 Nov 2024 at 16:32, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In virtio-net.c we assume that the IP length field in the packet is
> aligned, and we copy its address into a uint16_t* in the
> VirtioNetRscUnit struct which we then dereference later.  This isn't
> a safe assumption; it will also result in compilation failures if we
> mark the ip_header struct as QEMU_PACKED because the compiler will
> not let you take the address of an unaligned struct field.
>
> Make the ip_plen field in VirtioNetRscUnit a void*, and make all the
> places where we read or write through that pointer instead use some
> new accessor functions read_unit_ip_len() and write_unit_ip_len()
> which account for the pointer being potentially unaligned and also do
> the network-byte-order conversion we were previously using htons() to
> perform.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/virtio/virtio-net.h |  2 +-
>  hw/net/virtio-net.c            | 23 +++++++++++++++++++----
>  2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 060c23c04d2..b9ea9e824e3 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -102,7 +102,7 @@ typedef struct VirtioNetRscStat {
>  /* Rsc unit general info used to checking if can coalescing */
>  typedef struct VirtioNetRscUnit {
>      void *ip;   /* ip header */
> -    uint16_t *ip_plen;      /* data len pointer in ip header field */
> +    void *ip_plen; /* pointer to unaligned uint16_t data len in ip header */
>      struct tcp_header *tcp; /* tcp header */
>      uint16_t tcp_hdrlen;    /* tcp header len */
>      uint16_t payload;       /* pure payload without virtio/eth/ip/tcp */
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index f2104ed364a..11cf462180d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2049,6 +2049,21 @@ static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,
>      return virtio_net_receive_rcu(nc, buf, size, false);
>  }
>
> +/*
> + * Accessors to read and write the IP packet data length field. This
> + * is a potentially unaligned network-byte-order 16 bit unsigned integer
> + * pointed to by unit->ip_len.
> + */
> +static uint16_t read_unit_ip_len(VirtioNetRscUnit *unit)
> +{
> +    return ldl_be_p(unit->ip_plen);
> +}
> +
> +static void write_unit_ip_len(VirtioNetRscUnit *unit, uint16_t l)
> +{
> +    stl_be_p(unit->ip_plen, l);
> +}

These should of course be lduw_be_p() and stw_be_p(), since
it's a 16 bit field.

Interestingly nothing fails in "make check" or "make check-functional"
if this breaks, suggesting we aren't exercising virtio-net very hard.

-- PMM

Re: [PATCH 1/2] hw/net/virtio-net.c: Don't assume IP length field is aligned

[Jason Wang]

On Fri, Nov 8, 2024 at 4:48 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 7 Nov 2024 at 16:32, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > In virtio-net.c we assume that the IP length field in the packet is
> > aligned, and we copy its address into a uint16_t* in the
> > VirtioNetRscUnit struct which we then dereference later.  This isn't
> > a safe assumption; it will also result in compilation failures if we
> > mark the ip_header struct as QEMU_PACKED because the compiler will
> > not let you take the address of an unaligned struct field.
> >
> > Make the ip_plen field in VirtioNetRscUnit a void*, and make all the
> > places where we read or write through that pointer instead use some
> > new accessor functions read_unit_ip_len() and write_unit_ip_len()
> > which account for the pointer being potentially unaligned and also do
> > the network-byte-order conversion we were previously using htons() to
> > perform.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  include/hw/virtio/virtio-net.h |  2 +-
> >  hw/net/virtio-net.c            | 23 +++++++++++++++++++----
> >  2 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index 060c23c04d2..b9ea9e824e3 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -102,7 +102,7 @@ typedef struct VirtioNetRscStat {
> >  /* Rsc unit general info used to checking if can coalescing */
> >  typedef struct VirtioNetRscUnit {
> >      void *ip;   /* ip header */
> > -    uint16_t *ip_plen;      /* data len pointer in ip header field */
> > +    void *ip_plen; /* pointer to unaligned uint16_t data len in ip header */
> >      struct tcp_header *tcp; /* tcp header */
> >      uint16_t tcp_hdrlen;    /* tcp header len */
> >      uint16_t payload;       /* pure payload without virtio/eth/ip/tcp */
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index f2104ed364a..11cf462180d 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2049,6 +2049,21 @@ static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,
> >      return virtio_net_receive_rcu(nc, buf, size, false);
> >  }
> >
> > +/*
> > + * Accessors to read and write the IP packet data length field. This
> > + * is a potentially unaligned network-byte-order 16 bit unsigned integer
> > + * pointed to by unit->ip_len.
> > + */
> > +static uint16_t read_unit_ip_len(VirtioNetRscUnit *unit)
> > +{
> > +    return ldl_be_p(unit->ip_plen);
> > +}
> > +
> > +static void write_unit_ip_len(VirtioNetRscUnit *unit, uint16_t l)
> > +{
> > +    stl_be_p(unit->ip_plen, l);
> > +}
>
> These should of course be lduw_be_p() and stw_be_p(), since
> it's a 16 bit field.
>
> Interestingly nothing fails in "make check" or "make check-functional"
> if this breaks, suggesting we aren't exercising virtio-net very hard.

If I was not wrong, we don't test RSC in those tests. And RSC is only
enabled for Windows guests for certification purposes.

Thanks

>
> -- PMM
>

Re: [PATCH 1/2] hw/net/virtio-net.c: Don't assume IP length field is aligned

[Yuri Benditovich]

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

On Sun, Nov 10, 2024 at 1:10 PM Yan Vugenfirer <yvugenfi@redhat.com> wrote:

> Please take a look is this is host\guest common struct
>
> ---------- Forwarded message ---------
> From: Peter Maydell <peter.maydell@linaro.org>
> Date: Thu, Nov 7, 2024 at 6:33 PM
> Subject: [PATCH 1/2] hw/net/virtio-net.c: Don't assume IP length field is
> aligned
> To: <qemu-devel@nongnu.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>, Jason Wang <jasowang@redhat.com>,
> Dmitry Fleytman <dmitry.fleytman@gmail.com>, Akihiko Odaki <
> akihiko.odaki@daynix.com>
>
>
> In virtio-net.c we assume that the IP length field in the packet is
> aligned, and we copy its address into a uint16_t* in the
> VirtioNetRscUnit struct which we then dereference later.  This isn't
> a safe assumption; it will also result in compilation failures if we
> mark the ip_header struct as QEMU_PACKED because the compiler will
> not let you take the address of an unaligned struct field.
>
> Make the ip_plen field in VirtioNetRscUnit a void*, and make all the
> places where we read or write through that pointer instead use some
> new accessor functions read_unit_ip_len() and write_unit_ip_len()
> which account for the pointer being potentially unaligned and also do
> the network-byte-order conversion we were previously using htons() to
> perform.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/virtio/virtio-net.h |  2 +-
>  hw/net/virtio-net.c            | 23 +++++++++++++++++++----
>  2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-net.h
> b/include/hw/virtio/virtio-net.h
> index 060c23c04d2..b9ea9e824e3 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -102,7 +102,7 @@ typedef struct VirtioNetRscStat {
>  /* Rsc unit general info used to checking if can coalescing */
>  typedef struct VirtioNetRscUnit {
>      void *ip;   /* ip header */
> -    uint16_t *ip_plen;      /* data len pointer in ip header field */
> +    void *ip_plen; /* pointer to unaligned uint16_t data len in ip header
> */
>      struct tcp_header *tcp; /* tcp header */
>      uint16_t tcp_hdrlen;    /* tcp header len */
>      uint16_t payload;       /* pure payload without virtio/eth/ip/tcp */
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index f2104ed364a..11cf462180d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2049,6 +2049,21 @@ static ssize_t virtio_net_do_receive(NetClientState
> *nc, const uint8_t *buf,
>      return virtio_net_receive_rcu(nc, buf, size, false);
>  }
>
> +/*
> + * Accessors to read and write the IP packet data length field. This
> + * is a potentially unaligned network-byte-order 16 bit unsigned integer
> + * pointed to by unit->ip_len.
> + */
> +static uint16_t read_unit_ip_len(VirtioNetRscUnit *unit)
> +{
> +    return ldl_be_p(unit->ip_plen);
>

shouldn't it be lduw_be_p?


> +}
> +
> +static void write_unit_ip_len(VirtioNetRscUnit *unit, uint16_t l)
> +{
> +    stl_be_p(unit->ip_plen, l);
>

shouldn't it be stw_be_p?


> +}
> +
>  static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain,
>                                           const uint8_t *buf,
>                                           VirtioNetRscUnit *unit)
> @@ -2063,7 +2078,7 @@ static void
> virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain,
>      unit->ip_plen = &ip->ip_len;
>      unit->tcp = (struct tcp_header *)(((uint8_t *)unit->ip) + ip_hdrlen);
>      unit->tcp_hdrlen = (htons(unit->tcp->th_offset_flags) & 0xF000) >> 10;
> -    unit->payload = htons(*unit->ip_plen) - ip_hdrlen - unit->tcp_hdrlen;
> +    unit->payload = read_unit_ip_len(unit) - ip_hdrlen - unit->tcp_hdrlen;
>  }
>
>  static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain,
> @@ -2082,7 +2097,7 @@ static void
> virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain,
>
>      /* There is a difference between payload length in ipv4 and v6,
>         ip header is excluded in ipv6 */
> -    unit->payload = htons(*unit->ip_plen) - unit->tcp_hdrlen;
> +    unit->payload = read_unit_ip_len(unit) - unit->tcp_hdrlen;
>  }
>
>  static size_t virtio_net_rsc_drain_seg(VirtioNetRscChain *chain,
> @@ -2231,7 +2246,7 @@ static int32_t
> virtio_net_rsc_coalesce_data(VirtioNetRscChain *chain,
>      VirtioNetRscUnit *o_unit;
>
>      o_unit = &seg->unit;
> -    o_ip_len = htons(*o_unit->ip_plen);
> +    o_ip_len = read_unit_ip_len(o_unit);
>      nseq = htonl(n_unit->tcp->th_seq);
>      oseq = htonl(o_unit->tcp->th_seq);
>
> @@ -2267,7 +2282,7 @@ coalesce:
>          o_unit->payload += n_unit->payload; /* update new data len */
>
>          /* update field in ip header */
> -        *o_unit->ip_plen = htons(o_ip_len + n_unit->payload);
> +        write_unit_ip_len(o_unit, o_ip_len + n_unit->payload);
>
>          /* Bring 'PUSH' big, the whql test guide says 'PUSH' can be
> coalesced
>             for windows guest, while this may change the behavior for linux
> --
> 2.34.1
>
>
>

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

[PATCH 2/2] net: mark struct ip_header as QEMU_PACKED

[Peter Maydell]

The ip_header is not actually guaranteed to be aligned. We attempt to
deal with this in some places such as net_checksum_calculate() by
using stw_be_p and so on to access the fields, but this is not
sufficient to be correct, because even accessing a byte member
within an unaligned struct is undefined behaviour. The clang
sanitizer will emit warnings like these if net_checksum_calculate()
is called:

     Stopping network: ../../net/checksum.c:106:9: runtime error: member access within misaligned address 0x556aad9b502e for type 'struct ip_header', which requires 4 byte alignment
    0x556aad9b502e: note: pointer points here
     34 56 08 00 45 00  01 48 a5 09 40 00 40 11  7c 8b 0a 00 02 0f 0a 00  02 02 00 44 00 43 01 34  19 56
                 ^
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../net/checksum.c:106:9 in
    ../../net/checksum.c:106:9: runtime error: load of misaligned address 0x556aad9b502e for type 'uint8_t' (aka 'unsigned char'), which requires 4 byte alignment
    0x556aad9b502e: note: pointer points here
     34 56 08 00 45 00  01 48 a5 09 40 00 40 11  7c 8b 0a 00 02 0f 0a 00  02 02 00 44 00 43 01 34  19 56
                 ^

Fix this by marking the ip_header struct as QEMU_PACKED, so that
the compiler knows that it might be unaligned and will generate
the right code for accessing fields.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/net/eth.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/eth.h b/include/net/eth.h
index 3b80b6e07f3..14c34f530fe 100644
--- a/include/net/eth.h
+++ b/include/net/eth.h
@@ -56,7 +56,7 @@ struct ip_header {
     uint8_t  ip_p;           /* protocol */
     uint16_t ip_sum;         /* checksum */
     uint32_t ip_src, ip_dst; /* source and destination address */
-};
+} QEMU_PACKED;
 
 typedef struct tcp_header {
     uint16_t th_sport;          /* source port */
-- 
2.34.1