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]

Date:	Thu, 16 Jan 2014 11:30:01 -0800
From:	Florian Fainelli <florian@...nwrt.org>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	David Laight <David.Laight@...lab.com>,
	Ben Hutchings <ben@...adent.org.uk>,
	"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

2014/1/16 Ben Hutchings <bhutchings@...arflare.com>:
> 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?

No this driver does not it, your fix padding the SKB to max(skb->len,
ETH_ZLEN) looks good to me. Could you submit these as two separate
patches?

Thanks!
-- 
Florian
--
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