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

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

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

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
>

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