[PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[Neil Horman]

Official patch to fix the r8169 frame length check error.

Based on this initial thread:
http://marc.info/?l=linux-netdev&m=126202972828626&w=1
This is the official patch to fix the frame length problems in the r8169
driver.  As noted in the previous thread, while this patch incurs a performance
hit on the driver, its possible to improve performance dynamically by updating
the mtu and rx_copybreak values at runtime to return performance to what it was
for those NICS which are unaffected by the ideosyncracy (if there are any).

Summary:

    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 964305c..1db95d4 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -187,7 +187,12 @@ static DEFINE_PCI_DEVICE_TABLE(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 = -1;
 static struct {
 	u32 msg_enable;
@@ -3254,9 +3259,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;
 }
@@ -3269,7 +3278,17 @@ static int rtl8169_open(struct net_device *dev)
 
 	pm_runtime_get_sync(&pdev->dev);
 
-	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.
@@ -3929,7 +3948,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] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 29 Mar 2010 12:03:56 -0400

> Official patch to fix the r8169 frame length check error.
 ...
> Signed-off-by: Neil Horman <nhorman@redhat.com>

Applied, thanks a lot Neil.

Re: [PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[Ben Hutchings]

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

On Mon, 2010-03-29 at 12:03 -0400, Neil Horman wrote:
> Official patch to fix the r8169 frame length check error.
> 
> Based on this initial thread:
> http://marc.info/?l=linux-netdev&m=126202972828626&w=1
> This is the official patch to fix the frame length problems in the r8169
> driver.  As noted in the previous thread, while this patch incurs a performance
> hit on the driver, its possible to improve performance dynamically by updating
> the mtu and rx_copybreak values at runtime to return performance to what it was
> for those NICS which are unaffected by the ideosyncracy (if there are any).
[...]

I have to say that this compromise behaviour is highly non-obvious.
Further, there is now no way to set a non-standard MTU without enabling
the insecure behaviour.  (This is in part a flaw in the driver
interface, of course.  We should have an interface to get and set MRU
rather than making drivers decide an MRU based on the MTU,
inconsistently and without any visibility to the administrator.)

It also sucks that the secure but low-performance behaviour is enabled
for all variants, while AIUI only some suffer from the bug.  I realise
you probably don't have access to every variant (and neither does
Francois) but perhaps you could come up with a test case that could be
used to start whitelisting common variants that don't have the bug?

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

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

Re: [PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 29 Mar 2010 23:01:45 +0100

> It also sucks that the secure but low-performance behaviour is enabled
> for all variants, while AIUI only some suffer from the bug.  I realise
> you probably don't have access to every variant (and neither does
> Francois) but perhaps you could come up with a test case that could be
> used to start whitelisting common variants that don't have the bug?

As far as we know all chip variants seem to have the problem.

Furthermore, this issue has been known about and investigated for
about 3 months.  In that time no better options for handling this
issue reliably have been discovered and implemented.

Feel free to code up (and test) something better yourself if you don't
like the fix as it exists currently. :-)

Re: [PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[Ben Hutchings]

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

On Mon, 2010-03-29 at 15:09 -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Mon, 29 Mar 2010 23:01:45 +0100
> 
> > It also sucks that the secure but low-performance behaviour is enabled
> > for all variants, while AIUI only some suffer from the bug.  I realise
> > you probably don't have access to every variant (and neither does
> > Francois) but perhaps you could come up with a test case that could be
> > used to start whitelisting common variants that don't have the bug?
> 
> As far as we know all chip variants seem to have the problem.

That's not what I understood from the discussion of the early
back-and-forth changes to receive buffer size.

> Furthermore, this issue has been known about and investigated for
> about 3 months.  In that time no better options for handling this
> issue reliably have been discovered and implemented.
>
> Feel free to code up (and test) something better yourself if you don't
> like the fix as it exists currently. :-)

I would have had a go already, if I actually had some of this hardware
to hand.  Luckily I have managed to avoid buying any so far.  But if
anyone is prepared to loan me a NIC then I promise to have a go at it.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

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

Re: [PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[Neil Horman]

On Mon, Mar 29, 2010 at 11:21:05PM +0100, Ben Hutchings wrote:
> On Mon, 2010-03-29 at 15:09 -0700, David Miller wrote:
> > From: Ben Hutchings <ben@decadent.org.uk>
> > Date: Mon, 29 Mar 2010 23:01:45 +0100
> > 
> > > It also sucks that the secure but low-performance behaviour is enabled
> > > for all variants, while AIUI only some suffer from the bug.  I realise
> > > you probably don't have access to every variant (and neither does
> > > Francois) but perhaps you could come up with a test case that could be
> > > used to start whitelisting common variants that don't have the bug?
> > 
> > As far as we know all chip variants seem to have the problem.
> 
> That's not what I understood from the discussion of the early
> back-and-forth changes to receive buffer size.
> 
As dave mentioned, we have no conclusive evidence that only certain chip
variants show the behavior.  realtek hasn't said anything about it one way or
the other.  AFAIK, all you need to do to test given chip is try to receive a
frame thats longer than configured receive filter.  I only had one NIC variant
to test, and it failed that test.

> > Furthermore, this issue has been known about and investigated for
> > about 3 months.  In that time no better options for handling this
> > issue reliably have been discovered and implemented.
> >
> > Feel free to code up (and test) something better yourself if you don't
> > like the fix as it exists currently. :-)
> 
> I would have had a go already, if I actually had some of this hardware
> to hand.  Luckily I have managed to avoid buying any so far.  But if
> anyone is prepared to loan me a NIC then I promise to have a go at it.
> 
I only had the one variant that failed.  My hope was to make them all safe, and,
should any variants be found in the field that didn't exhibit the behavior, we
could whitelist them as they got reported.

Regards
Neil

> Ben.
> 
> -- 
> Ben Hutchings
> Once a job is fouled up, anything done to improve it makes it worse.

Re: [PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[Brandon Philips]

On 12:03 Mon 29 Mar 2010, Neil Horman wrote:
> Signed-off-by: Neil Horman <nhorman@redhat.com>

Cc: stable@kernel.org?
Tested-by: Brandon Philips <bphilips@suse.de>

> +	if (max_frame != 16383)
> +		printk(KERN_WARNING "WARNING! Changing of MTU on this NIC"
> +			"May lead to frame reception errors!\n");

This needs a space to look right. You might as well add the PFX too:

		printk(KERN_WARNING PFX "WARNING! Changing of MTU on this "
			"NIC may lead to frame reception errors!\n");

Thanks Neil.

Cheers,

	Brandon

Re: [PATCH] r8169: offical fix for CVE-2009-4537 (overlength frame DMAs)

[Neil Horman]

On Wed, Mar 31, 2010 at 05:24:50PM -0700, Brandon Philips wrote:
> On 12:03 Mon 29 Mar 2010, Neil Horman wrote:
> > Signed-off-by: Neil Horman <nhorman@redhat.com>
> 
> Cc: stable@kernel.org?
> Tested-by: Brandon Philips <bphilips@suse.de>
> 
> > +	if (max_frame != 16383)
> > +		printk(KERN_WARNING "WARNING! Changing of MTU on this NIC"
> > +			"May lead to frame reception errors!\n");
> 
> This needs a space to look right. You might as well add the PFX too:
> 
> 		printk(KERN_WARNING PFX "WARNING! Changing of MTU on this "
> 			"NIC may lead to frame reception errors!\n");
> 
> Thanks Neil.
> 
> Cheers,
> 
> 	Brandon
> 
Daves already merged my patch, but I'll submit a cleanup patch shortly to take
care of this.
Thanks!
Neil