Xen devel - Apr 2013 - [PATCH V4 7/7] xen-netback: don't disconnect frontend when seeing oversize packet

Some frontend drivers are sending packets > 64 KiB in length. This length
overflows the length field in the first slot making the following slots have
an invalid length.

Turn this error back into a non-fatal error by dropping the packet. To avoid
having the following slots having fatal errors, consume all slots in the
packet.

This does not reopen the security hole in XSA-39 as if the packet as an
invalid number of slots it will still hit fatal error case.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/netback.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 00ae841..e21c15e 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -993,12 +993,22 @@ static int netbk_count_requests(struct xenvif *vif,
 
 		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
 		       sizeof(*txp));
-		if (txp->size > first->size) {
-			netdev_err(vif->dev,
-				   "Invalid tx request, slot size %u > remaining size %u\n",
-				   txp->size, first->size);
-			netbk_fatal_tx_err(vif);
-			return -EIO;
+
+		/* If the guest submitted a frame >= 64 KiB then
+		 * first->size overflowed and following slots will
+		 * appear to be larger than the frame.
+		 *
+		 * This cannot be fatal error as there are buggy
+		 * frontends that do this.
+		 *
+		 * Consume all slots and drop the packet.
+		 */
+		if (!drop_err && txp->size > first->size) {
+			if (net_ratelimit())
+				netdev_dbg(vif->dev,
+					   "Invalid tx request, slot size %u > remaining size %u\n",
+					   txp->size, first->size);
+			drop_err = -EIO;
 		}
 
 		first->size -= txp->size;
-- 
1.7.10.4

On Fri, 2013-04-12 at 15:24 +0100, Wei Liu wrote:
> Some frontend drivers are sending packets > 64 KiB in length. This
length
> overflows the length field in the first slot making the following slots
have
> an invalid length.
> 
> Turn this error back into a non-fatal error by dropping the packet. To
avoid
> having the following slots having fatal errors, consume all slots in the
> packet.
> 
> This does not reopen the security hole in XSA-39 as if the packet as an
> invalid number of slots it will still hit fatal error case.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On Fri, Apr 12, 2013 at 4:24 PM, Wei Liu <wei.liu2@citrix.com>
wrote:
> Some frontend drivers are sending packets > 64 KiB in length. This
length
> overflows the length field in the first slot making the following slots
have
> an invalid length.
>
> Turn this error back into a non-fatal error by dropping the packet. To
avoid
> having the following slots having fatal errors, consume all slots in the
> packet.
>
> This does not reopen the security hole in XSA-39 as if the packet as an
> invalid number of slots it will still hit fatal error case.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Maybe this should be tagged for stable? Maybe part of patch 6/7 as
well. I had to remove the part which was disabling the device because
of issues encountered in stable tree.

--
William

On Fri, 2013-04-12 at 18:17 +0100, William Dauchy wrote:
> On Fri, Apr 12, 2013 at 4:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > Some frontend drivers are sending packets > 64 KiB in length. This
length
> > overflows the length field in the first slot making the following
slots have
> > an invalid length.
> >
> > Turn this error back into a non-fatal error by dropping the packet. To
avoid
> > having the following slots having fatal errors, consume all slots in
the
> > packet.
> >
> > This does not reopen the security hole in XSA-39 as if the packet as
an
> > invalid number of slots it will still hit fatal error case.
> >
> > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Maybe this should be tagged for stable? Maybe part of patch 6/7 as
> well. I had to remove the part which was disabling the device because
> of issues encountered in stable tree.
AFAICT the majority of this series (as well as perhaps some of Wei''s
earlier fixes) should be candidates for any stable tree which received
the XSA-39 security fixes. Wei -- could you enumerate which patches are
required to fixup the XSA-39 regressions?

Ian.

On Mon, Apr 15, 2013 at 10:03:25AM +0100, Ian Campbell
wrote:
> On Fri, 2013-04-12 at 18:17 +0100, William Dauchy wrote:
> > On Fri, Apr 12, 2013 at 4:24 PM, Wei Liu <wei.liu2@citrix.com>
wrote:
> > > Some frontend drivers are sending packets > 64 KiB in length.
This length
> > > overflows the length field in the first slot making the following
slots have
> > > an invalid length.
> > >
> > > Turn this error back into a non-fatal error by dropping the
packet. To avoid
> > > having the following slots having fatal errors, consume all slots
in the
> > > packet.
> > >
> > > This does not reopen the security hole in XSA-39 as if the packet
as an
> > > invalid number of slots it will still hit fatal error case.
> > >
> > > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Maybe this should be tagged for stable? Maybe part of patch 6/7 as
> > well. I had to remove the part which was disabling the device because
> > of issues encountered in stable tree.
> 
> AFAICT the majority of this series (as well as perhaps some of
Wei''s
> earlier fixes) should be candidates for any stable tree which received
> the XSA-39 security fixes. Wei -- could you enumerate which patches are
> required to fixup the XSA-39 regressions?
> 
Are mechinical fixes such as removing redudent variables / changing log
messages candidates for stable? 1-4 are such kind of fixes.

5-7 are the real meat. If 1-4 are not taken to stable, they will need
small adjustment to apply, which is just trivial.


Wei.
> Ian.
>

On Mon, Apr 15, 2013 at 03:53:20PM +0100, Wei Liu wrote:
> 
> 5-7 are the real meat. If 1-4 are not taken to stable, they will need
> small adjustment to apply, which is just trivial.
> 
And just to be clear, 5 is not specific written for XSA-39 as it is a
fix for netfront and XSA-39 is fix to netback. However it should
go to stable as well.


Wei.
> 
> Wei.
> 
> > Ian.
> >

On Mon, 2013-04-15 at 15:53 +0100, Wei Liu wrote:
> On Mon, Apr 15, 2013 at 10:03:25AM +0100, Ian Campbell wrote:
> > On Fri, 2013-04-12 at 18:17 +0100, William Dauchy wrote:
> > > On Fri, Apr 12, 2013 at 4:24 PM, Wei Liu
<wei.liu2@citrix.com> wrote:
> > > > Some frontend drivers are sending packets > 64 KiB in
length. This length
> > > > overflows the length field in the first slot making the
following slots have
> > > > an invalid length.
> > > >
> > > > Turn this error back into a non-fatal error by dropping the
packet. To avoid
> > > > having the following slots having fatal errors, consume all
slots in the
> > > > packet.
> > > >
> > > > This does not reopen the security hole in XSA-39 as if the
packet as an
> > > > invalid number of slots it will still hit fatal error case.
> > > >
> > > > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > Maybe this should be tagged for stable? Maybe part of patch 6/7
as
> > > well. I had to remove the part which was disabling the device
because
> > > of issues encountered in stable tree.
> > 
> > AFAICT the majority of this series (as well as perhaps some of
Wei''s
> > earlier fixes) should be candidates for any stable tree which received
> > the XSA-39 security fixes. Wei -- could you enumerate which patches
are
> > required to fixup the XSA-39 regressions?
> > 
> 
> Are mechinical fixes such as removing redudent variables / changing log
> messages candidates for stable? 1-4 are such kind of fixes.
#4 looks like meat to me too?

My personal opinion is that it is better where possible to take
relatively simple and/or obvious precursors (renaming and such fit this
IMHO) than to risk making a mistake in the backport.
> 5-7 are the real meat. If 1-4 are not taken to stable, they will need
> small adjustment to apply, which is just trivial.
> 
> 
> Wei.
> 
> > Ian.
> >

On Mon, Apr 15, 2013 at 04:24:49PM +0100, Ian Campbell
wrote:
> On Mon, 2013-04-15 at 15:53 +0100, Wei Liu wrote:
> > On Mon, Apr 15, 2013 at 10:03:25AM +0100, Ian Campbell wrote:
> > > On Fri, 2013-04-12 at 18:17 +0100, William Dauchy wrote:
> > > > On Fri, Apr 12, 2013 at 4:24 PM, Wei Liu
<wei.liu2@citrix.com> wrote:
> > > > > Some frontend drivers are sending packets > 64 KiB
in length. This length
> > > > > overflows the length field in the first slot making the
following slots have
> > > > > an invalid length.
> > > > >
> > > > > Turn this error back into a non-fatal error by dropping
the packet. To avoid
> > > > > having the following slots having fatal errors, consume
all slots in the
> > > > > packet.
> > > > >
> > > > > This does not reopen the security hole in XSA-39 as if
the packet as an
> > > > > invalid number of slots it will still hit fatal error
case.
> > > > >
> > > > > Signed-off-by: David Vrabel
<david.vrabel@citrix.com>
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > 
> > > > Maybe this should be tagged for stable? Maybe part of patch
6/7 as
> > > > well. I had to remove the part which was disabling the
device because
> > > > of issues encountered in stable tree.
> > > 
> > > AFAICT the majority of this series (as well as perhaps some of
Wei''s
> > > earlier fixes) should be candidates for any stable tree which
received
> > > the XSA-39 security fixes. Wei -- could you enumerate which
patches are
> > > required to fixup the XSA-39 regressions?
> > > 
> > 
> > Are mechinical fixes such as removing redudent variables / changing
log
> > messages candidates for stable? 1-4 are such kind of fixes.
> 
> #4 looks like meat to me too?
> 
Not really. DaveM applied first 3 and later I discoverd there was a log
message which should have been changed in 2. So I wrote a incremental
patch on top of 2 to fix that.
> My personal opinion is that it is better where possible to take
> relatively simple and/or obvious precursors (renaming and such fit this
> IMHO) than to risk making a mistake in the backport.
> 
OK, then the whole series is candidate for stable.


Wei.

> > > Are mechinical fixes such as removing redudent variables /
changing log
> > > messages candidates for stable? 1-4 are such kind of fixes.
> > 
> > #4 looks like meat to me too?
> > 
> 
> Not really. DaveM applied first 3 and later I discoverd there was a log
> message which should have been changed in 2. So I wrote a incremental
> patch on top of 2 to fix that.
Oops, I think I read "V4 7/7" as 4/7...
> 
> > My personal opinion is that it is better where possible to take
> > relatively simple and/or obvious precursors (renaming and such fit
this
> > IMHO) than to risk making a mistake in the backport.
> > 
> 
> OK, then the whole series is candidate for stable.
Once things go in can you enumerate the actual commit numbers please?

Ian.

On Mon, Apr 15, 2013 at 04:35:04PM +0100, Ian Campbell
wrote:
> 
> > > > Are mechinical fixes such as removing redudent variables /
changing log
> > > > messages candidates for stable? 1-4 are such kind of fixes.
> > > 
> > > #4 looks like meat to me too?
> > > 
> > 
> > Not really. DaveM applied first 3 and later I discoverd there was a
log
> > message which should have been changed in 2. So I wrote a incremental
> > patch on top of 2 to fix that.
> 
> Oops, I think I read "V4 7/7" as 4/7...
> 
> > 
> > > My personal opinion is that it is better where possible to take
> > > relatively simple and/or obvious precursors (renaming and such
fit this
> > > IMHO) than to risk making a mistake in the backport.
> > > 
> > 
> > OK, then the whole series is candidate for stable.
> 
> Once things go in can you enumerate the actual commit numbers please?
> 
Sure.


Wei.
> Ian.
>

From: Wei Liu <wei.liu2@citrix.com>
Date: Mon, 15 Apr 2013 16:30:38 +0100
> On Mon, Apr 15, 2013 at 04:24:49PM +0100, Ian Campbell wrote:
>> On Mon, 2013-04-15 at 15:53 +0100, Wei Liu wrote:
>> > On Mon, Apr 15, 2013 at 10:03:25AM +0100, Ian Campbell wrote:
>> > > On Fri, 2013-04-12 at 18:17 +0100, William Dauchy wrote:
>> > > > On Fri, Apr 12, 2013 at 4:24 PM, Wei Liu
<wei.liu2@citrix.com> wrote:
>> > > > > Some frontend drivers are sending packets > 64
KiB in length. This length
>> > > > > overflows the length field in the first slot making
the following slots have
>> > > > > an invalid length.
>> > > > >
>> > > > > Turn this error back into a non-fatal error by
dropping the packet. To avoid
>> > > > > having the following slots having fatal errors,
consume all slots in the
>> > > > > packet.
>> > > > >
>> > > > > This does not reopen the security hole in XSA-39 as
if the packet as an
>> > > > > invalid number of slots it will still hit fatal
error case.
>> > > > >
>> > > > > Signed-off-by: David Vrabel
<david.vrabel@citrix.com>
>> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> > > > 
>> > > > Maybe this should be tagged for stable? Maybe part of
patch 6/7 as
>> > > > well. I had to remove the part which was disabling the
device because
>> > > > of issues encountered in stable tree.
>> > > 
>> > > AFAICT the majority of this series (as well as perhaps some
of Wei''s
>> > > earlier fixes) should be candidates for any stable tree which
received
>> > > the XSA-39 security fixes. Wei -- could you enumerate which
patches are
>> > > required to fixup the XSA-39 regressions?
>> > > 
>> > 
>> > Are mechinical fixes such as removing redudent variables /
changing log
>> > messages candidates for stable? 1-4 are such kind of fixes.
>> 
>> #4 looks like meat to me too?
>> 
> 
> Not really. DaveM applied first 3 and later I discoverd there was a log
> message which should have been changed in 2. So I wrote a incremental
> patch on top of 2 to fix that.
Please don''t do this, it is so confusing.

When I''ve applied some of your patches, don''t repost them,
only post the
new patches which are not yet in my tree.

All development and patches occurs relative to my tree, so by reposting
patches already applied you make for confusion and more work on my part.