| Message ID | 1441383666-6590-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/09/15 18:21, Stefan Hajnoczi wrote: > From: P J P <pjp@fedoraproject.org> > > 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 ... ? > 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). Reviewed-by: Thomas Huth <thuth@redhat.com>
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 <pjp@fedoraproject.org> > > > > 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.
On Fri, Sep 04, 2015 at 05:21:06PM +0100, Stefan Hajnoczi wrote: > From: P J P <pjp@fedoraproject.org> > > 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] > > Signed-off-by: P J P <pjp@fedoraproject.org> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/net/e1000.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Thanks, applied to my net tree: https://github.com/stefanha/qemu/commits/net Stefan
Hello Stefan, > On Wednesday, 9 September 2015 2:40 PM, Stefan Hajnoczi wrote: > P J P probably chose this approach because it is the smallest > change. True. > On Wednesday, 9 September 2015 2:40 PM, Stefan Hajnoczi wrote: >> hw/net/e1000.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Thanks, applied to my net tree: > https://github.com/stefanha/qemu/commits/net Thank you so much, I appreciate it. Thank you. ---Regards -P J P http://feedmug.com
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");