[PATCH RFC] r8169: straighten out overlength frame detection

[PATCH RFC] r8169: straighten out overlength frame detection

[Neil Horman]

Hey all-
	A while back Eric submitted a patch for r8169 in which the proper
allocated frame size was written to RXMaxSize to prevent the NIC from dmaing too
much data.  This was done in commit fdd7b4c3302c93f6833e338903ea77245eb510b4.  A
long time prior to that however, Francois posted
126fa4b9ca5d9d7cb7d46f779ad3bd3631ca387c, which expiclitly disabled the MaxSize
setting due to the fact that the hardware behaved in odd ways when overlong
frames were received on NIC's supported by this driver.  This was mentioned in a
security conference recently:
http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html

It seems that if we can't enable frame size filtering, then, as Eric correctly
noticed, we can find ourselves DMA-ing too much data to a buffer, causing
corruption.  As a result is seems that we are forced to allocate a frame which
is ready to handle a maximally sized receive.

I've not tested the below patch at all, and clearly it stinks to have to do.
But I thought it would be worth posting to solicit comments on it.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 r8169.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)


diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 60f96c4..42e3b22 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -63,6 +63,7 @@ static const int multicast_filter_limit = 32;
 #define RX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
 #define TX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
 #define EarlyTxThld	0x3F	/* 0x3F means NO early transmit */
+#define RxPacketMaxSize	0x3FE8	/* 16K - 1 - ETH_HLEN - VLAN - CRC... */
 #define SafeMtu		0x1c20	/* ... actually life sucks beyond ~7k */
 #define InterFrameGap	0x03	/* 3 means InterFrameGap = the shortest one */
 
@@ -3383,7 +3384,7 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr)
 static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz)
 {
 	/* Low hurts. Let's disable the filtering. */
-	RTL_W16(RxMaxSize, rx_buf_sz + 1);
+	RTL_W16(RxMaxSize, 16383);
 }
 
 static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version)
@@ -3430,7 +3431,7 @@ static void rtl_hw_start_8169(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr);
 
 	if ((tp->mac_version == RTL_GIGA_MAC_VER_01) ||
 	    (tp->mac_version == RTL_GIGA_MAC_VER_02) ||
@@ -3691,7 +3692,7 @@ static void rtl_hw_start_8168(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr);
 
 	tp->cp_cmd |= RTL_R16(CPlusCmd) | PktCntrDisable | INTT_1;
 
@@ -3871,7 +3872,7 @@ static void rtl_hw_start_8101(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr);
 
 	tp->cp_cmd |= rtl_rw_cpluscmd(ioaddr) | PCIMulRW;
 
@@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
 
 	pad = align ? align : NET_IP_ALIGN;
 
-	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
+	skb = netdev_alloc_skb(dev, 16383 + pad);
 	if (!skb)
 		goto err_out;
 

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Neil Horman]

Apologies, Reposting, I had meant to cc the usual suspects, and completely
forgot

Neil


On Mon, Dec 28, 2009 at 02:48:34PM -0500, Neil Horman wrote:
> Hey all-
> 	A while back Eric submitted a patch for r8169 in which the proper
> allocated frame size was written to RXMaxSize to prevent the NIC from dmaing too
> much data.  This was done in commit fdd7b4c3302c93f6833e338903ea77245eb510b4.  A
> long time prior to that however, Francois posted
> 126fa4b9ca5d9d7cb7d46f779ad3bd3631ca387c, which expiclitly disabled the MaxSize
> setting due to the fact that the hardware behaved in odd ways when overlong
> frames were received on NIC's supported by this driver.  This was mentioned in a
> security conference recently:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> 
> It seems that if we can't enable frame size filtering, then, as Eric correctly
> noticed, we can find ourselves DMA-ing too much data to a buffer, causing
> corruption.  As a result is seems that we are forced to allocate a frame which
> is ready to handle a maximally sized receive.
> 
> I've not tested the below patch at all, and clearly it stinks to have to do.
> But I thought it would be worth posting to solicit comments on it.
> 
> Thanks & Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  r8169.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 60f96c4..42e3b22 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -63,6 +63,7 @@ static const int multicast_filter_limit = 32;
>  #define RX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
>  #define TX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
>  #define EarlyTxThld	0x3F	/* 0x3F means NO early transmit */
> +#define RxPacketMaxSize	0x3FE8	/* 16K - 1 - ETH_HLEN - VLAN - CRC... */
>  #define SafeMtu		0x1c20	/* ... actually life sucks beyond ~7k */
>  #define InterFrameGap	0x03	/* 3 means InterFrameGap = the shortest one */
>  
> @@ -3383,7 +3384,7 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr)
>  static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz)
>  {
>  	/* Low hurts. Let's disable the filtering. */
> -	RTL_W16(RxMaxSize, rx_buf_sz + 1);
> +	RTL_W16(RxMaxSize, 16383);
>  }
>  
>  static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version)
> @@ -3430,7 +3431,7 @@ static void rtl_hw_start_8169(struct net_device *dev)
>  
>  	RTL_W8(EarlyTxThres, EarlyTxThld);
>  
> -	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
> +	rtl_set_rx_max_size(ioaddr);
>  
>  	if ((tp->mac_version == RTL_GIGA_MAC_VER_01) ||
>  	    (tp->mac_version == RTL_GIGA_MAC_VER_02) ||
> @@ -3691,7 +3692,7 @@ static void rtl_hw_start_8168(struct net_device *dev)
>  
>  	RTL_W8(EarlyTxThres, EarlyTxThld);
>  
> -	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
> +	rtl_set_rx_max_size(ioaddr);
>  
>  	tp->cp_cmd |= RTL_R16(CPlusCmd) | PktCntrDisable | INTT_1;
>  
> @@ -3871,7 +3872,7 @@ static void rtl_hw_start_8101(struct net_device *dev)
>  
>  	RTL_W8(EarlyTxThres, EarlyTxThld);
>  
> -	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
> +	rtl_set_rx_max_size(ioaddr);
>  
>  	tp->cp_cmd |= rtl_rw_cpluscmd(ioaddr) | PCIMulRW;
>  
> @@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
>  
>  	pad = align ? align : NET_IP_ALIGN;
>  
> -	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
> +	skb = netdev_alloc_skb(dev, 16383 + pad);
>  	if (!skb)
>  		goto err_out;
>  
> --
> 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
> 

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[François romieu]

(I'm back)

The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
[...]
> frames were received on NIC's supported by this driver.  This was mentioned in a
> security conference recently:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html

Is there a paper ?

> It seems that if we can't enable frame size filtering, then, as Eric correctly
> noticed, we can find ourselves DMA-ing too much data to a buffer, causing
> corruption.  As a result is seems that we are forced to allocate a frame which
> is ready to handle a maximally sized receive.

Either that or the switch does not allow jumbo frames.

> I've not tested the below patch at all, and clearly it stinks to have to do.
> But I thought it would be worth posting to solicit comments on it.
[...]
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 60f96c4..42e3b22 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
>  
>  	pad = align ? align : NET_IP_ALIGN;
>  
> -	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
> +	skb = netdev_alloc_skb(dev, 16383 + pad);

I doubt that we will be able to allocate that much memory reliably for long.

I'd rather go for static buffers + copy (+ src mac address of our new friend).

Is it enough if I write it in a pair of evening ?

-- 
Ueimor

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Neil Horman]

On Mon, Dec 28, 2009 at 10:31:14PM +0100, François romieu wrote:
> (I'm back)
> 
> The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> [...]
> > frames were received on NIC's supported by this driver.  This was mentioned in a
> > security conference recently:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> 
> Is there a paper ?
> 
> > It seems that if we can't enable frame size filtering, then, as Eric correctly
> > noticed, we can find ourselves DMA-ing too much data to a buffer, causing
> > corruption.  As a result is seems that we are forced to allocate a frame which
> > is ready to handle a maximally sized receive.
> 
> Either that or the switch does not allow jumbo frames.
> 
Possible, but regardless, if the result is that we either dma beyond the end of
an allocated skb, or get garbage in the frame size register, it seems we need to
support your initially referenced commit.  Again, I'm not 100% sure here, I'm
soliciting opinion, based on the above presentation.

> > I've not tested the below patch at all, and clearly it stinks to have to do.
> > But I thought it would be worth posting to solicit comments on it.
> [...]
> > diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> > index 60f96c4..42e3b22 100644
> > --- a/drivers/net/r8169.c
> > +++ b/drivers/net/r8169.c
> > @@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
> >  
> >  	pad = align ? align : NET_IP_ALIGN;
> >  
> > -	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
> > +	skb = netdev_alloc_skb(dev, 16383 + pad);
> 
> I doubt that we will be able to allocate that much memory reliably for long.
> 
I agree, thats why I'm posting this with RFC, because it seems like a terrible
solution.  But at the same time, if we can't rely on the NIC to report frame
sizes reliably, I'm not sure what other choice we have.

> I'd rather go for static buffers + copy (+ src mac address of our new friend).
> 
Thats another choice, yes.  We could probably attain that easily by setting the
copybreak value to 16383 (so as to avoid changing the receive path too much if
there are classes of supported hw that do not encounter this problem).

> Is it enough if I write it in a pair of evening ?
> 
Yeah, thats fine by me, I was just responding to the presentation so we could
get the ball rolling.  If you like, I'd be happy to do it as well.  Just let me
know.


Thanks & regards
Neil

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[David Dillow]

On Mon, 2009-12-28 at 22:31 +0100, François romieu wrote:
> (I'm back)
> 
> The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> [...]
> I doubt that we will be able to allocate that much memory reliably for long.
> 
> I'd rather go for static buffers + copy (+ src mac address of our new friend).

The driver doesn't support fragmented receives, but it seems like the HW
does -- is this known to work on the different revisions? Or
alternatively, is it known to be broken on any of them? It seems like it
would be preferable to implement this and use RxMaxSize to tell the NIC
how big the allocated buffer fragments are.

Or I'm misreading the capabilities of the NIC?

In any event, I'm probably not going to have time to write/test it on
the HW I have anytime soon, so perhaps the static buffers/copy is the
safest option for now, albeit non-optimal.

Dave

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Ben Hutchings]

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

On Mon, 2009-12-28 at 22:31 +0100, François romieu wrote:
> (I'm back)
> 
> The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> [...]
> > frames were received on NIC's supported by this driver.  This was mentioned in a
> > security conference recently:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> 
> Is there a paper ?
[...]

I was present in the session and emailed you and Neil about this
yesterday; did you not get the message?  The slides are now linked from
the above page; see pages 74-87.  I suggest you ask the speaker if you
need any more details.

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Neil Horman]

On Tue, Dec 29, 2009 at 12:51:52AM +0000, Ben Hutchings wrote:
> On Mon, 2009-12-28 at 22:31 +0100, François romieu wrote:
> > (I'm back)
> > 
> > The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> > [...]
> > > frames were received on NIC's supported by this driver.  This was mentioned in a
> > > security conference recently:
> > > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> > 
> > Is there a paper ?
> [...]
> 
> I was present in the session and emailed you and Neil about this
> yesterday; did you not get the message?  The slides are now linked from
> the above page; see pages 74-87.  I suggest you ask the speaker if you
> need any more details.
> 
> Ben.
> 
I'm sorry, Ben, I don't think I received this note (although I admit I've not
been checking to closely over the holidays).  I jumped on this after I got a
phone call about it today.  I'll make sure to look for your note shortly.
Neil

> -- 
> Ben Hutchings
> Always try to do things in chronological order;
> it's less confusing that way.

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Neil Horman]

On Mon, Dec 28, 2009 at 07:24:51PM -0500, David Dillow wrote:
> On Mon, 2009-12-28 at 22:31 +0100, François romieu wrote:
> > (I'm back)
> > 
> > The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> > [...]
> > I doubt that we will be able to allocate that much memory reliably for long.
> > 
> > I'd rather go for static buffers + copy (+ src mac address of our new friend).
> 
> The driver doesn't support fragmented receives, but it seems like the HW
> does -- is this known to work on the different revisions? Or
> alternatively, is it known to be broken on any of them? It seems like it
> would be preferable to implement this and use RxMaxSize to tell the NIC
> how big the allocated buffer fragments are.
> 
> Or I'm misreading the capabilities of the NIC?
> 
> In any event, I'm probably not going to have time to write/test it on
> the HW I have anytime soon, so perhaps the static buffers/copy is the
> safest option for now, albeit non-optimal.
> 
Agreed, very non-optimal, but the only safe solution, if the rx size register
can't be trusted.  

What about this solution?  Its the same as before, but it takes Francois'
thoughts into account.  It ups the default copybreak value to 16383 so that all
the recevied frames should succede in try_rx_copy.  Again, completely untested,
but it compiles.  If theres some way to identify hardware subclasses that are
affected by this issues, you can restrict the affect by chaning the RxMaxSize
and copybreak in the probe routine.

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 r8169.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)


diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 60f96c4..c4e331c 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -63,6 +63,7 @@ static const int multicast_filter_limit = 32;
 #define RX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
 #define TX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
 #define EarlyTxThld	0x3F	/* 0x3F means NO early transmit */
+#define RxPacketMaxSize	0x3FE8	/* 16K - 1 - ETH_HLEN - VLAN - CRC... */
 #define SafeMtu		0x1c20	/* ... actually life sucks beyond ~7k */
 #define InterFrameGap	0x03	/* 3 means InterFrameGap = the shortest one */
 
@@ -186,7 +187,7 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
-static int rx_copybreak = 200;
+static int rx_copybreak = 16383;
 static int use_dac;
 static struct {
 	u32 msg_enable;
@@ -3383,7 +3384,7 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr)
 static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz)
 {
 	/* Low hurts. Let's disable the filtering. */
-	RTL_W16(RxMaxSize, rx_buf_sz + 1);
+	RTL_W16(RxMaxSize, 16383);
 }
 
 static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version)
@@ -3430,7 +3431,7 @@ static void rtl_hw_start_8169(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr, 16383);
 
 	if ((tp->mac_version == RTL_GIGA_MAC_VER_01) ||
 	    (tp->mac_version == RTL_GIGA_MAC_VER_02) ||
@@ -3691,7 +3692,7 @@ static void rtl_hw_start_8168(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr, 16383);
 
 	tp->cp_cmd |= RTL_R16(CPlusCmd) | PktCntrDisable | INTT_1;
 
@@ -3871,7 +3872,7 @@ static void rtl_hw_start_8101(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr, 16383);
 
 	tp->cp_cmd |= rtl_rw_cpluscmd(ioaddr) | PCIMulRW;
 
@@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
 
 	pad = align ? align : NET_IP_ALIGN;
 
-	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
+	skb = netdev_alloc_skb(dev, 16383 + pad);
 	if (!skb)
 		goto err_out;
 

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Ben Hutchings]

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

On Mon, 2009-12-28 at 20:16 -0500, Neil Horman wrote:
> On Tue, Dec 29, 2009 at 12:51:52AM +0000, Ben Hutchings wrote:
> > On Mon, 2009-12-28 at 22:31 +0100, François romieu wrote:
> > > (I'm back)
> > > 
> > > The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> > > [...]
> > > > frames were received on NIC's supported by this driver.  This was mentioned in a
> > > > security conference recently:
> > > > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> > > 
> > > Is there a paper ?
> > [...]
> > 
> > I was present in the session and emailed you and Neil about this
> > yesterday; did you not get the message?  The slides are now linked from
> > the above page; see pages 74-87.  I suggest you ask the speaker if you
> > need any more details.
> > 
> > Ben.
> > 
> I'm sorry, Ben, I don't think I received this note (although I admit I've not
> been checking to closely over the holidays).  I jumped on this after I got a
> phone call about it today.  I'll make sure to look for your note shortly.

My memory failed me - I actually sent this to Francois and Eric:

> Hopefully you're already aware of this, but I'm not sure.
> 
> Fabian Yamaguchi made a presentation at 26C3
> <http://events.ccc.de/congress/2009/Fahrplan/events/3596.en.html> which
> included a bug in r8169 reintroduced by:
> 
> commit fdd7b4c3302c93f6833e338903ea77245eb510b4
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Tue Jun 9 04:01:02 2009 -0700
> 
>     r8169: fix crash when large packets are received
> 
> On some older r8169 controllers this will enable scattering on receive,
> and the first word of the second and subsequent RX buffers for a frame
> will wrongly be treated as a status word.  This can be used for denial
> of service at the very least.

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH RFC] r8169: straighten out overlength frame detection

[Neil Horman]

On Mon, Dec 28, 2009 at 10:31:14PM +0100, François romieu wrote:
> (I'm back)
> 
> The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> [...]
> > frames were received on NIC's supported by this driver.  This was mentioned in a
> > security conference recently:
> > http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> 
> Is there a paper ?
> 
> > It seems that if we can't enable frame size filtering, then, as Eric correctly
> > noticed, we can find ourselves DMA-ing too much data to a buffer, causing
> > corruption.  As a result is seems that we are forced to allocate a frame which
> > is ready to handle a maximally sized receive.
> 
> Either that or the switch does not allow jumbo frames.
> 
> > I've not tested the below patch at all, and clearly it stinks to have to do.
> > But I thought it would be worth posting to solicit comments on it.
> [...]
> > diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> > index 60f96c4..42e3b22 100644
> > --- a/drivers/net/r8169.c
> > +++ b/drivers/net/r8169.c
> > @@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
> >  
> >  	pad = align ? align : NET_IP_ALIGN;
> >  
> > -	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
> > +	skb = netdev_alloc_skb(dev, 16383 + pad);
> 
> I doubt that we will be able to allocate that much memory reliably for long.
> 
> I'd rather go for static buffers + copy (+ src mac address of our new friend).
> 
> Is it enough if I write it in a pair of evening ?
> 
> -- 
> Ueimor
> 

Sorry for the noise, but I'm juggling this and family visits at the same time
over the hollidays :)

It just occured to me that this would be a much easier way to fix this issue,
than what I had previously posted.  Instead of reverting Erics patch, we just
set rx_buf_sz and copybreak to 16383.  That should force the behavior we're
after, and it can easily be tuned for hw that works properly in set_rx_max_buf

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 r8169.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)


diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 60f96c4..cba3966 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -186,7 +186,12 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
-static int rx_copybreak = 200;
+/*
+ * copybreak default is set here so that we
+ * can copy frames rather than needing to constantly
+ * reallocate 4 page skbs
+ */
+static int rx_copybreak = 16383;
 static int use_dac;
 static struct {
 	u32 msg_enable;
@@ -3247,9 +3252,14 @@ static void __devexit rtl8169_remove_one(struct pci_dev *pdev)
 static void rtl8169_set_rxbufsize(struct rtl8169_private *tp,
 				  struct net_device *dev)
 {
-	unsigned int max_frame = dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
-
-	tp->rx_buf_sz = (max_frame > RX_BUF_SIZE) ? max_frame : RX_BUF_SIZE;
+	/*
+	 * Note: Don't touch this.  Some r8169 hw
+	 * Can't deliver a proper frame length
+	 * if rx filtering is enabled, so we need to 
+	 * disable it, which in turn means we need to
+	 * be ready to receive maximally sized frames
+	 */
+	tp->rx_buf_sz = 16383;
 }
 
 static int rtl8169_open(struct net_device *dev)
@@ -3383,7 +3393,7 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr)
 static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz)
 {
 	/* Low hurts. Let's disable the filtering. */
-	RTL_W16(RxMaxSize, rx_buf_sz + 1);
+	RTL_W16(RxMaxSize, (rx_buf_sz == 16383) ? rx_buf_sz : rx_buf_sz + 1);
 }
 
 static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version)

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Neil Horman]

Ok, third times the charm.  Its been noted that both apporaches to fixing this
bug are going to have major performance impacts, which is bad.  Unless we can
get a list of affected hardware to restrict the fix to certain NICS, I don't see
how we can get around that.   I have come up with this patch however, which I
think at least partially addresses the performance concerns.

        A while back Eric submitted a patch for r8169 in which the proper
allocated frame size was written to RXMaxSize to prevent the NIC from dmaing too
much data.  This was done in commit fdd7b4c3302c93f6833e338903ea77245eb510b4.  A
long time prior to that however, Francois posted
126fa4b9ca5d9d7cb7d46f779ad3bd3631ca387c, which expiclitly disabled the MaxSize
setting due to the fact that the hardware behaved in odd ways when overlong
frames were received on NIC's supported by this driver.  This was mentioned in a
security conference recently:
http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html

It seems that if we can't enable frame size filtering, then, as Eric correctly
noticed, we can find ourselves DMA-ing too much data to a buffer, causing
corruption.  As a result is seems that we are forced to allocate a frame which
is ready to handle a maximally sized receive.

This obviously has performance issues with it, so to mitigate that issue, this
patch does two things:

1) Raises the copybreak value to the frame allocation size, which should force
appropriately sized packets to get allocated on rx, rather than a full new 16k
buffer.

2) This patch only disables frame filtering initially (i.e., during the NIC
open), changing the MTU results in ring buffer allocation of a size in relation
to the new mtu (along with a warning indicating that this is dangerous).

Because of item (2), individuals who can't cope with the performance hit (or can
otherwise filter frames to prevent the bug), or who have hardware they are sure
is unaffected by this issue, can manually lower the copybreak and reset the mtu
such that performance is restored easily.

Signed-off-by: Neil Horman <nhorman@redhat.com>



 r8169.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)


diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index c403ce0..9aac49c 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -186,7 +186,12 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
-static int rx_copybreak = 200;
+/*
+ * we set our copybreak very high so that we don't have
+ * to allocate 16k frames all the time (see note in
+ * rtl8169_open()
+ */ 
+static int rx_copybreak = 16383;
 static int use_dac;
 static struct {
 	u32 msg_enable;
@@ -3240,9 +3245,13 @@ static void __devexit rtl8169_remove_one(struct pci_dev *pdev)
 }
 
 static void rtl8169_set_rxbufsize(struct rtl8169_private *tp,
-				  struct net_device *dev)
+				  unsigned int mtu)
 {
-	unsigned int max_frame = dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+	unsigned int max_frame = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+
+	if (max_frame != 16383)
+		printk(KERN_WARNING "WARNING! Changing of MTU on this NIC"
+			"May lead to frame reception errors!\n");
 
 	tp->rx_buf_sz = (max_frame > RX_BUF_SIZE) ? max_frame : RX_BUF_SIZE;
 }
@@ -3254,7 +3263,17 @@ static int rtl8169_open(struct net_device *dev)
 	int retval = -ENOMEM;
 
 
-	rtl8169_set_rxbufsize(tp, dev);
+	/*
+	 * Note that we use a magic value here, its wierd I know
+	 * its done because, some subset of rtl8169 hardware suffers from
+	 * a problem in which frames received that are longer than 
+	 * the size set in RxMaxSize register return garbage sizes
+	 * when received.  To avoid this we need to turn off filtering, 
+	 * which is done by setting a value of 16383 in the RxMaxSize register
+	 * and allocating 16k frames to handle the largest possible rx value
+	 * thats what the magic math below does.
+	 */
+	rtl8169_set_rxbufsize(tp, 16383 - VLAN_ETH_HLEN - ETH_FCS_LEN);
 
 	/*
 	 * Rx and Tx desscriptors needs 256 bytes alignment.
@@ -3907,7 +3926,7 @@ static int rtl8169_change_mtu(struct net_device *dev, int new_mtu)
 
 	rtl8169_down(dev);
 
-	rtl8169_set_rxbufsize(tp, dev);
+	rtl8169_set_rxbufsize(tp, dev->mtu);
 
 	ret = rtl8169_init_ring(dev);
 	if (ret < 0)

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Eric Dumazet]

Le 05/01/2010 14:57, Neil Horman a écrit :
> Ok, third times the charm.  Its been noted that both apporaches to fixing this
> bug are going to have major performance impacts, which is bad.  Unless we can
> get a list of affected hardware to restrict the fix to certain NICS, I don't see
> how we can get around that.   I have come up with this patch however, which I
> think at least partially addresses the performance concerns.
> 
>         A while back Eric submitted a patch for r8169 in which the proper
> allocated frame size was written to RXMaxSize to prevent the NIC from dmaing too
> much data.  This was done in commit fdd7b4c3302c93f6833e338903ea77245eb510b4.  A
> long time prior to that however, Francois posted
> 126fa4b9ca5d9d7cb7d46f779ad3bd3631ca387c, which expiclitly disabled the MaxSize
> setting due to the fact that the hardware behaved in odd ways when overlong
> frames were received on NIC's supported by this driver.  This was mentioned in a
> security conference recently:
> http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html
> 
> It seems that if we can't enable frame size filtering, then, as Eric correctly
> noticed, we can find ourselves DMA-ing too much data to a buffer, causing
> corruption.  As a result is seems that we are forced to allocate a frame which
> is ready to handle a maximally sized receive.
> 
> This obviously has performance issues with it, so to mitigate that issue, this
> patch does two things:
> 
> 1) Raises the copybreak value to the frame allocation size, which should force
> appropriately sized packets to get allocated on rx, rather than a full new 16k
> buffer.
> 
> 2) This patch only disables frame filtering initially (i.e., during the NIC
> open), changing the MTU results in ring buffer allocation of a size in relation
> to the new mtu (along with a warning indicating that this is dangerous).
> 
> Because of item (2), individuals who can't cope with the performance hit (or can
> otherwise filter frames to prevent the bug), or who have hardware they are sure
> is unaffected by this issue, can manually lower the copybreak and reset the mtu
> such that performance is restored easily.
> 
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> 
> 
> 
>  r8169.c |   29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index c403ce0..9aac49c 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -186,7 +186,12 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
>  
>  MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
>  
> -static int rx_copybreak = 200;
> +/*
> + * we set our copybreak very high so that we don't have
> + * to allocate 16k frames all the time (see note in
> + * rtl8169_open()
> + */ 
> +static int rx_copybreak = 16383;
>  static int use_dac;
>  static struct {
>  	u32 msg_enable;
> @@ -3240,9 +3245,13 @@ static void __devexit rtl8169_remove_one(struct pci_dev *pdev)
>  }
>  
>  static void rtl8169_set_rxbufsize(struct rtl8169_private *tp,
> -				  struct net_device *dev)
> +				  unsigned int mtu)
>  {
> -	unsigned int max_frame = dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
> +	unsigned int max_frame = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
> +
> +	if (max_frame != 16383)
> +		printk(KERN_WARNING "WARNING! Changing of MTU on this NIC"
> +			"May lead to frame reception errors!\n");
>  
>  	tp->rx_buf_sz = (max_frame > RX_BUF_SIZE) ? max_frame : RX_BUF_SIZE;
>  }
> @@ -3254,7 +3263,17 @@ static int rtl8169_open(struct net_device *dev)
>  	int retval = -ENOMEM;
>  
>  
> -	rtl8169_set_rxbufsize(tp, dev);
> +	/*
> +	 * Note that we use a magic value here, its wierd I know
> +	 * its done because, some subset of rtl8169 hardware suffers from
> +	 * a problem in which frames received that are longer than 
> +	 * the size set in RxMaxSize register return garbage sizes
> +	 * when received.  To avoid this we need to turn off filtering, 
> +	 * which is done by setting a value of 16383 in the RxMaxSize register
> +	 * and allocating 16k frames to handle the largest possible rx value
> +	 * thats what the magic math below does.
> +	 */
> +	rtl8169_set_rxbufsize(tp, 16383 - VLAN_ETH_HLEN - ETH_FCS_LEN);
>  
>  	/*
>  	 * Rx and Tx desscriptors needs 256 bytes alignment.
> @@ -3907,7 +3926,7 @@ static int rtl8169_change_mtu(struct net_device *dev, int new_mtu)
>  
>  	rtl8169_down(dev);
>  
> -	rtl8169_set_rxbufsize(tp, dev);
> +	rtl8169_set_rxbufsize(tp, dev->mtu);
>  
>  	ret = rtl8169_init_ring(dev);
>  	if (ret < 0)
> --

Its a start, but should not depend on MTU of device.
If a script sets it to 1500, we can have the security problem again ?

We should have static buffers of 16384 bytes, and always copy to freshly allocated skbs.

If hardware is buggy, driver should focus on security first,
performance doesnt matter in this case.

It seems that we should also avoid the sizeof(FCS) subtract too.
(or test that pkt_size is >= min_frame_size)

(the guy was able to feed driver with a 'random' status..., making machine crash again ?

Sorry, I dont have this hardware so cannot test your patch.

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 60f96c4..c2bbf59 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4500,7 +4500,7 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
                } else {
                        struct sk_buff *skb = tp->Rx_skbuff[entry];
                        dma_addr_t addr = le64_to_cpu(desc->addr);
-                       int pkt_size = (status & 0x00001FFF) - 4;
+                       int pkt_size = (status & 0x00001FFF);
                        struct pci_dev *pdev = tp->pci_dev;
 
                        /*


Avoiding FCS copy brings almost nothing at all anyway, many drivers dont care.

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 05 Jan 2010 16:15:53 +0100

> If hardware is buggy, driver should focus on security first,
> performance doesnt matter in this case.

Copying every packet is way too much of a performance hit to
accept.

Things are not so black and white, there are shades of grey.

We can fix this without making things so slow, try harder...

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Neil Horman]

On Tue, Jan 05, 2010 at 12:40:40PM -0800, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 05 Jan 2010 16:15:53 +0100
> 
> > If hardware is buggy, driver should focus on security first,
> > performance doesnt matter in this case.
> 
> Copying every packet is way too much of a performance hit to
> accept.
> 
> Things are not so black and white, there are shades of grey.
> 
> We can fix this without making things so slow, try harder...
I think thats what I've done in my third version of the patch here.  By raising
the copybreak value, and setting an initial allocation size of 16k, we:

1) close the security hole by default
2) allow for smaller allocations during run time (allocations of the recevied
frame size, rather than 16k every time)
3) Give the user an out if they need the added performance. They have a module
option to lower the copybreak thrshold, and changing the mtu to a different
value will reset the rx allocation size, and frame filter value (and issue a big
warning that doing so may be dangerous)

As we determine which hardware id's are buggy, we can start blacklisting them
specifically, ejoining them from making mtu changes at all, and when we are
confident we have identified all the buggy hardware, we can flip the default
behavior to allocate a resonable frame size and set a good copybreak threshold
for anything not on the blacklist.

Regards
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
> 

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 5 Jan 2010 16:38:56 -0500

> I think thats what I've done in my third version of the patch here.
> By raising the copybreak value, and setting an initial allocation
> size of 16k, we:

Right, I'll take an even closer look at your patch later this
afternoon, thanks Neil.

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Neil Horman]

On Tue, Jan 05, 2010 at 01:45:37PM -0800, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue, 5 Jan 2010 16:38:56 -0500
> 
> > I think thats what I've done in my third version of the patch here.
> > By raising the copybreak value, and setting an initial allocation
> > size of 16k, we:
> 
> Right, I'll take an even closer look at your patch later this
> afternoon, thanks Neil.
> 
No problem, thank you Dave!
Neil

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Francois Romieu]

On Tue, Jan 05, 2010 at 12:40:40PM -0800, David Miller wrote:
[...]
> We can fix this without making things so slow, try harder...

I have reproduced something which seems rather close to the behavior
described in the ccc paper (I have a couple of old PCI rev 10 8169
chipset based network cards).

The setup includes two r8169 face-to-face. The target has an usual
MTU. I tweaked its skb allocation function to include 4000 bytes
of space instead of the usual 1500 something. It is grossly poisoned
to estimate the size of the actual DMA. The RX descriptor has no
knowledge of the extra buffer space above 1500 bytes.

The sender has a bigger MTU (say 6000) and it sends increasingly
sized ping. No rocket science so far.

The result looks like this (so far I checked only one descriptor at
a time when a packet is received):

entry 000 opts1 0x3281c040 opts2 0x00000000 size 066
entry 001 opts1 0x3486c5ec opts2 0x00000000 size 1518
entry 002 opts1 0x3481c040 opts2 0x00000000 size 066
entry 003 opts1 0x3486c5ed opts2 0x00000000 size 1518
entry 004 opts1 0x3486c5ee opts2 0x00000000 size 1518
entry 005 opts1 0x3481c040 opts2 0x00000000 size 066
entry 006 opts1 0x3486c5ef opts2 0x00000000 size 1522
entry 007 opts1 0x3481c040 opts2 0x00000000 size 066
entry 008 opts1 0x3486c5f0 opts2 0x00000000 size 1522 <- first test packet
entry 009 opts1 0x3486c5f1 opts2 0x00000000 size 1522
entry 010 opts1 0x3486c5f2 opts2 0x00000000 size 1522
entry 011 opts1 0x3486c5f3 opts2 0x00000000 size 1526
entry 012 opts1 0x3486c5f4 opts2 0x00000000 size 1526
entry 013 opts1 0x3486c5f5 opts2 0x00000000 size 1526 <- ... so far so good
entry 014 opts1 0x3486c5f6 opts2 0x00000000 size 1526
entry 015 opts1 0x3486c5f7 opts2 0x00000000 size 1530
entry 016 opts1 0x3486c5f8 opts2 0x00000000 size 1530
entry 017 opts1 0x3486c5f9 opts2 0x00000000 size 1530
entry 018 opts1 0x3486c5fa opts2 0x00000000 size 1530
entry 019 opts1 0x3486c5fb opts2 0x00000000 size 1534
entry 020 opts1 0x3486c5fc opts2 0x00000000 size 1534
entry 021 opts1 0x3486c5fd opts2 0x00000000 size 1534
entry 022 opts1 0x3486c5fe opts2 0x00000000 size 1534
entry 023 opts1 0x3486c5ff opts2 0x00000000 size 1536
entry 024 opts1 0x20803ff0 opts2 0x00000000 size 1536 <- problem starts here.
entry 025 opts1 0x00803ff0 opts2 0x00000000 size 1536    The lines below are
entry 026 opts1 0x00803ff0 opts2 0x00000000 size 1536    not meaningful.
entry 027 opts1 0x00803ff0 opts2 0x00000000 size 1536

The driver will mess things up because it needs the desc->opts1.RxRes bit
to be set before it claims that the packet is broken. Since this condition
is not fulfilled when opts1 = 0x20803ff0, the problem goes unnoticed and
it can be further exploited.

AFAIUI, if the driver does not lose sync (to use the paper's words), i.e.
if it notices the condition above and stops / resets the chipset, there
is not much to exploit.

-- 
Ueimor

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Thu, 7 Jan 2010 02:01:22 +0100

> entry 024 opts1 0x20803ff0 opts2 0x00000000 size 1536 <- problem starts here.
> entry 025 opts1 0x00803ff0 opts2 0x00000000 size 1536    The lines below are
> entry 026 opts1 0x00803ff0 opts2 0x00000000 size 1536    not meaningful.
> entry 027 opts1 0x00803ff0 opts2 0x00000000 size 1536
> 
> The driver will mess things up because it needs the desc->opts1.RxRes bit
> to be set before it claims that the packet is broken. Since this condition
> is not fulfilled when opts1 = 0x20803ff0, the problem goes unnoticed and
> it can be further exploited.
> 
> AFAIUI, if the driver does not lose sync (to use the paper's words), i.e.
> if it notices the condition above and stops / resets the chipset, there
> is not much to exploit.

Thanks for doing this test.

Also note the FirstFrag and LastFrag bits.  Here we see FirstFrag set,
and this indicates a fragmented frame.

The fix seems to be to simply check the error bits unconditionally, or
alternatively validate the FirstFrag and LastFrag bits
unconditionally.

I don't even think we need to reset the chip, just do what the code
does now and recycle the buffer back to the chip as we currently do
when RxRes is set.  We can keep a state bit around, showing that we
saw the beginning of a fragmented frame, and we recycle buffers back
to the chip when in state state until we see LastFrag.

That should work, right?

In your trace, we're merely seeing the >1536 frame being chopped
into a fragmented frame by the device.  The first one has FirstFrag
set and the remaining (that you've shown) have neither bit set.

Did you eventually get a descriptor with LastFrag (bit 28) set?

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Francois Romieu]

On Wed, Jan 06, 2010 at 05:15:14PM -0800, David Miller wrote:
[...]
> I don't even think we need to reset the chip, just do what the code
> does now and recycle the buffer back to the chip as we currently do
> when RxRes is set.  We can keep a state bit around, showing that we
> saw the beginning of a fragmented frame, and we recycle buffers back
> to the chip when in state state until we see LastFrag.
> 
> That should work, right ?
> 
> In your trace, we're merely seeing the >1536 frame being chopped
> into a fragmented frame by the device.  The first one has FirstFrag
> set and the remaining (that you've shown) have neither bit set.
> 
> Did you eventually get a descriptor with LastFrag (bit 28) set?

Yes.

For a single packet, at the time the first interruption is received,
4 Rx descriptors have been written. The FirstFrag bit is set on the
first descriptor only and the LastFrag bit is not set anywhere :

entry 019 opts1 0x20803ff0 opts2 0x00000000 used 1536 <- FirstFrag
entry 020 opts1 0x00803ff0 opts2 0x00000000 used 1536
entry 021 opts1 0x00803ff0 opts2 0x00000000 used 1536
entry 022 opts1 0x00803ff0 opts2 0x00000000 used 1536
entry 023 opts1 0x80000600 opts2 0x00000000 used 1524
                                                 ^^^^
(apparently we race with the DMA transfer)

More events pop up, starting at 023. They are all identical to 20 / 21 /22.
It then ends as :

entry 028 opts1 0x00803ff0 opts2 0x00000000 used 1536
entry 029 opts1 0x10803ff0 opts2 0x00000000 used 1010 <- LastFrag bit set
entry 030 opts1 0x80000600 opts2 0x00000000 used 000  <- 0 used byte. Entry
entry 031 opts1 0x80000600 opts2 0x00000000 used 000     is available.
entry 032 opts1 0x80000600 opts2 0x00000000 used 000

The total size of the DMA is not far from 16384 bytes (1536 * 10 + 1010).

Simply recycling the buffer may work. I'll do a few tests with it : I am
still unable to corrupt the descriptor ring itself.

-- 
Ueimor

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 9 Jan 2010 00:48:00 +0100

> For a single packet, at the time the first interruption is received,
> 4 Rx descriptors have been written. The FirstFrag bit is set on the
> first descriptor only and the LastFrag bit is not set anywhere :
> 
> entry 019 opts1 0x20803ff0 opts2 0x00000000 used 1536 <- FirstFrag
> entry 020 opts1 0x00803ff0 opts2 0x00000000 used 1536
> entry 021 opts1 0x00803ff0 opts2 0x00000000 used 1536
> entry 022 opts1 0x00803ff0 opts2 0x00000000 used 1536
> entry 023 opts1 0x80000600 opts2 0x00000000 used 1524
>                                                  ^^^^
> (apparently we race with the DMA transfer)
>
> More events pop up, starting at 023. They are all identical to 20 / 21 /22.
> It then ends as :
> 
> entry 028 opts1 0x00803ff0 opts2 0x00000000 used 1536
> entry 029 opts1 0x10803ff0 opts2 0x00000000 used 1010 <- LastFrag bit set
> entry 030 opts1 0x80000600 opts2 0x00000000 used 000  <- 0 used byte. Entry
> entry 031 opts1 0x80000600 opts2 0x00000000 used 000     is available.
> entry 032 opts1 0x80000600 opts2 0x00000000 used 000
> 
> The total size of the DMA is not far from 16384 bytes (1536 * 10 + 1010).

My guess is that once the chip starts filling out frags like this,
the signal to trigger what should be the LastFrag entry never
propagates to the DMA engine and thus the descriptor table.

Thus the device just keeps filling packet contents with subsequent
data, with FirstFrag and LastFrag clear, until that ~16K limit is
reached.  At which point it finally sets LastFrag.

> Simply recycling the buffer may work. I'll do a few tests with it : I am
> still unable to corrupt the descriptor ring itself.

Whilst the above will end up gobbling up to (16K - BIG_PACKET_SZ)
worth of innocent frames, the DMA engine seems to keep things at least
self-consistent.

The only bug seems to be the omission of the LastFrag trigger at the
proper place.

About recycling, as far as I can tell that's exactly what the driver
already does in these exact cases, right?  If RxRes is not set, and
we don't see both FirstFrag and LastFrag set, we'll recycle the frame.

		if (unlikely(status & RxRES)) {
 ...
		} else {
 ...
			if (unlikely(rtl8169_fragmented_frame(status))) {
				dev->stats.rx_dropped++;
				dev->stats.rx_length_errors++;
				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
				continue;
			}

Therefore we shouldn't need to change anything and there is actually
no "bug" or "exploit" at all.  We just end up dropping some RX frames
because the chip didn't DMA them properly.

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Ben Hutchings]

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

On Fri, 2010-01-08 at 16:02 -0800, David Miller wrote:
[...]
> Whilst the above will end up gobbling up to (16K - BIG_PACKET_SZ)
> worth of innocent frames, the DMA engine seems to keep things at least
> self-consistent.
> 
> The only bug seems to be the omission of the LastFrag trigger at the
> proper place.

No, the attacker controls the completion status by writing it in
previous valid frames.  Please read the slides
(<http://events.ccc.de/congress/2009/Fahrplan/attachments/1483_26c3_ipv4_fuckups.pdf> pages 80-87).

I suspect that:
1. There is an internal ring buffer for RX DMA containing both frame
payload and completion status
2. When a frame is (slightly?) over-length, the ingress and egress logic
can disagree about the length of payload in the buffer
3. This results in stale data (usually frame payload) being written to
memory as the completion status

It is conceivable that the bug can be avoided simply by rounding the 
RxMaxSize.

[...]
> Therefore we shouldn't need to change anything and there is actually
> no "bug" or "exploit" at all.  We just end up dropping some RX frames
> because the chip didn't DMA them properly.

The intent of the exploit is precisely to cause other packets to be
dropped!

Ben.

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
                                - John Levine, moderator of comp.compilers

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

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Francois Romieu]

On Sun, Jan 10, 2010 at 01:57:18AM +0000, Ben Hutchings wrote:
> On Fri, 2010-01-08 at 16:02 -0800, David Miller wrote:
> [...]
> > Whilst the above will end up gobbling up to (16K - BIG_PACKET_SZ)
> > worth of innocent frames, the DMA engine seems to keep things at least
> > self-consistent.
> > 
> > The only bug seems to be the omission of the LastFrag trigger at the
> > proper place.
> 
> No, the attacker controls the completion status by writing it in
> previous valid frames.  Please read the slides
> (<http://events.ccc.de/congress/2009/Fahrplan/attachments/1483_26c3_ipv4_fuckups.pdf> pages 80-87).

Yes.

The paper suggests that it should be possible to control opts1
either completely or enough to break things badly.

The first packet does not hurt too much. Things look different in
the descriptor of the subsequent packets. I am more or less able
to control bits 8-23 from 0-31 in opts1, enough to be unpleasant.

Iff the FirstFrag and LastFrag bits can not be set on these packets,
it should be enough to (1) do the fragmented_frame test sooner and
return the descriptor to the chipset. Otherwise we can (2) take a
complete reset on the first suspect packet (whose pattern is more
specific) to stop the challenger here.

FWIW, a netratelimited printk of the source mac address may help too.

I am biased in favor of (2) as:
- it will not inhibit multi-desc packets
- the challenger is supposed to be in the LAN and can already hurt
  quite a lot anyway

Comments ?

-- 
Ueimor

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Mon, 11 Jan 2010 00:50:17 +0100

> Iff the FirstFrag and LastFrag bits can not be set on these packets,
> it should be enough to (1) do the fragmented_frame test sooner and
> return the descriptor to the chipset. Otherwise we can (2) take a
> complete reset on the first suspect packet (whose pattern is more
> specific) to stop the challenger here.
> 
> FWIW, a netratelimited printk of the source mac address may help too.
> 
> I am biased in favor of (2) as:
> - it will not inhibit multi-desc packets
> - the challenger is supposed to be in the LAN and can already hurt
>   quite a lot anyway
> 
> Comments ?

In my opinion if you reset, you give them more power.

Instead of just dropping the next few frames, you allow them
to cause a drop of how ever many RX frames can arrive during
the reset period _PLUS_ the amount of other RX frames which
were in the receive ring at the point of detection.

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Francois Romieu]

On Sun, Jan 10, 2010 at 10:45:04PM -0800, David Miller wrote:
[...]
> In my opinion if you reset, you give them more power.
> 
> Instead of just dropping the next few frames, you allow them
> to cause a drop of how ever many RX frames can arrive during
> the reset period _PLUS_ the amount of other RX frames which
> were in the receive ring at the point of detection.

Sure.

The fragmented frame test has been moved a few lines up, before checking
for the status bits. Exceedingly small frames are detected and dropped
as well.

I have spent the evening disrupting the communication. As a challenger
one does not even need complete control : send a few random packets and
the card goes to neverland. :o/

I'll do some tests limiting the crap at the first packet.

-- 
Ueimor

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Tue, 12 Jan 2010 01:16:46 +0100

> I have spent the evening disrupting the communication. As a challenger
> one does not even need complete control : send a few random packets and
> the card goes to neverland. :o/
> 
> I'll do some tests limiting the crap at the first packet.

Thanks for doing this work Francois.

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Brandon Philips]

On 22:24 Mon 11 Jan 2010, David Miller wrote:
> From: Francois Romieu <romieu@fr.zoreil.com>
> Date: Tue, 12 Jan 2010 01:16:46 +0100
> 
> > I have spent the evening disrupting the communication. As a challenger
> > one does not even need complete control : send a few random packets and
> > the card goes to neverland. :o/
> > 
> > I'll do some tests limiting the crap at the first packet.
> 
> Thanks for doing this work Francois.

How is the testing going? 

Thanks again,

	Brandon

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Neil Horman]

On Tue, Jan 12, 2010 at 01:16:46AM +0100, Francois Romieu wrote:
> On Sun, Jan 10, 2010 at 10:45:04PM -0800, David Miller wrote:
> [...]
> > In my opinion if you reset, you give them more power.
> > 
> > Instead of just dropping the next few frames, you allow them
> > to cause a drop of how ever many RX frames can arrive during
> > the reset period _PLUS_ the amount of other RX frames which
> > were in the receive ring at the point of detection.
> 
> Sure.
> 
> The fragmented frame test has been moved a few lines up, before checking
> for the status bits. Exceedingly small frames are detected and dropped
> as well.
> 
> I have spent the evening disrupting the communication. As a challenger
> one does not even need complete control : send a few random packets and
> the card goes to neverland. :o/
> 
> I'll do some tests limiting the crap at the first packet.
> 
Any further results you can share with us Francois?

Thanks
Neil

Re: [PATCH RFC] r8169: straighten out overlength frame detection (v3)

[Brandon Philips]

On 16:50 Sat 30 Jan 2010, Neil Horman wrote:
> On Tue, Jan 12, 2010 at 01:16:46AM +0100, Francois Romieu wrote:
> > On Sun, Jan 10, 2010 at 10:45:04PM -0800, David Miller wrote:
> > [...]
> > > In my opinion if you reset, you give them more power.
> > > 
> > > Instead of just dropping the next few frames, you allow them
> > > to cause a drop of how ever many RX frames can arrive during
> > > the reset period _PLUS_ the amount of other RX frames which
> > > were in the receive ring at the point of detection.
> > 
> > Sure.
> > 
> > The fragmented frame test has been moved a few lines up, before checking
> > for the status bits. Exceedingly small frames are detected and dropped
> > as well.
> > 
> > I have spent the evening disrupting the communication. As a challenger
> > one does not even need complete control : send a few random packets and
> > the card goes to neverland. :o/
> > 
> > I'll do some tests limiting the crap at the first packet.
> > 
> Any further results you can share with us Francois?

Can I offer any help? I found that I have access to a Rev 0x10 card.

Cheers,

	Brandon