netdev - Re: [PATCH net] af_key: free SKBs under RCU protection

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]

Message-ID: <a1b484c544c9f631246374ccf09e491f@codeaurora.org>
Date:   Fri, 21 Sep 2018 11:09:05 -0600
From:   stranche@...eaurora.org
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org, steffen.klassert@...unet.com
Subject: Re: [PATCH net] af_key: free SKBs under RCU protection

>> 
>> As long as one skb has sock_rfree has its destructor, the socket 
>> attached to
>> this skb can not be released. There is no race here.
>> 
>> Note that skb_clone() does not propagate the destructor.
>> 
>> The issue here is that in the rcu lookup, we can find a socket that 
>> has been
>> dismantled, with a 0 refcount.
>> 
>> We must not use sock_hold() in this case, since we are not sure the 
>> socket refcount is not already 0.
>> 
>> pfkey_broadcast() and pfkey_broadcast_one() violate basic RCU rules.
>> 
>> When in a RCU lookup, one want to increment an object refcount, it 
>> needs
>> to be extra-careful, as I did in my proposal.
>> 
>> Note that the race could be automatically detected with 
>> CONFIG_REFCOUNT_FULL=y
> 
> Bug was added in commit 7f6b9dbd5afb ("af_key: locking change")

Hi Eric,

I tried your refcount idea below, but it still results in the same 
crash.

>>>> --- a/net/key/af_key.c
>>>> +++ b/net/key/af_key.c
>>>> @@ -201,7 +201,9 @@ static int pfkey_broadcast_one(struct sk_buff
>>>> *skb, struct sk_buff **skb2,
>>>>  {
>>>>         int err = -ENOBUFS;
>>>> 
>>>> -       sock_hold(sk);
>>>> +       if (!refcount_inc_not_zero(&sk->sk_refcnt))
>>>> +               return -ENOENT;
>>>> +
>>>>         if (*skb2 == NULL) {
>>>>                 if (refcount_read(&skb->users) != 1) {
>>>>                         *skb2 = skb_clone(skb, allocation);

I also tried reverting 7f6b9dbd5afb ("af_key: locking change") and 
running the
test there and I still see the crash, so it doesn't seem to be an RCU 
specific
issue.

Is there anything else that could be causing this?