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

[Stephen Longfield]

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