netdev - Re:Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in del

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-next>] [day] [month] [year] [list]

Date:   Tue, 8 Aug 2017 09:10:34 +0800 (CST)
From:   "Gao Feng" <gfree.wind@....163.com>
To:     "Gao Feng" <gfree.wind@....163.com>
Cc:     "Cong Wang" <xiyou.wangcong@...il.com>, xeb@...l.ru,
        "David Miller" <davem@...emloft.net>,
        "Linux Kernel Network Developers" <netdev@...r.kernel.org>
Subject: Re:Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in
 del_chan

At 2017-08-08 01:17:02, "Cong Wang" <xiyou.wangcong@...il.com> wrote:
>On Sun, Aug 6, 2017 at 6:32 PM, Gao Feng <gfree.wind@....163.com> wrote:
>> I think the RCU should be supposed to avoid the race between del_chan and lookup_chan.
>
>More precisely, it is callid_sock which is protected by RCU.
>
>Unless I miss any other code path, pptp_exit_module() is
>problematic too, I don't think it can just vfree() the whole thing.
>
>
>> The synchronize_rcu could make sure if there was one which calls lookup_chan in this period, it would be finished and the sock refcnt is increased if necessary.
>>
>> So I think it is ok to invoke sock_put directly without SOCK_RCU_FREE, because the lookup_chan caller has already hold the sock refcnt,
>>

>

Hi Cong,
I just thought about this issue last night, then I get your this email this morning.

>If you mean the sock_hold() inside lookup_chan(), no,
>it doesn't help because we already dereference the sock
>before it.

>

Sorry, I don't get you clearly. Why the sock_hold() isn't helpful?
The pptp_release invokes synchronize_rcu after del_chan, it could make sure the others has increased the sock refcnt if necessary
and the lookup is over.
There is no one could get the sock after synchronize_rcu in pptp_release.

But I think about another problem.
It seems the pptp_sock_destruct should not invoke del_chan and pppox_unbind_sock.
Because when the sock refcnt is 0, the pptp_release must have be invoked already.

There are two cases totally.
1. when pptp_release invokes sock_put, the refcnt is 0. The del_chan and pppox_unbind_sock are invoked.
2. when pptp_release invokes sock_put, the refcnt is not 0. It means someone holds the sock during the period pptp_release invokes del_chan.
Then someone invokes sock_put and the sock refcnt reach 0, it would invoke sk_free and invokes pptp_sock_destruct.
So it is unnecessary to invoke del_chan and pppox_unbind_sock again.
And it would bring a race issue even if the pptp_sock_destruct invoked del_chan.

If so, I would send another patch for it.

>Also, lookup_chan_dst() does not have a refcnt, I don't
>find any code preventing it deref'ing other sock in callid_sock

>than the calling one.

Sorry, the last email is html format, not text.
So I send it with text format again.

Best Regards
Feng