[PATCH] forcedeth: fix random memory scribbling bug

[PATCH] forcedeth: fix random memory scribbling bug

[Manfred Spraul]

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

Two critical bugs were found in forcedeth 0.47:
- TSO doesn't work.
- pci_map_single() for the rx buffers is called with size==0. This bug 
is critical, it causes random memory corruptions on systems with an iommu.

Below is a minimal fix for both bugs, for inclusion into 2.6.15.
TSO will be fixed properly in the next version.
Tested on x86-64.

Signed-Off-By: Manfred Spraul <manfred@colorfullife.com>

[-- Attachment #2: patch-forcedeth-048-bugfixes --]
[-- Type: text/plain, Size: 2291 bytes --]

--- 2.6/drivers/net/forcedeth.c	2005-12-19 01:36:54.000000000 +0100
+++ x64/drivers/net/forcedeth.c	2005-12-24 12:16:30.000000000 +0100
@@ -10,7 +10,7 @@
  * trademarks of NVIDIA Corporation in the United States and other
  * countries.
  *
- * Copyright (C) 2003,4 Manfred Spraul
+ * Copyright (C) 2003,4,5 Manfred Spraul
  * Copyright (C) 2004 Andrew de Quincey (wol support)
  * Copyright (C) 2004 Carl-Daniel Hailfinger (invalid MAC handling, insane
  *		IRQ rate fixes, bigendian fixes, cleanups, verification)
@@ -100,6 +100,7 @@
  *	0.45: 18 Sep 2005: Remove nv_stop/start_rx from every link check
  *	0.46: 20 Oct 2005: Add irq optimization modes.
  *	0.47: 26 Oct 2005: Add phyaddr 0 in phy scan.
+ *	0.48: 24 Dec 2005: Disable TSO, bugfix for pci_map_single
  *
  * Known bugs:
  * We suspect that on some hardware no TX done interrupts are generated.
@@ -111,7 +112,7 @@
  * DEV_NEED_TIMERIRQ will not harm you on sane hardware, only generating a few
  * superfluous timer interrupts from the nic.
  */
-#define FORCEDETH_VERSION		"0.47"
+#define FORCEDETH_VERSION		"0.48"
 #define DRV_NAME			"forcedeth"
 
 #include <linux/module.h>
@@ -871,8 +872,8 @@
 		} else {
 			skb = np->rx_skbuff[nr];
 		}
-		np->rx_dma[nr] = pci_map_single(np->pci_dev, skb->data, skb->len,
-						PCI_DMA_FROMDEVICE);
+		np->rx_dma[nr] = pci_map_single(np->pci_dev, skb->data,
+					skb->end-skb->data, PCI_DMA_FROMDEVICE);
 		if (np->desc_ver == DESC_VER_1 || np->desc_ver == DESC_VER_2) {
 			np->rx_ring.orig[nr].PacketBuffer = cpu_to_le32(np->rx_dma[nr]);
 			wmb();
@@ -999,7 +1000,7 @@
 		wmb();
 		if (np->rx_skbuff[i]) {
 			pci_unmap_single(np->pci_dev, np->rx_dma[i],
-						np->rx_skbuff[i]->len,
+						np->rx_skbuff[i]->end-np->rx_skbuff[i]->data,
 						PCI_DMA_FROMDEVICE);
 			dev_kfree_skb(np->rx_skbuff[i]);
 			np->rx_skbuff[i] = NULL;
@@ -1334,7 +1335,7 @@
 		 * the performance.
 		 */
 		pci_unmap_single(np->pci_dev, np->rx_dma[i],
-				np->rx_skbuff[i]->len,
+				np->rx_skbuff[i]->end-np->rx_skbuff[i]->data,
 				PCI_DMA_FROMDEVICE);
 
 		{
@@ -2455,7 +2456,7 @@
 		np->txrxctl_bits |= NVREG_TXRXCTL_RXCHECK;
 		dev->features |= NETIF_F_HW_CSUM | NETIF_F_SG;
 #ifdef NETIF_F_TSO
-		dev->features |= NETIF_F_TSO;
+		/* disabled dev->features |= NETIF_F_TSO; */
 #endif
  	}
 

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Jeff Garzik]

Manfred Spraul wrote:
> Two critical bugs were found in forcedeth 0.47:
> - TSO doesn't work.
> - pci_map_single() for the rx buffers is called with size==0. This bug 
> is critical, it causes random memory corruptions on systems with an iommu.
> 
> Below is a minimal fix for both bugs, for inclusion into 2.6.15.
> TSO will be fixed properly in the next version.
> Tested on x86-64.
> 
> Signed-Off-By: Manfred Spraul <manfred@colorfullife.com>

1) Why does forcedeth require a non-standard calculation for each 
pci_map_single() call?

2) I have requested multiple times that you avoid MIME...

3) Why disable TSO completely?  It sounds like it should default to off, 
then permit enabling via ethtool.

	Jeff

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Manfred Spraul]

Jeff Garzik wrote:

> Manfred Spraul wrote:
>
>> Two critical bugs were found in forcedeth 0.47:
>> - TSO doesn't work.
>> - pci_map_single() for the rx buffers is called with size==0. This 
>> bug is critical, it causes random memory corruptions on systems with 
>> an iommu.
>>
>> Below is a minimal fix for both bugs, for inclusion into 2.6.15.
>> TSO will be fixed properly in the next version.
>> Tested on x86-64.
>>
>> Signed-Off-By: Manfred Spraul <manfred@colorfullife.com>
>
>
> 1) Why does forcedeth require a non-standard calculation for each 
> pci_map_single() call?
>
- skb->len is the wrong thing (tm), since it's 0 until skb_put().
- I have not found a field that contains the actual size of the data 
area of an skb.
- the results must be identical for map and unmap.
- I could recalculate the size of the allocation from np->rx_buf_sz, but 
I don't like that. Right now it would work, but it's too subtile that 
changing rx_buf_sz while there are outstanding rx buffers results in a 
iommu memory leak.
Therefore I decided to calculate the mapping size with "skb->end - 
skb->data": The size of the mapping for an skb is calculated by looking 
at fields in the skb, no knowledge about driver fields.

> 2) I have requested multiple times that you avoid MIME...
>
It's the first time that you complain about Content-Transfer-Encoding: 
7bit attachments.

> 3) Why disable TSO completely?  It sounds like it should default to 
> off, then permit enabling via ethtool.
>
The bugfix is in 0.49 - it's just a bit larger, I would consider it for 
2.5.16.

--
    Manfred

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Linus Torvalds]

On Sat, 24 Dec 2005, Manfred Spraul wrote:
>
> Two critical bugs were found in forcedeth 0.47:
> - TSO doesn't work.
> - pci_map_single() for the rx buffers is called with size==0. This bug is
> critical, it causes random memory corruptions on systems with an iommu.

Good catch. Btw, should we perhaps disallow (or at least WARN_ON()) 
pci_map_single() with a length of zero? I think it's always likely a bug..

However, that

	"skb->end - skb->data"

calculation is a bit strange. It correctly maps the whole skb, but 
wouldn't it make more sense to use the length we actually tell the card to 
use? 

In other words, wouldn't it be a whole lot more sensible and logical to 
use

	np->rx_buf_sz

instead? That's the value we use for allocation and that's the size we 
tell the card we have.

Of course, on the alloc path, it seems to add an additional 
"NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.

		Linus

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Manfred Spraul]

Linus Torvalds wrote:

>Of course, on the alloc path, it seems to add an additional 
>"NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.
>
>  
>
The problem is the pci_unmap_single() call that happens during 
nv_close() or the rx interrupt handler. I think it makes more sense to 
rely on fields in the individual skb instead of reading from 
np->rx_buf_sz. If np->rx_buf_sz changes inbetween, then we have a memory 
leak.

--
    Manfred

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Linus Torvalds]

On Sat, 24 Dec 2005, Manfred Spraul wrote:
> 
> > 2) I have requested multiple times that you avoid MIME...
> 
> It's the first time that you complain about Content-Transfer-Encoding: 7bit
> attachments.

These proper text encodings are easy to _apply_, because the raw email is 
uncorrupted. 

However, attachments are still broken for a very fundamental reason: 
basically no email client will ever quote them on replies. Which means 
that if somebody has commentary about some specific part of the patch, the 
attachement is _totally_ the wrong thing to do.

In other words, there's a reason I encourage people VERY STRONGLY to use 
in-line patches. If you have a broken mailer that corrupts whitespace, 
please just fix it.

		Linus

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Jeff Garzik]

Linus Torvalds wrote:
> However, that
> 
> 	"skb->end - skb->data"
> 
> calculation is a bit strange. It correctly maps the whole skb, but 

nod


> wouldn't it make more sense to use the length we actually tell the card to 
> use? 
> 
> In other words, wouldn't it be a whole lot more sensible and logical to 
> use
> 
> 	np->rx_buf_sz
> 
> instead? That's the value we use for allocation and that's the size we 
> tell the card we have.

That's the sort of thing I prefer.


> Of course, on the alloc path, it seems to add an additional 
> "NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.

None of the other ethernet drivers do 'end - data', which is why I 
hesitated quite a bit on this change.

	Jeff

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Linus Torvalds]

On Sat, 24 Dec 2005, Manfred Spraul wrote:

> Linus Torvalds wrote:
> 
> > Of course, on the alloc path, it seems to add an additional
> > "NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.
>
> The problem is the pci_unmap_single() call that happens during nv_close() or
> the rx interrupt handler. I think it makes more sense to rely on fields in the
> individual skb instead of reading from np->rx_buf_sz. If np->rx_buf_sz changes
> inbetween, then we have a memory leak.

Fair enough. Patch applied.

		Linus

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Sat, 24 Dec 2005, Manfred Spraul wrote:
> 
> 
>>Linus Torvalds wrote:
>>
>>
>>>Of course, on the alloc path, it seems to add an additional
>>>"NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.
>>
>>The problem is the pci_unmap_single() call that happens during nv_close() or
>>the rx interrupt handler. I think it makes more sense to rely on fields in the
>>individual skb instead of reading from np->rx_buf_sz. If np->rx_buf_sz changes
>>inbetween, then we have a memory leak.
> 
> 
> Fair enough. Patch applied.

Paranoia -- the situation above never occurs.  It is coded as are other 
drivers:  np->rx_buf_sz only changes in ->change_mtu(), which (a) is 
serialized against close and (b) always stops the engine and drains RX 
skbs before changing the size.

So can we please remove the subtraction code now added to the hot path? 
  If not now, for 2.6.16?

	Jeff

Re: [PATCH] forcedeth: fix random memory scribbling bug

[Francois Romieu]

Jeff Garzik <jgarzik@pobox.com> :
[...]
> Paranoia -- the situation above never occurs.  It is coded as are other 
> drivers:  np->rx_buf_sz only changes in ->change_mtu(), which (a) is 
> serialized against close and (b) always stops the engine and drains RX 
> skbs before changing the size.

Yep.

Btw, regarding the "more sense" thing, now:
- pci_{map/unmap}_single() uses skb->foo
- nv_alloc_rx() and friends use np->rx_buf_sz

(thread moved to netdev@vger.kernel.org)

--
Ueimor