diff mbox series

hw/net: Fix read of uninitialized memory in imx_fec.

Message ID 20221221183202.3788132-1-slongfield@google.com (mailing list archive)
State New, archived
Headers show
Series hw/net: Fix read of uninitialized memory in imx_fec. | expand

Commit Message

Stephen Longfield Dec. 21, 2022, 6:32 p.m. UTC 
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(-)

--
2.39.0.314.g84b9a713c41-goog

Comments

Peter Maydell Jan. 5, 2023, 3:33 p.m. UTC | #1 
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
Cédric Le Goater Jan. 5, 2023, 4:46 p.m. UTC | #2 
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.
Peter Maydell Jan. 5, 2023, 4:50 p.m. UTC | #3 
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
Cédric Le Goater Jan. 5, 2023, 5:08 p.m. UTC | #4 
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
diff mbox series

Patch

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