[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [Qemu-devel] [PATCH] e1000: Avoid infinite loop in pro
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-stable] [Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor |
Date: |
Wed, 9 Sep 2015 10:10:07 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Mon, Sep 07, 2015 at 11:04:47AM +0200, Thomas Huth wrote:
> On 04/09/15 18:21, Stefan Hajnoczi wrote:
> > From: P J P <address@hidden>
> >
> > While processing transmit descriptors, it could lead to an infinite
> > loop if 'bytes' was to become zero; Add a check to avoid it.
> >
> > [The guest can force 'bytes' to 0 by setting the hdr_len and mss
> > descriptor fields to 0.
> > --Stefan]
>
> I wonder whether we should log an LOG_GUEST_ERROR in that case since
> this sounds like a problem in the guest ... ?
e1000.c seems to use DBGOUT(TXERR, ...) but doesn't do any
rate-limiting, which can pose a problem if a malicious guest keeps
triggering a warning message to fill up the disk on the host :(.
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 5c6bcd0..09c9e9d 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -740,7 +740,8 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
> > memmove(tp->data, tp->header, tp->hdr_len);
> > tp->size = tp->hdr_len;
> > }
> > - } while (split_size -= bytes);
> > + split_size -= bytes;
> > + } while (bytes && split_size);
> > } else if (!tp->tse && tp->cptse) {
> > // context descriptor TSE is not set, while data descriptor TSE is
> > set
> > DBGOUT(TXERR, "TCP segmentation error\n");
>
> Looks sane ... (but IMHO code would be more readable though if it would
> break out of the loop already earlier, as soon as it is clear that
> bytes == 0, so that e.g. the pci_dma_read(..., 0) is not called at all).
I agree. P J P probably chose this approach because it is the smallest
change. I'm happy enough as-is and you've posted your Reviewed-by too
so let's leave it.