Re: [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()

[Jason Wang]

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,