Re: [PATCH] hw/net: Fix read of uninitialized memory in ftgmac100

[Cédric Le Goater]

On 1/9/23 18:50, Stephen Longfield wrote:
> Does anything more need to happen with this patch before it can be
> applied? Not sure if it had gotten lost over the holidays.

I queued it with other aspeed changes :

  https://github.com/legoater/qemu/commits/aspeed-8.0

We have some time before 8.0 is released.

Thanks,

C.


> Best,
>
> --Stephen
>
>
> On Wed, Dec 21, 2022 at 9:58 AM Stephen Longfield <slongfield@google.com> wrote:
> > On Tue, Dec 20, 2022 at 11:30 PM Cédric Le Goater <clg@kaod.org> wrote:
> > > On 12/20/22 23:14, Stephen Longfield wrote:
> > > > With the `size += 4` before the call to `crc32`, the CRC calculation
> > > > would overrun the buffer. Size is used in the while loop starting on
> > > > line 1009 to determine how much data to write back, with the last
> > > > four bytes coming from `crc_ptr`, so do need to increase it, but should
> > > > do this after the computation.
> > > >
> > > > I'm unsure why this use of uninitialized memory in the CRC doesn't
> > > > result in CRC errors, but it seems clear to me that it should not be
> > > > included in the calculation.
> > > >
> > > > Signed-off-by: Stephen Longfield <slongfield@google.com>
> > > > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > >
> > >
> > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > >
> > > I think imx_fec.c is impacted also.
> > >
> > > Thanks,
> > >
> > > C.
> >
> > Thanks for pointing that out, looks to be exactly the same. I'll send
> > out a separate patch that fixes the issue in that file.
> >
> > Best,
> >
> > --Stephen
> >
> > > > ---
> > > >    hw/net/ftgmac100.c | 4 ++--
> > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> > > > index 83ef0a783e..d3bf14be53 100644
> > > > --- a/hw/net/ftgmac100.c
> > > > +++ b/hw/net/ftgmac100.c
> > > > @@ -980,9 +980,9 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const 
> > > > uint8_t *buf,
> > > >            return size;
> > > >        }
> > > >
> > > > -    /* 4 bytes for the CRC.  */
> > > > -    size += 4;
> > > >        crc = cpu_to_be32(crc32(~0, buf, size));
> > > > +    /* Increase size by 4, loop below reads the last 4 bytes from crc_ptr. */
> > > > +    size += 4;
> > > >        crc_ptr = (uint8_t *) &crc;
> > > >
> > > >        /* Huge frames are truncated.  */
> > > > --
> > > > 2.39.0.314.g84b9a713c41-goog