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

[Neil Horman <nhorman@tuxdriver.com>]

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