netdev - Re: [EXT] [PATCH] aquantia: Reserve space when allocating an SKB

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, 19 Nov 2020 00:14:05 +0000
From:   "Ramsay, Lincoln" <Lincoln.Ramsay@...i.com>
To:     Igor Russkikh <irusskikh@...vell.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Dmitry Bogdanov <dbogdanov@...vell.com>
Subject: Re: [EXT] [PATCH] aquantia: Reserve space when allocating an SKB

Hi Igor,

> Here I understand your intention. You are trying to "offset" the placement of
> the packet data, and the restore it back when construction SKB.

Originally, I just added the skb_reserve call, but that broke everything. When I looked at what the igb driver was doing, this approach seemed reasonable but I wasn't sure it'd work.

> The problem however is that hardware is being programmed with fixed descriptor
> size for placement. And its equal to AQ_CFG_RX_FRAME_MAX (2K by default).
> 
> This means, HW will do writes of up to 2K packet data into a single
> descriptor, and then (if not enough), will go for next descriptor data.
> 
> With your solution, packets of size (AQ_CFG_RX_FRAME_MAX - AQ_SKB_PAD) up to
> size of AQ_CFG_RX_FRAME_MAX will overwrite the area of page they designated
> to. Ultimately, HW will do a memory corruption of next page.

Yeah... this is the kind of thing I was worried about. It seemed to me that the SKB was being built around a hardware buffer rather than around heap-allocated memory. I just hoped that the rx_off value would somehow make it work.

The code in aq_get_rxpages seems to suggest that multiple frames can fit in a rxpage, so maybe the logic there prevents overwriting? (at the expense of not fitting as many frames into the page before it has to get a new one?)

I didn't notice any issues when I was testing, but apart from port forwarding ssh (which is tiny) and some copying of files on (probably not even close to saturating the link) there's not a huge network load placed on the device. I guess it's entirely possible that an overwrite problem would only show up under heavy load? (ie. more, and larger amounts of data in flight through the kernel at once)

> I think the only acceptable solution here would be removing that optimized
> path of build_skb, and keep only napi_alloc_skb. Or, we can think of keeping
> it under some configuration condition (which is also not good).

I'll attempt to confirm that this works too, at least for our tests :)

Lincoln