hw/net/smc91c111.c | 87 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 8 deletions(-)
This patchset fixes some potential array overflows in the smc91c111 ethernet device model, including the one found in https://gitlab.com/qemu-project/qemu/-/issues/2742 There are two classes of bugs: * we accept packet numbers from the guest, but we were not validating that they were in range before using them as an index into the data[][] array * we didn't sanitize the length field read from the data frame on tx before using it as an index to find the control byte at the end of the frame, so we could read off the end of the buffer This patchset fixes both of these. The datasheet is sadly silent on the h/w behaviour for these errors, so I opted to LOG_GUEST_ERROR and silently ignore the invalid operations. Patch 3 tidies up the existing code to use a constant defined in patch 2; I put it last so we can cc the first two patches to stable without having to also backport that patch. thanks -- PMM Peter Maydell (3): hw/net/smc91c111: Sanitize packet numbers hw/net/smc91c111: Sanitize packet length on tx hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers hw/net/smc91c111.c | 87 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 8 deletions(-) -- 2.43.0
Hi Peter, Jason, On 28/2/25 18:47, Peter Maydell wrote: > This patchset fixes some potential array overflows in the > smc91c111 ethernet device model, including the one found in > https://gitlab.com/qemu-project/qemu/-/issues/2742 > > There are two classes of bugs: > * we accept packet numbers from the guest, but we were not > validating that they were in range before using them as an > index into the data[][] array > * we didn't sanitize the length field read from the data > frame on tx before using it as an index to find the > control byte at the end of the frame, so we could read off > the end of the buffer > > This patchset fixes both of these. The datasheet is sadly > silent on the h/w behaviour for these errors, so I opted to > LOG_GUEST_ERROR and silently ignore the invalid operations. > > Patch 3 tidies up the existing code to use a constant defined > in patch 2; I put it last so we can cc the first two patches > to stable without having to also backport that patch. > > thanks > -- PMM > > Peter Maydell (3): > hw/net/smc91c111: Sanitize packet numbers > hw/net/smc91c111: Sanitize packet length on tx > hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers Since Jason just sent his network pull request, I'll take these patches via my hw-misc tree (with patch #2 fixed up), except if one of you object. Thanks, Phil.
On Tue, Mar 11, 2025 at 5:00 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Hi Peter, Jason, > > On 28/2/25 18:47, Peter Maydell wrote: > > This patchset fixes some potential array overflows in the > > smc91c111 ethernet device model, including the one found in > > https://gitlab.com/qemu-project/qemu/-/issues/2742 > > > > There are two classes of bugs: > > * we accept packet numbers from the guest, but we were not > > validating that they were in range before using them as an > > index into the data[][] array > > * we didn't sanitize the length field read from the data > > frame on tx before using it as an index to find the > > control byte at the end of the frame, so we could read off > > the end of the buffer > > > > This patchset fixes both of these. The datasheet is sadly > > silent on the h/w behaviour for these errors, so I opted to > > LOG_GUEST_ERROR and silently ignore the invalid operations. > > > > Patch 3 tidies up the existing code to use a constant defined > > in patch 2; I put it last so we can cc the first two patches > > to stable without having to also backport that patch. > > > > thanks > > -- PMM > > > > Peter Maydell (3): > > hw/net/smc91c111: Sanitize packet numbers > > hw/net/smc91c111: Sanitize packet length on tx > > hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers > > Since Jason just sent his network pull request, I'll take these > patches via my hw-misc tree (with patch #2 fixed up), except if > one of you object. > > Thanks, > > Phil. > Fine with me. Thanks
On Fri, 28 Feb 2025 at 17:48, Peter Maydell <peter.maydell@linaro.org> wrote: > > This patchset fixes some potential array overflows in the > smc91c111 ethernet device model, including the one found in > https://gitlab.com/qemu-project/qemu/-/issues/2742 > > There are two classes of bugs: > * we accept packet numbers from the guest, but we were not > validating that they were in range before using them as an > index into the data[][] array > * we didn't sanitize the length field read from the data > frame on tx before using it as an index to find the > control byte at the end of the frame, so we could read off > the end of the buffer > > This patchset fixes both of these. The datasheet is sadly > silent on the h/w behaviour for these errors, so I opted to > LOG_GUEST_ERROR and silently ignore the invalid operations. > > Patch 3 tidies up the existing code to use a constant defined > in patch 2; I put it last so we can cc the first two patches > to stable without having to also backport that patch. See also the other smc91c111 fuzzer fix patch: https://patchew.org/QEMU/20250228191652.1957208-1-peter.maydell@linaro.org/ (if I need to do a v2 of this series I'll put that one in too) -- PMM
Ping for review of this series (and the other smc91c111 patch I link below), please? thanks -- PMM On Fri, 28 Feb 2025 at 19:22, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 28 Feb 2025 at 17:48, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > This patchset fixes some potential array overflows in the > > smc91c111 ethernet device model, including the one found in > > https://gitlab.com/qemu-project/qemu/-/issues/2742 > > > > There are two classes of bugs: > > * we accept packet numbers from the guest, but we were not > > validating that they were in range before using them as an > > index into the data[][] array > > * we didn't sanitize the length field read from the data > > frame on tx before using it as an index to find the > > control byte at the end of the frame, so we could read off > > the end of the buffer > > > > This patchset fixes both of these. The datasheet is sadly > > silent on the h/w behaviour for these errors, so I opted to > > LOG_GUEST_ERROR and silently ignore the invalid operations. > > > > Patch 3 tidies up the existing code to use a constant defined > > in patch 2; I put it last so we can cc the first two patches > > to stable without having to also backport that patch. > > See also the other smc91c111 fuzzer fix patch: > https://patchew.org/QEMU/20250228191652.1957208-1-peter.maydell@linaro.org/ > > (if I need to do a v2 of this series I'll put that one in too)
© 2016 - 2025 Red Hat, Inc.