On 25-4-2017 11:36, James Hughes wrote:
> On 25 April 2017 at 10:10, Arend van Spriel
> <arend.vanspriel@xxxxxxxxxxxx> 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 <arend.vanspriel@xxxxxxxxxxxx>
>> ---
>> 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
