e1000: enhance frame fragment detection
Subsystems (2)
PATCH 1/2 e1000: enhance frame fragment detection
From: Jesse Brandeburg <[email protected]>
Originally From: Neil Horman <[email protected]> Modified by: Jesse Brandeburg <[email protected]>
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.
---
drivers/net/e1000/e1000.h | 2 ++ drivers/net/e1000/e1000_main.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)
drivers/net/e1000/e1000.h +2 -0
@@ -326,6 +326,8 @@ struct e1000_adapter {/* for ioport free */ int bars;int need_ioport; + +bool discarding; };enum e1000_state_t {
drivers/net/e1000/e1000_main.c +11 -2
@@ -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; }
PATCH 2/2 e1000e: enhance frame fragment detection
From: Jesse Brandeburg <[email protected]>
Originally patched by Neil Horman <[email protected]>
e1000e could with a jumbo frame enabled interface, and packet split disabled, receive a packet that would overflow a single rx buffer. While in practice very hard to craft a packet that could abuse this, it is possible.
this is related to CVE-2009-4538
---
drivers/net/e1000e/e1000.h | 1 + drivers/net/e1000e/netdev.c | 25 +++++++++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-)
drivers/net/e1000e/e1000.h +1 -0
@@ -421,6 +421,7 @@ struct e1000_info {/* CRC Stripping defines */ #define FLAG2_CRC_STRIPPING (1 << 0)#define FLAG2_HAS_PHY_WAKEUP (1 << 1) +#define FLAG2_IS_DISCARDING (1 << 2)#define E1000_RX_DESC_PS (R , i) \ (&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
drivers/net/e1000e/netdev.c +21 -4
@@ -450,13 +450,23 @@ 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 (!(status &E1000_RXD_STAT_EOP ) || (length <=4 )) { +/* +* !EOP means multiple descriptors were used to store a single +* packet, if that's the case we need to toss it. In fact, we +* need 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->flags2 |=FLAG2_IS_DISCARDING ; + +if (adapter->flags2 &FLAG2_IS_DISCARDING ) {/* All receives must fit into a single buffer */ e_dbg ("Receive packet consumed multiple buffers\n" );/* recycle */ buffer_info->skb = skb; +if (status &E1000_RXD_STAT_EOP ) + adapter->flags2 &= ~FLAG2_IS_DISCARDING ; goto next_desc; }
@@ -745,10 +755,16 @@ static bool e1000_clean_rx_irq_ps(struct e1000_adapter *adapter, PCI_DMA_FROMDEVICE); buffer_info->dma =0 ; -if (!(staterr &E1000_RXD_STAT_EOP )) { +/* see !EOP comment in other rx routine */ +if (!(staterr &E1000_RXD_STAT_EOP )) + adapter->flags2 |=FLAG2_IS_DISCARDING ; + +if (adapter->flags2 &FLAG2_IS_DISCARDING ) {e_dbg ("Packet Split buffers didn't pick up the full " "packet\n" );dev_kfree_skb_irq (skb); +if (staterr &E1000_RXD_STAT_EOP ) + adapter->flags2 &= ~FLAG2_IS_DISCARDING ; goto next_desc; }
@@ -1118,6 +1134,7 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter) rx_ring->next_to_clean =0 ; rx_ring->next_to_use =0 ; + adapter->flags2 &= ~FLAG2_IS_DISCARDING ;writel (0 , adapter->hw.hw_addr + rx_ring->head);writel (0 , adapter->hw.hw_addr + rx_ring->tail);
Re: [net-2.6 PATCH 2/2] e1000e: enhance frame fragment detection
From: Jeff Kirsher <[email protected]>
Date: Tue, 19 Jan 2010 16:15:59 -0800
Quoted 8 lines
From: Jesse Brandeburg <[email protected]>
Originally patched by Neil Horman <[email protected]>
e1000e could with a jumbo frame enabled interface, and packet split disabled, receive a packet that would overflow a single rx buffer. While in practice very hard to craft a packet that could abuse this, it is possible.
Quoted 5 lines
this is related to CVE-2009-4538
Signed-off-by Jesse Brandeburg <[email protected]>CC Neil Horman <[email protected]>Signed-off-by Jeff Kirsher <[email protected]>
Applied.
From: Jeff Kirsher <[email protected]>
Date: Tue, 19 Jan 2010 16:15:38 -0800
Quoted 20 lines
From: Jesse Brandeburg <[email protected]>
Originally From: Neil Horman <[email protected]> Modified by: Jesse Brandeburg <[email protected]>
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.
Quoted 3 lines
Signed-off-by Jesse Brandeburg <[email protected]>Acked-by Neil Horman <[email protected]>Signed-off-by Jeff Kirsher <[email protected]>
Applied.