netdev - Re: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

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: <1389893604.11912.47.camel@bwh-desktop.uk.level5networks.com>
Date:	Thu, 16 Jan 2014 17:33:24 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	David Laight <David.Laight@...LAB.COM>
CC:	'Ben Hutchings' <ben@...adent.org.uk>,
	Florian Fainelli <florian@...nwrt.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for
 SKB length checking

On Thu, 2014-01-16 at 09:10 +0000, David Laight wrote:
> From: Ben Hutchings
> > On Wed, 2014-01-15 at 13:04 -0800, Florian Fainelli wrote:
> > > Ever since this driver was merged the following code was included:
> ...
> > > -	if (skb->len < MISR)
> > > -		descptr->len = MISR;
> > > +	if (skb->len < ETH_ZLEN)
> > > +		descptr->len = ETH_ZLEN;
> > 
> > It looks like this is just increasing the TX descriptor length so the
> > packet is tail-padded with whatever happens to come next in the skb.
> > This is absolutely incorrect behaviour and may leak sensitive
> > information.
> 
> And possibly page fault if the data is right at the end of a page.

If the CPU was accessing it, it would be fine as the skb linear area
ends with struct skb_shared_info.  (sfc actually takes advantage of that
in efx_enqueue_skb_pio() which can add padding (*not* transmitted) to
ensure that the CPU does write-combining.)

But the NIC, if it's connected through an IOMMU, might get a page fault
due to the incorrect length of the DMA mapping.

> > Presumably it is necessary to pad the frame because the MAC is too lame
> > to do it, and the correct way to do that is using skb_padto(skb,
> > ETH_ZLEN).  But this may fail as it might have to allocate memory
> 
> Alternatively use two ring entries with the 'more' bit set on
> the first one and transmit the padding from a permanently allocated
> and dma-mapped block of zeros.
> Assuming the hardware support SG transmit and doesn't have a
> constraint on the length of the first fragment.

Do you see NETIF_F_SG in this driver?

Ben.

> Or have a pre-allocated an dma-mapped array of short buffers (one
> for each ring slot) and copy short packets into the array instead
> of dma-mapping the skb data.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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