| Message ID | 20221220221437.3303721-1-slongfield@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net: Fix read of uninitialized memory in ftgmac100 | expand |
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. > --- > 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 >
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 > > >
Does anything more need to happen with this patch before it can be applied? Not sure if it had gotten lost over the holidays. 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 > > > > >
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 >>>> >>>
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. */