| Message ID | 20201124092445.658647-1-mcascell@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets() | expand |
+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; ---
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,
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,
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
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)) {
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(+)