netdev - Re: [PATCH net] net: dsa: skb_put

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:   Mon, 21 Aug 2017 15:24:55 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Woojung.Huh@...rochip.com, netdev@...r.kernel.org
Cc:     davem@...emloft.net, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com
Subject: Re: [PATCH net] net: dsa: skb_put_padto() already frees nskb

On 08/21/2017 03:15 PM, Woojung.Huh@...rochip.com wrote:
> Florian,
> 
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@...il.com]
>> Sent: Monday, August 21, 2017 3:42 PM
>> To: netdev@...r.kernel.org
>> Cc: davem@...emloft.net; andrew@...n.ch;
>> vivien.didelot@...oirfairelinux.com; Woojung Huh - C21699; Florian Fainelli
>> Subject: [PATCH net] net: dsa: skb_put_padto() already frees nskb
>>
>> skb_put_padto() already frees the passed sk_buff reference upon error,
>> so calling kfree_skb() on it again is not necessary.
>>
>> Detected by CoverityScan, CID#1416687 ("USE_AFTER_FREE")
>>
>> Fixes: e71cb9e00922 ("net: dsa: ksz: fix skb freeing")
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>  net/dsa/tag_ksz.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
>> index de66ca8e6201..107172c82107 100644
>> --- a/net/dsa/tag_ksz.c
>> +++ b/net/dsa/tag_ksz.c
>> @@ -60,10 +60,8 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb,
>> struct net_device *dev)
>>  					 skb_transport_header(skb) - skb-
>>> head);
>>  		skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len));
>>
>> -		if (skb_put_padto(nskb, nskb->len + padlen)) {
>> -			kfree_skb(nskb);
>> +		if (skb_put_padto(nskb, nskb->len + padlen))
>>  			return NULL;
>> -		}
>>
>>  		kfree_skb(skb);
>>  	}
>> --
> 
> Because skb_put_padto() frees skb when it fails,  below lines in e71cb9e00922
> ("net: dsa: ksz: fix skb freeing") will be an issue to.
> 
> 	if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) {
> +		if (skb_put_padto(skb, skb->len + padlen))
> +			return NULL;
> +
> 
> When it fails skb will be freed twice in skb_put_padto() and
> caller of dsa_slave_xmit().

You are right, I am not sure what is the best way to fix tag_ksz.c other
than somehow open coding skb_put_padto() minus the freeing on error part?
-- 
Florian