netdev - RE: [PATCH] qlcnic: dont assume NET_IP

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]

Message-ID: <99737F4847ED0A48AECC9F4A1974A4B80F86F80082@MNEXMB2.qlogic.org>
Date:	Fri, 17 Sep 2010 05:53:03 -0500
From:	Amit Salecha <amit.salecha@...gic.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Ameen Rahman <ameen.rahman@...gic.com>,
	Anirban Chakraborty <anirban.chakraborty@...gic.com>,
	David Miller <davem@...emloft.net>
Subject: RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2



> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> Sent: Friday, September 17, 2010 3:27 PM
> To: Amit Salecha
> Cc: netdev@...r.kernel.org; Ameen Rahman; Anirban Chakraborty; David
> Miller
> Subject: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> Amit, I noticed following bug in qlnic driver.
> 
> Also, skb->truesize should not be changed by drivers, unless dealing
> with fragments.
> 
> When you have :
> 	skb->truesize = skb->len + sizeof(struct sk_buff);
> 
> you are lying to stack that can queue many "small" UDP packets,
> accouting for small packets in socket rcvbuf, while the truesize was
> really 1532 + sizeof(struct sk_buff)
> 
> Could you take a look ?

I will take a look and get back to you.

- Thanks.

> 
> Thanks
> 
> 
> [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> qlcnic driver allocates rx skbs and gives to hardware too bytes of
> extra
> storage, allowing for corruption of kernel data.
> 
> NET_IP_ALIGN being 0 on some platforms (including x86), drivers should
> not assume it's 2.
> 
> rds_ring->skb_size = rds_ring->dma_size + NET_IP_ALIGN;
> ...
> skb = dev_alloc_skb(rds_ring->skb_size);
> skb_reserve(skb, 2);
> pci_map_single(pdev, skb->data, rds_ring->dma_size,
> PCI_DMA_FROMDEVICE);
> 
> (and rds_ring->skb_size == rds_ring->dma_size) -> bug
> 
> 
> Because of extra alignment (1500 + 32) -> four extra bytes are
> available
> before the struct skb_shared_info, so corruption is not noticed.
> 
> Note: this driver could use netdev_alloc_skb_ip_align()
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> ---
> diff --git a/drivers/net/qlcnic/qlcnic_init.c
> b/drivers/net/qlcnic/qlcnic_init.c
> index 75ba744..60ab753 100644
> --- a/drivers/net/qlcnic/qlcnic_init.c
> +++ b/drivers/net/qlcnic/qlcnic_init.c
> @@ -1316,7 +1316,7 @@ qlcnic_alloc_rx_skb(struct qlcnic_adapter
> *adapter,
>  		return -ENOMEM;
>  	}
> 
> -	skb_reserve(skb, 2);
> +	skb_reserve(skb, NET_IP_ALIGN);
> 
>  	dma = pci_map_single(pdev, skb->data,
>  			rds_ring->dma_size, PCI_DMA_FROMDEVICE);
> 
>