[PATCH] hw/net/msf2-emac: Don't modify descriptor in-place in emac_store_desc()

[PATCH] hw/net/msf2-emac: Don't modify descriptor in-place in emac_store_desc()

[Peter Maydell]

The msf2-emac ethernet controller has functions emac_load_desc() and
emac_store_desc() which read and write the in-memory descriptor
blocks and handle conversion between guest and host endianness.

As currently written, emac_store_desc() does the endianness
conversion in-place; this means that it effectively consumes the
input EmacDesc struct, because on a big-endian host the fields will
be overwritten with the little-endian versions of their values.
Unfortunately, in all the callsites the code continues to access
fields in the EmacDesc struct after it has called emac_store_desc()
-- specifically, it looks at the d.next field.

The effect of this is that on a big-endian host networking doesn't
work because the address of the next descriptor is corrupted.

We could fix this by making the callsite avoid using the struct; but
it's more robust to have emac_store_desc() leave its input alone.

(emac_load_desc() also does an in-place conversion, but here this is
fine, because the function is supposed to be initializing the
struct.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is one of a number of issues that prevent 'make check-avocado'
working for arm targets on a big-endian host...

 hw/net/msf2-emac.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c
index 7ccd3e51427..34c1f768db0 100644
--- a/hw/net/msf2-emac.c
+++ b/hw/net/msf2-emac.c
@@ -120,12 +120,16 @@ static void emac_load_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
 
 static void emac_store_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
 {
-    /* Convert from host endianness into LE. */
-    d->pktaddr = cpu_to_le32(d->pktaddr);
-    d->pktsize = cpu_to_le32(d->pktsize);
-    d->next = cpu_to_le32(d->next);
+    EmacDesc outd;
+    /*
+     * Convert from host endianness into LE. We use a local struct because
+     * calling code may still want to look at the fields afterwards.
+     */
+    outd.pktaddr = cpu_to_le32(d->pktaddr);
+    outd.pktsize = cpu_to_le32(d->pktsize);
+    outd.next = cpu_to_le32(d->next);
 
-    address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, d, sizeof *d);
+    address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, &outd, sizeof outd);
 }
 
 static void msf2_dma_tx(MSF2EmacState *s)
-- 
2.34.1

Re: [PATCH] hw/net/msf2-emac: Don't modify descriptor in-place in emac_store_desc()

[Thomas Huth]

On 24/04/2023 17.19, Peter Maydell wrote:
> The msf2-emac ethernet controller has functions emac_load_desc() and
> emac_store_desc() which read and write the in-memory descriptor
> blocks and handle conversion between guest and host endianness.
> 
> As currently written, emac_store_desc() does the endianness
> conversion in-place; this means that it effectively consumes the
> input EmacDesc struct, because on a big-endian host the fields will
> be overwritten with the little-endian versions of their values.
> Unfortunately, in all the callsites the code continues to access
> fields in the EmacDesc struct after it has called emac_store_desc()
> -- specifically, it looks at the d.next field.
> 
> The effect of this is that on a big-endian host networking doesn't
> work because the address of the next descriptor is corrupted.
> 
> We could fix this by making the callsite avoid using the struct; but
> it's more robust to have emac_store_desc() leave its input alone.
> 
> (emac_load_desc() also does an in-place conversion, but here this is
> fine, because the function is supposed to be initializing the
> struct.)
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is one of a number of issues that prevent 'make check-avocado'
> working for arm targets on a big-endian host...
> 
>   hw/net/msf2-emac.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c
> index 7ccd3e51427..34c1f768db0 100644
> --- a/hw/net/msf2-emac.c
> +++ b/hw/net/msf2-emac.c
> @@ -120,12 +120,16 @@ static void emac_load_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
>   
>   static void emac_store_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)

You could likely also add a "const" to "EmacDesc *d" now.

>   {
> -    /* Convert from host endianness into LE. */
> -    d->pktaddr = cpu_to_le32(d->pktaddr);
> -    d->pktsize = cpu_to_le32(d->pktsize);
> -    d->next = cpu_to_le32(d->next);
> +    EmacDesc outd;
> +    /*
> +     * Convert from host endianness into LE. We use a local struct because
> +     * calling code may still want to look at the fields afterwards.
> +     */
> +    outd.pktaddr = cpu_to_le32(d->pktaddr);
> +    outd.pktsize = cpu_to_le32(d->pktsize);
> +    outd.next = cpu_to_le32(d->next);
>   
> -    address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, d, sizeof *d);
> +    address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, &outd, sizeof outd);
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] hw/net/msf2-emac: Don't modify descriptor in-place in emac_store_desc()

[Peter Maydell]

On Mon, 24 Apr 2023 at 17:27, Thomas Huth <thuth@redhat.com> wrote:
>
> On 24/04/2023 17.19, Peter Maydell wrote:
> > The msf2-emac ethernet controller has functions emac_load_desc() and
> > emac_store_desc() which read and write the in-memory descriptor
> > blocks and handle conversion between guest and host endianness.
> >
> > As currently written, emac_store_desc() does the endianness
> > conversion in-place; this means that it effectively consumes the
> > input EmacDesc struct, because on a big-endian host the fields will
> > be overwritten with the little-endian versions of their values.
> > Unfortunately, in all the callsites the code continues to access
> > fields in the EmacDesc struct after it has called emac_store_desc()
> > -- specifically, it looks at the d.next field.
> >
> > The effect of this is that on a big-endian host networking doesn't
> > work because the address of the next descriptor is corrupted.
> >
> > We could fix this by making the callsite avoid using the struct; but
> > it's more robust to have emac_store_desc() leave its input alone.
> >
> > (emac_load_desc() also does an in-place conversion, but here this is
> > fine, because the function is supposed to be initializing the
> > struct.)
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > This is one of a number of issues that prevent 'make check-avocado'
> > working for arm targets on a big-endian host...
> >
> >   hw/net/msf2-emac.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c
> > index 7ccd3e51427..34c1f768db0 100644
> > --- a/hw/net/msf2-emac.c
> > +++ b/hw/net/msf2-emac.c
> > @@ -120,12 +120,16 @@ static void emac_load_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
> >
> >   static void emac_store_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
>
> You could likely also add a "const" to "EmacDesc *d" now.

Yep; applied to target-arm.next with that change added.

-- PMM