* [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()
@ 2020-11-24 9:24 Mauro Matteo Cascella
2020-11-30 10:44 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 5+ messages in thread
From: Mauro Matteo Cascella @ 2020-11-24 9:24 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, pgn, mcascell
An integer underflow could occur during packet transmission due to 'tx_len' not
being updated if SONIC_TFC register is set to zero. Check for negative 'tx_len'
when removing existing FCS.
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1899722
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
Reported-by: Gaoning Pan <pgn@zju.edu.cn>
---
hw/net/dp8393x.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 674b04b354..205c0decc5 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -495,6 +495,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
} else {
/* Remove existing FCS */
tx_len -= 4;
+ if (tx_len < 0) {
+ SONIC_ERROR("tx_len is %d\n", tx_len);
+ break;
+ }
}
if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()
2020-11-24 9:24 [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets() Mauro Matteo Cascella
@ 2020-11-30 10:44 ` Philippe Mathieu-Daudé
2020-11-30 12:11 ` Mauro Matteo Cascella
0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-30 10:44 UTC (permalink / raw)
To: Mauro Matteo Cascella, qemu-devel
Cc: jasowang, pgn, Laurent Vivier, Finn Thain
+Laurent/Finn
On 11/24/20 10:24 AM, Mauro Matteo Cascella wrote:
> An integer underflow could occur during packet transmission due to 'tx_len' not
> being updated if SONIC_TFC register is set to zero. Check for negative 'tx_len'
> when removing existing FCS.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1899722
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> ---
> hw/net/dp8393x.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 674b04b354..205c0decc5 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -495,6 +495,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> } else {
> /* Remove existing FCS */
> tx_len -= 4;
> + if (tx_len < 0) {
> + SONIC_ERROR("tx_len is %d\n", tx_len);
> + break;
> + }
> }
>
> if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
>
Doesn't it make more sense to check 'tx_len >= 4'
and skip tx/rx when 'tx_len == 0'?
-- >8 --
@@ -488,25 +488,29 @@ static void
dp8393x_do_transmit_packets(dp8393xState *s)
}
}
- /* Handle Ethernet checksum */
- if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
- /* Don't append FCS there, to look like slirp packets
- * which don't have one */
- } else {
- /* Remove existing FCS */
- tx_len -= 4;
+ if (tx_len >= 4) {
+ /* Handle Ethernet checksum */
+ if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
+ /* Don't append FCS there, to look like slirp packets
+ * which don't have one */
+ } else {
+ /* Remove existing FCS */
+ tx_len -= 4;
+ }
}
- if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
- /* Loopback */
- s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
- if (nc->info->can_receive(nc)) {
- s->loopback_packet = 1;
- nc->info->receive(nc, s->tx_buffer, tx_len);
+ if (tx_len > 0) {
+ if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
+ /* Loopback */
+ s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
+ if (nc->info->can_receive(nc)) {
+ s->loopback_packet = 1;
+ nc->info->receive(nc, s->tx_buffer, tx_len);
+ }
+ } else {
+ /* Transmit packet */
+ qemu_send_packet(nc, s->tx_buffer, tx_len);
}
- } else {
- /* Transmit packet */
- qemu_send_packet(nc, s->tx_buffer, tx_len);
}
s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
---
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()
2020-11-30 10:44 ` Philippe Mathieu-Daudé
@ 2020-11-30 12:11 ` Mauro Matteo Cascella
2020-12-01 5:46 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Mauro Matteo Cascella @ 2020-11-30 12:11 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Jason Wang, pgn, QEMU Developers, Finn Thain, Laurent Vivier
On Mon, Nov 30, 2020 at 11:44 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Laurent/Finn
>
> On 11/24/20 10:24 AM, Mauro Matteo Cascella wrote:
> > An integer underflow could occur during packet transmission due to 'tx_len' not
> > being updated if SONIC_TFC register is set to zero. Check for negative 'tx_len'
> > when removing existing FCS.
> >
> > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1899722
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> > ---
> > hw/net/dp8393x.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 674b04b354..205c0decc5 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -495,6 +495,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> > } else {
> > /* Remove existing FCS */
> > tx_len -= 4;
> > + if (tx_len < 0) {
> > + SONIC_ERROR("tx_len is %d\n", tx_len);
> > + break;
> > + }
> > }
> >
> > if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
> >
>
> Doesn't it make more sense to check 'tx_len >= 4'
> and skip tx/rx when 'tx_len == 0'?
>
Yes, it makes sense. I thought that skipping tx/rx in case of null
'tx_len' could lead to potential inconsistencies when writing the
status or reading the footer of the packet. but I'm not really sure. I
can send a new version of the patch if needed, otherwise feel free to
apply your changes. Thank you.
> -- >8 --
> @@ -488,25 +488,29 @@ static void
> dp8393x_do_transmit_packets(dp8393xState *s)
> }
> }
>
> - /* Handle Ethernet checksum */
> - if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
> - /* Don't append FCS there, to look like slirp packets
> - * which don't have one */
> - } else {
> - /* Remove existing FCS */
> - tx_len -= 4;
> + if (tx_len >= 4) {
> + /* Handle Ethernet checksum */
> + if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
> + /* Don't append FCS there, to look like slirp packets
> + * which don't have one */
> + } else {
> + /* Remove existing FCS */
> + tx_len -= 4;
> + }
> }
>
> - if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
> - /* Loopback */
> - s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
> - if (nc->info->can_receive(nc)) {
> - s->loopback_packet = 1;
> - nc->info->receive(nc, s->tx_buffer, tx_len);
> + if (tx_len > 0) {
> + if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
> + /* Loopback */
> + s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
> + if (nc->info->can_receive(nc)) {
> + s->loopback_packet = 1;
> + nc->info->receive(nc, s->tx_buffer, tx_len);
> + }
> + } else {
> + /* Transmit packet */
> + qemu_send_packet(nc, s->tx_buffer, tx_len);
> }
> - } else {
> - /* Transmit packet */
> - qemu_send_packet(nc, s->tx_buffer, tx_len);
> }
> s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
>
> ---
>
Regards,
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()
2020-11-30 12:11 ` Mauro Matteo Cascella
@ 2020-12-01 5:46 ` Jason Wang
2020-12-01 12:09 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2020-12-01 5:46 UTC (permalink / raw)
To: Mauro Matteo Cascella, Philippe Mathieu-Daudé, Peter Maydell
Cc: pgn, QEMU Developers, Finn Thain, Laurent Vivier
On 2020/11/30 下午8:11, Mauro Matteo Cascella wrote:
> On Mon, Nov 30, 2020 at 11:44 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> +Laurent/Finn
>>
>> On 11/24/20 10:24 AM, Mauro Matteo Cascella wrote:
>>> An integer underflow could occur during packet transmission due to 'tx_len' not
>>> being updated if SONIC_TFC register is set to zero. Check for negative 'tx_len'
>>> when removing existing FCS.
>>>
>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1899722
>>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
>>> ---
>>> hw/net/dp8393x.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 674b04b354..205c0decc5 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -495,6 +495,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>>> } else {
>>> /* Remove existing FCS */
>>> tx_len -= 4;
>>> + if (tx_len < 0) {
>>> + SONIC_ERROR("tx_len is %d\n", tx_len);
>>> + break;
>>> + }
>>> }
>>>
>>> if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
>>>
>> Doesn't it make more sense to check 'tx_len >= 4'
>> and skip tx/rx when 'tx_len == 0'?
>>
> Yes, it makes sense. I thought that skipping tx/rx in case of null
> 'tx_len' could lead to potential inconsistencies when writing the
> status or reading the footer of the packet. but I'm not really sure. I
> can send a new version of the patch if needed, otherwise feel free to
> apply your changes. Thank you.
I think we can go with this patch first and tweak on top consider it's
near the release. So:
Acked-by: Jason Wang <jasowang@redhat.com>
Peter, do you want to merge this patch?
Thanks
>
>> -- >8 --
>> @@ -488,25 +488,29 @@ static void
>> dp8393x_do_transmit_packets(dp8393xState *s)
>> }
>> }
>>
>> - /* Handle Ethernet checksum */
>> - if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
>> - /* Don't append FCS there, to look like slirp packets
>> - * which don't have one */
>> - } else {
>> - /* Remove existing FCS */
>> - tx_len -= 4;
>> + if (tx_len >= 4) {
>> + /* Handle Ethernet checksum */
>> + if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
>> + /* Don't append FCS there, to look like slirp packets
>> + * which don't have one */
>> + } else {
>> + /* Remove existing FCS */
>> + tx_len -= 4;
>> + }
>> }
>>
>> - if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
>> - /* Loopback */
>> - s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
>> - if (nc->info->can_receive(nc)) {
>> - s->loopback_packet = 1;
>> - nc->info->receive(nc, s->tx_buffer, tx_len);
>> + if (tx_len > 0) {
>> + if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
>> + /* Loopback */
>> + s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
>> + if (nc->info->can_receive(nc)) {
>> + s->loopback_packet = 1;
>> + nc->info->receive(nc, s->tx_buffer, tx_len);
>> + }
>> + } else {
>> + /* Transmit packet */
>> + qemu_send_packet(nc, s->tx_buffer, tx_len);
>> }
>> - } else {
>> - /* Transmit packet */
>> - qemu_send_packet(nc, s->tx_buffer, tx_len);
>> }
>> s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
>>
>> ---
>>
> Regards,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()
2020-12-01 5:46 ` Jason Wang
@ 2020-12-01 12:09 ` Peter Maydell
0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2020-12-01 12:09 UTC (permalink / raw)
To: Jason Wang
Cc: Mauro Matteo Cascella, QEMU Developers, Laurent Vivier,
Finn Thain, Philippe Mathieu-Daudé,
Gaoning Pan
On Tue, 1 Dec 2020 at 05:46, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/30 下午8:11, Mauro Matteo Cascella wrote:
> > On Mon, Nov 30, 2020 at 11:44 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >> +Laurent/Finn
> >>
> >> On 11/24/20 10:24 AM, Mauro Matteo Cascella wrote:
> >>> An integer underflow could occur during packet transmission due to 'tx_len' not
> >>> being updated if SONIC_TFC register is set to zero. Check for negative 'tx_len'
> >>> when removing existing FCS.
> >>>
> >>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1899722
> >>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> >>> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> >>> ---
> >>> hw/net/dp8393x.c | 4 ++++
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> >>> index 674b04b354..205c0decc5 100644
> >>> --- a/hw/net/dp8393x.c
> >>> +++ b/hw/net/dp8393x.c
> >>> @@ -495,6 +495,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> >>> } else {
> >>> /* Remove existing FCS */
> >>> tx_len -= 4;
> >>> + if (tx_len < 0) {
> >>> + SONIC_ERROR("tx_len is %d\n", tx_len);
> >>> + break;
> >>> + }
> >>> }
> >>>
> >>> if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
> >>>
> >> Doesn't it make more sense to check 'tx_len >= 4'
> >> and skip tx/rx when 'tx_len == 0'?
> >>
> > Yes, it makes sense. I thought that skipping tx/rx in case of null
> > 'tx_len' could lead to potential inconsistencies when writing the
> > status or reading the footer of the packet. but I'm not really sure. I
> > can send a new version of the patch if needed, otherwise feel free to
> > apply your changes. Thank you.
>
>
> I think we can go with this patch first and tweak on top consider it's
> near the release. So:
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Peter, do you want to merge this patch?
rc4 is due for release today, and the dp8393x is a device not
used by any KVM platform, so this isn't a security fix. So
we don't *need* to take it for 5.2. On the other hand this is a
pretty small and constrained fix that won't affect anything
except the mips jazz and m68k q800 boards.
Applied to master for 5.2, thanks.
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-01 12:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 9:24 [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets() Mauro Matteo Cascella
2020-11-30 10:44 ` Philippe Mathieu-Daudé
2020-11-30 12:11 ` Mauro Matteo Cascella
2020-12-01 5:46 ` Jason Wang
2020-12-01 12:09 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).