[PATCH] e1000: enhance frame fragment detection

[PATCH] e1000: enhance frame fragment detection

[Neil Horman]

Hey all-
	A security discussion was recently given:
http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
And a patch that I submitted awhile back was brought up.  Apparently some of
their testing revealed that they were able to force a buffer fragment in e1000
in which the trailing fragment was greater than 4 bytes.  As a result the
fragment check I introduced failed to detect the fragement and a partial invalid
frame was passed up into the network stack.  I've written this patch to correct
it.  I'm in the process of testing it now, but it makes good logical sense to
me.  Effectively it maintains a per-adapter state variable which detects a
non-EOP frame, and discards it and subsequent non-EOP frames leading up to _and_
_including_ the next positive-EOP frame (as it is by definition the last
fragment).  This should prevent any and all partial frames from entering the
network stack from e1000

Regards
Neil


 e1000.h      |    3 ++-
 e1000_main.c |   14 ++++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)


diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 2a567df..3d421ab 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -331,7 +331,8 @@ struct e1000_adapter {
 enum e1000_state_t {
 	__E1000_TESTING,
 	__E1000_RESETTING,
-	__E1000_DOWN
+	__E1000_DOWN,
+	__E1000_DISCARDING
 };
 
 extern char e1000_driver_name[];
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 7e855f9..0731779 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -3850,16 +3850,26 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
 
 		length = le16_to_cpu(rx_desc->length);
 		/* !EOP means multiple descriptors were used to store a single
-		 * packet, also make sure the frame isn't just CRC only */
-		if (unlikely(!(status & E1000_RXD_STAT_EOP) || (length <= 4))) {
+		 * packet, if thats the case we need to toss it.  In fact, we
+		 * to toss every packet with the EOP bit clear and the next
+		 * frame that _does_ have the EOP bit set, as it is by
+		 * definition only a frame fragment
+		 */
+		if (unlikely(!(status & E1000_RXD_STAT_EOP)))
+			set_bit(__E1000_DISCARDING, &adapter->flags);
+
+		if (test_bit(__E1000_DISCARDING, &adapter->flags)) {
 			/* All receives must fit into a single buffer */
 			E1000_DBG("%s: Receive packet consumed multiple"
 				  " buffers\n", netdev->name);
 			/* recycle */
 			buffer_info->skb = skb;
+			if (status & E1000_RXD_STAT_EOP)
+				clear_bit(__E1000_DISCARDING, &adapter->flags);
 			goto next_desc;
 		}
 
+
 		if (unlikely(rx_desc->errors & E1000_RXD_ERR_FRAME_ERR_MASK)) {
 			u8 last_byte = *(skb->data + length - 1);
 			if (TBI_ACCEPT(hw, status, rx_desc->errors, length,

Re: [PATCH] e1000: enhance frame fragment detection

[Jeff Kirsher]

On Mon, Dec 28, 2009 at 12:10, Neil Horman <nhorman@tuxdriver.com> wrote:
> Hey all-
>        A security discussion was recently given:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> And a patch that I submitted awhile back was brought up.  Apparently some of
> their testing revealed that they were able to force a buffer fragment in e1000
> in which the trailing fragment was greater than 4 bytes.  As a result the
> fragment check I introduced failed to detect the fragement and a partial invalid
> frame was passed up into the network stack.  I've written this patch to correct
> it.  I'm in the process of testing it now, but it makes good logical sense to
> me.  Effectively it maintains a per-adapter state variable which detects a
> non-EOP frame, and discards it and subsequent non-EOP frames leading up to _and_
> _including_ the next positive-EOP frame (as it is by definition the last
> fragment).  This should prevent any and all partial frames from entering the
> network stack from e1000
>
> Regards
> Neil
>
>
>  e1000.h      |    3 ++-
>  e1000_main.c |   14 ++++++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
>

Thanks Neil.  I have add the patch to my queue of patches.

-- 
Cheers,
Jeff

Re: [PATCH] e1000: enhance frame fragment detection

[Neil Horman]

On Mon, Dec 28, 2009 at 04:42:09PM -0800, Jeff Kirsher wrote:
> On Mon, Dec 28, 2009 at 12:10, Neil Horman <nhorman@tuxdriver.com> wrote:
> > Hey all-
> >        A security discussion was recently given:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> > And a patch that I submitted awhile back was brought up.  Apparently some of
> > their testing revealed that they were able to force a buffer fragment in e1000
> > in which the trailing fragment was greater than 4 bytes.  As a result the
> > fragment check I introduced failed to detect the fragement and a partial invalid
> > frame was passed up into the network stack.  I've written this patch to correct
> > it.  I'm in the process of testing it now, but it makes good logical sense to
> > me.  Effectively it maintains a per-adapter state variable which detects a
> > non-EOP frame, and discards it and subsequent non-EOP frames leading up to _and_
> > _including_ the next positive-EOP frame (as it is by definition the last
> > fragment).  This should prevent any and all partial frames from entering the
> > network stack from e1000
> >
> > Regards
> > Neil
> >
> >
> >  e1000.h      |    3 ++-
> >  e1000_main.c |   14 ++++++++++++--
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> >
> 
> Thanks Neil.  I have add the patch to my queue of patches.
> 
> -- 
> Cheers,
> Jeff
> 
Thanks jeff, much appreciated
Happy Holidays
Neil

Re: [PATCH] e1000: enhance frame fragment detection

[Brandeburg, Jesse]

Neil, I couple of comments below, I was just looking at the implementation 
of this for e1000e.

On Mon, 28 Dec 2009, Neil Horman wrote:

> Hey all-
> 	A security discussion was recently given:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> And a patch that I submitted awhile back was brought up.  Apparently some of
> their testing revealed that they were able to force a buffer fragment in e1000
> in which the trailing fragment was greater than 4 bytes.  As a result the
> fragment check I introduced failed to detect the fragement and a partial invalid
> frame was passed up into the network stack.  I've written this patch to correct
> it.  I'm in the process of testing it now, but it makes good logical sense to
> me.  Effectively it maintains a per-adapter state variable which detects a
> non-EOP frame, and discards it and subsequent non-EOP frames leading up to _and_
> _including_ the next positive-EOP frame (as it is by definition the last
> fragment).  This should prevent any and all partial frames from entering the
> network stack from e1000
> 
> Regards
> Neil
> 
> 
>  e1000.h      |    3 ++-
>  e1000_main.c |   14 ++++++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> index 2a567df..3d421ab 100644
> --- a/drivers/net/e1000/e1000.h
> +++ b/drivers/net/e1000/e1000.h
> @@ -331,7 +331,8 @@ struct e1000_adapter {
>  enum e1000_state_t {
>  	__E1000_TESTING,
>  	__E1000_RESETTING,
> -	__E1000_DOWN
> +	__E1000_DOWN,
> +	__E1000_DISCARDING
>  };
>  
>  extern char e1000_driver_name[];
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> index 7e855f9..0731779 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -3850,16 +3850,26 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
>  
>  		length = le16_to_cpu(rx_desc->length);
>  		/* !EOP means multiple descriptors were used to store a single
> -		 * packet, also make sure the frame isn't just CRC only */
> -		if (unlikely(!(status & E1000_RXD_STAT_EOP) || (length <= 4))) {
> +		 * packet, if thats the case we need to toss it.  In fact, we
> +		 * to toss every packet with the EOP bit clear and the next
> +		 * frame that _does_ have the EOP bit set, as it is by
> +		 * definition only a frame fragment
> +		 */
> +		if (unlikely(!(status & E1000_RXD_STAT_EOP)))
> +			set_bit(__E1000_DISCARDING, &adapter->flags);

test_bit and set_bit and clear_bit are atomic operations, isn't that quite 
a bit of overhead for something that is already being done in a guaranteed 
single context?

> +
> +		if (test_bit(__E1000_DISCARDING, &adapter->flags)) {
>  			/* All receives must fit into a single buffer */
>  			E1000_DBG("%s: Receive packet consumed multiple"
>  				  " buffers\n", netdev->name);
>  			/* recycle */
>  			buffer_info->skb = skb;
> +			if (status & E1000_RXD_STAT_EOP)
> +				clear_bit(__E1000_DISCARDING, &adapter->flags);

couldn't these simply be read/modify/write assignments (aka |=)

That would significantly avoid the extra cycles needed to implement three 
atomic ops.

------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev 

Re: [PATCH] e1000: enhance frame fragment detection

[Neil Horman]

On Tue, Jan 05, 2010 at 01:44:25PM -0800, Brandeburg, Jesse wrote:
> Neil, I couple of comments below, I was just looking at the implementation 
> of this for e1000e.
> 
> On Mon, 28 Dec 2009, Neil Horman wrote:
> 
> > Hey all-
> > 	A security discussion was recently given:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> > And a patch that I submitted awhile back was brought up.  Apparently some of
> > their testing revealed that they were able to force a buffer fragment in e1000
> > in which the trailing fragment was greater than 4 bytes.  As a result the
> > fragment check I introduced failed to detect the fragement and a partial invalid
> > frame was passed up into the network stack.  I've written this patch to correct
> > it.  I'm in the process of testing it now, but it makes good logical sense to
> > me.  Effectively it maintains a per-adapter state variable which detects a
> > non-EOP frame, and discards it and subsequent non-EOP frames leading up to _and_
> > _including_ the next positive-EOP frame (as it is by definition the last
> > fragment).  This should prevent any and all partial frames from entering the
> > network stack from e1000
> > 
> > Regards
> > Neil
> > 
> > 
> >  e1000.h      |    3 ++-
> >  e1000_main.c |   14 ++++++++++++--
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> > index 2a567df..3d421ab 100644
> > --- a/drivers/net/e1000/e1000.h
> > +++ b/drivers/net/e1000/e1000.h
> > @@ -331,7 +331,8 @@ struct e1000_adapter {
> >  enum e1000_state_t {
> >  	__E1000_TESTING,
> >  	__E1000_RESETTING,
> > -	__E1000_DOWN
> > +	__E1000_DOWN,
> > +	__E1000_DISCARDING
> >  };
> >  
> >  extern char e1000_driver_name[];
> > diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> > index 7e855f9..0731779 100644
> > --- a/drivers/net/e1000/e1000_main.c
> > +++ b/drivers/net/e1000/e1000_main.c
> > @@ -3850,16 +3850,26 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
> >  
> >  		length = le16_to_cpu(rx_desc->length);
> >  		/* !EOP means multiple descriptors were used to store a single
> > -		 * packet, also make sure the frame isn't just CRC only */
> > -		if (unlikely(!(status & E1000_RXD_STAT_EOP) || (length <= 4))) {
> > +		 * packet, if thats the case we need to toss it.  In fact, we
> > +		 * to toss every packet with the EOP bit clear and the next
> > +		 * frame that _does_ have the EOP bit set, as it is by
> > +		 * definition only a frame fragment
> > +		 */
> > +		if (unlikely(!(status & E1000_RXD_STAT_EOP)))
> > +			set_bit(__E1000_DISCARDING, &adapter->flags);
> 
> test_bit and set_bit and clear_bit are atomic operations, isn't that quite 
> a bit of overhead for something that is already being done in a guaranteed 
> single context?
> 
> > +
> > +		if (test_bit(__E1000_DISCARDING, &adapter->flags)) {
> >  			/* All receives must fit into a single buffer */
> >  			E1000_DBG("%s: Receive packet consumed multiple"
> >  				  " buffers\n", netdev->name);
> >  			/* recycle */
> >  			buffer_info->skb = skb;
> > +			if (status & E1000_RXD_STAT_EOP)
> > +				clear_bit(__E1000_DISCARDING, &adapter->flags);
> 
> couldn't these simply be read/modify/write assignments (aka |=)
> 
> That would significantly avoid the extra cycles needed to implement three 
> atomic ops.
> 
They certainly could be non-atomic assignments, but the other flags in the
adapter falgs are atomic and I dont think its safe to mix and match the
accesses, lest we get a waw race somewhere.

If you really think we need to save the save the cycles the best thing to
probably do is define a new flags field separate from adapter->flags that can be
accessed with non-atomics.

Let me know if you would prefer that, and I'll happily re-spin the patch.
Neil

Re: [PATCH] e1000: enhance frame fragment detection

[Brandeburg, Jesse]

a counter patch, without atomic ops, since we are protected by napi when 
modifying this variable.

Originally From: Neil Horman <nhorman@tuxdriver.com>
Modified by: Jesse Brandeburg <jesse.brandeburg@intel.com>

<original message>
Hey all-
	A security discussion was recently given:
http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
And a patch that I submitted awhile back was brought up.  Apparently some of
their testing revealed that they were able to force a buffer fragment in e1000
in which the trailing fragment was greater than 4 bytes.  As a result the
fragment check I introduced failed to detect the fragement and a partial
invalid frame was passed up into the network stack.  I've written this patch
to correct it.  I'm in the process of testing it now, but it makes good
logical sense to me.  Effectively it maintains a per-adapter state variable
which detects a non-EOP frame, and discards it and subsequent non-EOP frames
leading up to _and_ _including_ the next positive-EOP frame (as it is by
definition the last fragment).  This should prevent any and all partial frames
from entering the network stack from e1000.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---

 drivers/net/e1000/e1000.h      |    2 ++
 drivers/net/e1000/e1000_main.c |   13 +++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)


diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 2a567df..e8932db 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -326,6 +326,8 @@ struct e1000_adapter {
 	/* for ioport free */
 	int bars;
 	int need_ioport;
+
+	bool discarding;
 };
 
 enum e1000_state_t {
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 7e855f9..9bc9fcd 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -3850,13 +3850,22 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
 
 		length = le16_to_cpu(rx_desc->length);
 		/* !EOP means multiple descriptors were used to store a single
-		 * packet, also make sure the frame isn't just CRC only */
-		if (unlikely(!(status & E1000_RXD_STAT_EOP) || (length <= 4))) {
+		 * packet, if thats the case we need to toss it.  In fact, we
+		 * to toss every packet with the EOP bit clear and the next
+		 * frame that _does_ have the EOP bit set, as it is by
+		 * definition only a frame fragment
+		 */
+		if (unlikely(!(status & E1000_RXD_STAT_EOP)))
+			adapter->discarding = true;
+
+		if (adapter->discarding) {
 			/* All receives must fit into a single buffer */
 			E1000_DBG("%s: Receive packet consumed multiple"
 				  " buffers\n", netdev->name);
 			/* recycle */
 			buffer_info->skb = skb;
+			if (status & E1000_RXD_STAT_EOP)
+				adapter->discarding = false;
 			goto next_desc;
 		}
 

Re: [PATCH] e1000: enhance frame fragment detection

[Neil Horman]

On Wed, Jan 06, 2010 at 03:27:42PM -0800, Brandeburg, Jesse wrote:
> a counter patch, without atomic ops, since we are protected by napi when 
> modifying this variable.
> 
> Originally From: Neil Horman <nhorman@tuxdriver.com>
> Modified by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> <original message>
> Hey all-
> 	A security discussion was recently given:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> And a patch that I submitted awhile back was brought up.  Apparently some of
> their testing revealed that they were able to force a buffer fragment in e1000
> in which the trailing fragment was greater than 4 bytes.  As a result the
> fragment check I introduced failed to detect the fragement and a partial
> invalid frame was passed up into the network stack.  I've written this patch
> to correct it.  I'm in the process of testing it now, but it makes good
> logical sense to me.  Effectively it maintains a per-adapter state variable
> which detects a non-EOP frame, and discards it and subsequent non-EOP frames
> leading up to _and_ _including_ the next positive-EOP frame (as it is by
> definition the last fragment).  This should prevent any and all partial frames
> from entering the network stack from e1000.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Seems like a fine alternative to me.  Thanks!
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH] e1000: enhance frame fragment detection

[Brandeburg, Jesse]

On Wed, 6 Jan 2010, Brandeburg, Jesse wrote:
> a counter patch, without atomic ops, since we are protected by napi when 
> modifying this variable.
> 
> Originally From: Neil Horman <nhorman@tuxdriver.com>
> Modified by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> <original message>
> Hey all-
> 	A security discussion was recently given:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> And a patch that I submitted awhile back was brought up.  Apparently some of
> their testing revealed that they were able to force a buffer fragment in e1000
> in which the trailing fragment was greater than 4 bytes.  As a result the
> fragment check I introduced failed to detect the fragement and a partial
> invalid frame was passed up into the network stack.  I've written this patch
> to correct it.  I'm in the process of testing it now, but it makes good
> logical sense to me.  Effectively it maintains a per-adapter state variable
> which detects a non-EOP frame, and discards it and subsequent non-EOP frames
> leading up to _and_ _including_ the next positive-EOP frame (as it is by
> definition the last fragment).  This should prevent any and all partial frames
> from entering the network stack from e1000.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

I would like to withdraw this patch, at least for 2.6.32+ e1000 and e1000e 
are both not susceptible to this attack.  We have verified the below with 
testing, including code modifications to guarantee the correct paths were 
taken when receiving overlong frames.

What has happened is that in commit 
edbbb3ca107715067b27a71e6ea7f58750912aa2 the e1000 driver had a feature 
added to use 4kB data buffers when in jumbo mode.  This code understands 
chains of data buffers, (in fact depends on it) so even when receiving a 
packet that is longer than 4kB, the packet is handed in its entirety to 
the stack.

I believe RedHat has not backported this patch, and kernels <= 2.6.31 
still need the fix, so both need some version of this workaround, but 
2.6.32 does not.

As for e1000e, in jumbo mode it has always used what we call "packet split 
mode" in the driver, where hardware uses a special descriptor that can 
contain 4 dma fragments, a header buffer of 256 bytes and up to 3 4kB data 
buffers.  If a packet that arrives is > (12kB + 256) then it will overflow 
into the next descriptor, using only the first 4kB data buffer of the 
second descriptor (our hardware has a hard limit of 16kB for any ethernet 
frame, longer are dropped at the hardware level)

The code correctly handles the !EOP packet and drops it, and the next 
packet will hit the !length (of the header buffer) condition and also be 
dropped.

Other Intel hardware is not susceptible to this attack.  Hardware 
supported by the e100 (no jumbo frames), the ixgb driver (MFS register), 
the igb driver (RLPML register), and ixgbe (MHADD/MAXFRS register) do not 
have this issue.

Hope this clears up some things,

 Jesse

------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev 
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: enhance frame fragment detection

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On Tue, 2010-01-12 at 17:56 -0800, Brandeburg, Jesse wrote:
> On Wed, 6 Jan 2010, Brandeburg, Jesse wrote:
> > a counter patch, without atomic ops, since we are protected by napi when 
> > modifying this variable.
> > 
> > Originally From: Neil Horman <nhorman@tuxdriver.com>
> > Modified by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > 
> > <original message>
> > Hey all-
> > 	A security discussion was recently given:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> > And a patch that I submitted awhile back was brought up.  Apparently some of
> > their testing revealed that they were able to force a buffer fragment in e1000
> > in which the trailing fragment was greater than 4 bytes.  As a result the
> > fragment check I introduced failed to detect the fragement and a partial
> > invalid frame was passed up into the network stack.  I've written this patch
> > to correct it.  I'm in the process of testing it now, but it makes good
> > logical sense to me.  Effectively it maintains a per-adapter state variable
> > which detects a non-EOP frame, and discards it and subsequent non-EOP frames
> > leading up to _and_ _including_ the next positive-EOP frame (as it is by
> > definition the last fragment).  This should prevent any and all partial frames
> > from entering the network stack from e1000.
> > 
> > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> I would like to withdraw this patch, at least for 2.6.32+ e1000 and e1000e 
> are both not susceptible to this attack.  We have verified the below with 
> testing, including code modifications to guarantee the correct paths were 
> taken when receiving overlong frames.
[...]
> I believe RedHat has not backported this patch, and kernels <= 2.6.31 
> still need the fix, so both need some version of this workaround, but 
> 2.6.32 does not.
[...]

There's also the 2.6.27 stable series, and several long-term supported
distributions.  I'm particularly interested in getting a patch for
Debian 5.0's kernel based on 2.6.26.  Please advise what would be a
suitable change for the older kernel versions.

Ben.

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] e1000: enhance frame fragment detection

[Neil Horman]

On Tue, Jan 12, 2010 at 05:56:28PM -0800, Brandeburg, Jesse wrote:
> On Wed, 6 Jan 2010, Brandeburg, Jesse wrote:
> > a counter patch, without atomic ops, since we are protected by napi when 
> > modifying this variable.
> > 
> > Originally From: Neil Horman <nhorman@tuxdriver.com>
> > Modified by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > 
> > <original message>
> > Hey all-
> > 	A security discussion was recently given:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> > And a patch that I submitted awhile back was brought up.  Apparently some of
> > their testing revealed that they were able to force a buffer fragment in e1000
> > in which the trailing fragment was greater than 4 bytes.  As a result the
> > fragment check I introduced failed to detect the fragement and a partial
> > invalid frame was passed up into the network stack.  I've written this patch
> > to correct it.  I'm in the process of testing it now, but it makes good
> > logical sense to me.  Effectively it maintains a per-adapter state variable
> > which detects a non-EOP frame, and discards it and subsequent non-EOP frames
> > leading up to _and_ _including_ the next positive-EOP frame (as it is by
> > definition the last fragment).  This should prevent any and all partial frames
> > from entering the network stack from e1000.
> > 
> > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> I would like to withdraw this patch, at least for 2.6.32+ e1000 and e1000e 
> are both not susceptible to this attack.  We have verified the below with 
> testing, including code modifications to guarantee the correct paths were 
> taken when receiving overlong frames.
> 
> What has happened is that in commit 
> edbbb3ca107715067b27a71e6ea7f58750912aa2 the e1000 driver had a feature 
> added to use 4kB data buffers when in jumbo mode.  This code understands 
> chains of data buffers, (in fact depends on it) so even when receiving a 
> packet that is longer than 4kB, the packet is handed in its entirety to 
> the stack.
> 
> I believe RedHat has not backported this patch, and kernels <= 2.6.31 
> still need the fix, so both need some version of this workaround, but 
> 2.6.32 does not.
> 
> As for e1000e, in jumbo mode it has always used what we call "packet split 
> mode" in the driver, where hardware uses a special descriptor that can 
> contain 4 dma fragments, a header buffer of 256 bytes and up to 3 4kB data 
> buffers.  If a packet that arrives is > (12kB + 256) then it will overflow 
> into the next descriptor, using only the first 4kB data buffer of the 
> second descriptor (our hardware has a hard limit of 16kB for any ethernet 
> frame, longer are dropped at the hardware level)
> 
> The code correctly handles the !EOP packet and drops it, and the next 
> packet will hit the !length (of the header buffer) condition and also be 
> dropped.
> 
> Other Intel hardware is not susceptible to this attack.  Hardware 
> supported by the e100 (no jumbo frames), the ixgb driver (MFS register), 
> the igb driver (RLPML register), and ixgbe (MHADD/MAXFRS register) do not 
> have this issue.
> 
> Hope this clears up some things,
> 
I'm sorry, it doesn't clear much up, at least not for me.  The patch you're
referencing above deals only with the jumbo receive path, not the non-jumbo
case, which is not written to handle skb chains.  The vulnerability targets the
latter case specifically.  We've seen cases in which an extra data is
transferred into a subsequent buffer in the ring in that path.  Normally in our
reproducing cases, I only saw a 4 byte overrun.  Theres a check specifically in
the e1000(e) drivers for that case.  Unfortunately I never tested other cases,
but if someone sets a low mtu (say 1000 bytes), I don't see why the same issue
can't manifest as a buffer chain consisting of a 1000 byte skb followed by up to
an extra 522 byte skb.  such a condition would bypass that check and result in
admitting a garbage frame to the network stack.

Neil

Re: [PATCH] e1000: enhance frame fragment detection

[Brandeburg, Jesse]

On Tue, 12 Jan 2010, Neil Horman wrote:
> I'm sorry, it doesn't clear much up, at least not for me.  The patch you're
> referencing above deals only with the jumbo receive path, not the non-jumbo
> case, which is not written to handle skb chains.  The vulnerability targets the
> latter case specifically.  We've seen cases in which an extra data is
> transferred into a subsequent buffer in the ring in that path.  Normally in our
> reproducing cases, I only saw a 4 byte overrun.  Theres a check specifically in
> the e1000(e) drivers for that case.  Unfortunately I never tested other cases,
> but if someone sets a low mtu (say 1000 bytes), I don't see why the same issue
> can't manifest as a buffer chain consisting of a 1000 byte skb followed by up to
> an extra 522 byte skb.  such a condition would bypass that check and result in
> admitting a garbage frame to the network stack.

Hm, you're right. /me smacks head.  Thanks for your comments Neil, they 
are very useful.

Wish we had thought to test the 1000 mtu case before I replied.  In any 
case, we now have verified that the fix in this thread is good in the case 
of 1000 mtu. 

So I now withdraw my withdrawal.  

We have a couple more things to test/fix before we post the final 
version(s), I know this is priority but I also don't want to rush out an 
incomplete fix.

Current plan is Jeff K will post the official version in the next couple 
of days, for e1000 and e1000e, which isn't necessary for >=1500 mtu, but 
is apparently necessary for smaller MTU.

------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev 
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: enhance frame fragment detection

[Neil Horman]

On Tue, Jan 12, 2010 at 06:47:41PM -0800, Brandeburg, Jesse wrote:
> On Tue, 12 Jan 2010, Neil Horman wrote:
> > I'm sorry, it doesn't clear much up, at least not for me.  The patch you're
> > referencing above deals only with the jumbo receive path, not the non-jumbo
> > case, which is not written to handle skb chains.  The vulnerability targets the
> > latter case specifically.  We've seen cases in which an extra data is
> > transferred into a subsequent buffer in the ring in that path.  Normally in our
> > reproducing cases, I only saw a 4 byte overrun.  Theres a check specifically in
> > the e1000(e) drivers for that case.  Unfortunately I never tested other cases,
> > but if someone sets a low mtu (say 1000 bytes), I don't see why the same issue
> > can't manifest as a buffer chain consisting of a 1000 byte skb followed by up to
> > an extra 522 byte skb.  such a condition would bypass that check and result in
> > admitting a garbage frame to the network stack.
> 
> Hm, you're right. /me smacks head.  Thanks for your comments Neil, they 
> are very useful.
> 
I'm glad, thank you for listening.  I just couldn't reconcile what you were
saying with what the vulnerability was as it was reported.

> Wish we had thought to test the 1000 mtu case before I replied.  In any 
> case, we now have verified that the fix in this thread is good in the case 
> of 1000 mtu. 
> 
Agreed, we've done so as well here.

> So I now withdraw my withdrawal.  
> 
> We have a couple more things to test/fix before we post the final 
> version(s), I know this is priority but I also don't want to rush out an 
> incomplete fix.
> 
Don't rush, I expect distros can go with what we have currently if we need to
update later we can.

> Current plan is Jeff K will post the official version in the next couple 
> of days, for e1000 and e1000e, which isn't necessary for >=1500 mtu, but 
> is apparently necessary for smaller MTU.
> 
Copy that, thanks!
Neil


------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev 
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired