e1000: enhance frame fragment detection

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(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html