hw/net/imx_fec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Size is used at lines 1088/1188 for the loop, which reads the last 4
bytes from the crc_ptr so it does need to get increased, however it
shouldn't be increased before the buffer is passed to CRC computation,
or the crc32 function will access uninitialized memory.
This was pointed out to me by clg@kaod.org during the code review of
a similar patch to hw/net/ftgmac100.c
Change-Id: Ib0464303b191af1e28abeb2f5105eb25aadb5e9b
Signed-off-by: Stephen Longfield <slongfield@google.com>
Reviewed-by: Patrick Venture <venture@google.com>
---
hw/net/imx_fec.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 8c11b237de..c862d96593 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1068,9 +1068,9 @@ static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
return 0;
}
- /* 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. */
@@ -1164,9 +1164,9 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
return 0;
}
- /* 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;
if (shift16) {
--
2.39.0.314.g84b9a713c41-goog
On Wed, 21 Dec 2022 at 18:32, Stephen Longfield <slongfield@google.com> wrote: > > Size is used at lines 1088/1188 for the loop, which reads the last 4 > bytes from the crc_ptr so it does need to get increased, however it > shouldn't be increased before the buffer is passed to CRC computation, > or the crc32 function will access uninitialized memory. > > This was pointed out to me by clg@kaod.org during the code review of > a similar patch to hw/net/ftgmac100.c > > Change-Id: Ib0464303b191af1e28abeb2f5105eb25aadb5e9b > Signed-off-by: Stephen Longfield <slongfield@google.com> > Reviewed-by: Patrick Venture <venture@google.com> Applied to target-arm.next, thanks. (Looking at other ethernet device models we do indeed want to crc just the packet, not "packet plus 4 0 bytes" or something.) -- PMM
On 1/5/23 16:33, Peter Maydell wrote: > On Wed, 21 Dec 2022 at 18:32, Stephen Longfield <slongfield@google.com> wrote: >> >> Size is used at lines 1088/1188 for the loop, which reads the last 4 >> bytes from the crc_ptr so it does need to get increased, however it >> shouldn't be increased before the buffer is passed to CRC computation, >> or the crc32 function will access uninitialized memory. >> >> This was pointed out to me by clg@kaod.org during the code review of >> a similar patch to hw/net/ftgmac100.c >> >> Change-Id: Ib0464303b191af1e28abeb2f5105eb25aadb5e9b >> Signed-off-by: Stephen Longfield <slongfield@google.com> >> Reviewed-by: Patrick Venture <venture@google.com> > > Applied to target-arm.next, thanks. Did you take the ftgmac100 also ? > (Looking at other ethernet device models we do indeed want to crc > just the packet, not "packet plus 4 0 bytes" or something.) (There are some coverity issues in that area) C.
On Thu, 5 Jan 2023 at 16:46, Cédric Le Goater <clg@kaod.org> wrote: > > On 1/5/23 16:33, Peter Maydell wrote: > > On Wed, 21 Dec 2022 at 18:32, Stephen Longfield <slongfield@google.com> wrote: > >> > >> Size is used at lines 1088/1188 for the loop, which reads the last 4 > >> bytes from the crc_ptr so it does need to get increased, however it > >> shouldn't be increased before the buffer is passed to CRC computation, > >> or the crc32 function will access uninitialized memory. > >> > >> This was pointed out to me by clg@kaod.org during the code review of > >> a similar patch to hw/net/ftgmac100.c > >> > >> Change-Id: Ib0464303b191af1e28abeb2f5105eb25aadb5e9b > >> Signed-off-by: Stephen Longfield <slongfield@google.com> > >> Reviewed-by: Patrick Venture <venture@google.com> > > > > Applied to target-arm.next, thanks. > > Did you take the ftgmac100 also ? No, I missed that one (patches arriving over a holiday period are more likely to get lost). ftgmac100 is aspeed, can you remind me, are you handling those patches at the moment or would you rather I took it through target-arm.next ? thanks -- PMM
On 1/5/23 17:50, Peter Maydell wrote: > On Thu, 5 Jan 2023 at 16:46, Cédric Le Goater <clg@kaod.org> wrote: >> >> On 1/5/23 16:33, Peter Maydell wrote: >>> On Wed, 21 Dec 2022 at 18:32, Stephen Longfield <slongfield@google.com> wrote: >>>> >>>> Size is used at lines 1088/1188 for the loop, which reads the last 4 >>>> bytes from the crc_ptr so it does need to get increased, however it >>>> shouldn't be increased before the buffer is passed to CRC computation, >>>> or the crc32 function will access uninitialized memory. >>>> >>>> This was pointed out to me by clg@kaod.org during the code review of >>>> a similar patch to hw/net/ftgmac100.c >>>> >>>> Change-Id: Ib0464303b191af1e28abeb2f5105eb25aadb5e9b >>>> Signed-off-by: Stephen Longfield <slongfield@google.com> >>>> Reviewed-by: Patrick Venture <venture@google.com> >>> >>> Applied to target-arm.next, thanks. >> >> Did you take the ftgmac100 also ? > > No, I missed that one (patches arriving over a holiday > period are more likely to get lost). ftgmac100 is aspeed, > can you remind me, are you handling those patches at the moment > or would you rather I took it through target-arm.next ? The flow is low. I can handle them. Here is what I have gathered of interest for the 8.0 cycle [*] : ba1f0f30e6 tests/avocado/machine_aspeed.py: Update SDK images 394467a569 tests/avocado/machine_aspeed.py: Add shutdown to the SDK tests f2941822a5 tests/avocado: Test Aspeed Zephyr SDK v00.01.08 on AST1030 board beb01f33da hw/arm/aspeed_ast10x0: Add TODO comment to use Cortex-M4F 126f0870ff hw/arm/aspeed_ast10x0: Map HACE peripheral 4251463a5d hw/arm/aspeed_ast10x0: Map the secure SRAM d8ab4e2235 hw/arm/aspeed_ast10x0: Map I3C peripheral 47ae570bcc hw/arm/aspeed_ast10x0: Add various unimplemented peripherals b361c08ed3 hw/misc/aspeed_hace: Do not crash if address_space_map() failed 68e35c6359 hw/arm/aspeed: Use the IEC binary prefix definitions 83008870c8 hw/watchdog/wdt_aspeed: Log unimplemented registers as UNIMP level 7bc127b231 hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers 6b2a6a8b67 hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize' 186cd4db58 tests/avocado/machine_aspeed.py: Mask systemd services to speed up SDK boot 7e37fc7efa tests/avocado/machine_aspeed.py: update buildroot tests 0411291f96 m25p80: Add the is25wp256 SFPD table 5c6cd67647 avocado/boot_linux_console.py: Update ast2600 test 8d965c9276 hw/net: Fix read of uninitialized memory in imx_fec. e4e4a89ea3 hw/net: Fix read of uninitialized memory in ftgmac100 6b0eef46c7 aspeed: Add Supermicro X11 SPI machine type 5386d8eec8 target/arm: Allow users to set the number of VFP registers c83bd61b34 m25p80: Improve error when the backend file size does not match the device Not all are ready or reviewed yet. I also hope to get some cycles of Philippe to merge eMMC support in 8.0. C. [*] https://github.com/legoater/qemu/commits/aspeed-8.0
© 2016 - 2024 Red Hat, Inc.