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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 Dec 2009 10:35:11 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	François romieu <romieu@...eil.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	eric.dumazet@...il.com, nhorman@...hat.com
Subject: Re: [PATCH RFC] r8169: straighten out overlength frame detection

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