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

[Peter Maydell]

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