On 25 April 2017 at 12:10, Arend Van Spriel <[email protected]> wrote: > On 25-4-2017 11:36, James Hughes wrote: >> On 25 April 2017 at 10:10, Arend van Spriel >> <[email protected]> wrote: >>> An issue was found brcmfmac driver in which a skbuff in .start_xmit() >>> callback was actually cloned. So instead of checking for sufficient >>> headroom it should also be writable. Hence use skb_cow_head() to >>> check and expand the headroom appropriately. >>> >>> Signed-off-by: Arend van Spriel <[email protected]> >>> --- >>> Hi Kalle, >>> >>> Did a recursive grep in drivers/net/wireless and found a similar >>> case in ath6kl. I do not have the hardware to test so this is >>> only compile tested. >>> >>> Regards, >>> Arend >>> --- >>> drivers/net/wireless/ath/ath6kl/txrx.c | 13 ++++--------- >>> 1 file changed, 4 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c >>> b/drivers/net/wireless/ath/ath6kl/txrx.c >>> index a531e0c..e6b2517 100644 >>> --- a/drivers/net/wireless/ath/ath6kl/txrx.c >>> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c >>> @@ -399,15 +399,10 @@ int ath6kl_data_tx(struct sk_buff *skb, struct >>> net_device *dev) >>> csum_dest = skb->csum_offset + csum_start; >>> } >>> >>> - if (skb_headroom(skb) < dev->needed_headroom) { >>> - struct sk_buff *tmp_skb = skb; >>> - >>> - skb = skb_realloc_headroom(skb, >>> dev->needed_headroom); >>> - kfree_skb(tmp_skb); >>> - if (skb == NULL) { >>> - dev->stats.tx_dropped++; >>> - return 0; >>> - } >>> + if (skb_cow_head(skb, dev->needed_headroom)) { >>> + dev->stats.tx_dropped++; >>> + kfree_skb(skb); >>> + return 0; >>> } >>> >>> if (ath6kl_wmi_dix_2_dot3(ar->wmi, skb)) { >>> -- >>> 1.9.1 >>> >> >> Not sure if this is the right place to comment on this, but I've had a >> quick look around various network drivers, and there are similar >> constructs in a LOT of drivers. I've picked two at random, and both >> seem to show this issue. When the issue first came up in a USB >> attached smsc ethernet driver, at least 6 other drivers with similar >> faults were found in the net/usb tree. Now I could just be being >> paranoid, and am missing something, so here are the files I looked >> at... >> >> drivers/net/marvell/mwifiex/uap_txrx.c line 161 - no relevant skb_cow >> operations in this file, but changes are made to the buffers > > This piece of code is used in rx. They have in-driver bridging > implemented in mwifiex. Surprised to see such a feature in a upstream > driver. > >> /drivers/net/ethernet/sun/niu.c line 6657 - ditto > > Looks suspicious indeed. > >> I'm a bit of a beginner at this stuff, so not sure how this should be >> taken forward. > > I looked at the wireless drivers specifically and initial grep was for > skb_push(), but that gave a lot of results. So just did a grep for > drivers touching struct net_device::needed_headroom. Admittedly that is > more of a glance than a proper look and it would probably be best if > driver maintainers would check for such headroom constructs in their > driver(s). > > Regards, > Arend
I only checked those two so I suspect more will be in there. There is also a lot of boilerplate code that could be removed simply by using skb_cow_header...is there a standard way of telling all maintainers to check their drivers for particular issues? I did a grep for skb_headroom since it seems unlikely that would be required except in circumstances like this, to find an initial list of possibilities but don't have time to check all the hits!
