netdev - Re: [PATCH net] tipc: fix missing rtnl lock protection during setting link properties

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b6ec84e9-b791-c953-a61b-edc125a81add@windriver.com>
Date:   Thu, 4 Jan 2018 15:30:52 +0800
From:   Ying Xue <ying.xue@...driver.com>
To:     David Miller <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <jon.maloy@...csson.com>,
        <syzkaller-bugs@...glegroups.com>,
        <tipc-discussion@...ts.sourceforge.net>
Subject: Re: [PATCH net] tipc: fix missing rtnl lock protection during setting
 link properties

On 01/03/2018 11:48 PM, David Miller wrote:
> As soon as you drop the RTNL lock, the media or bearer entry can be
> removed from the tables.
> 

Thanks for the review. Yes, you are right. But even if we temporarily
release RTNL lock, it's still safe for us because when we set
media/bearer properties in __tipc_nl_compat_doit(), tipc_nl_media_set()
and tipc_nl_bearer_set() will probe media or bearer again within RTNL
lock protection.

> This invalidates what you do next, whether it's
> tipc_nl_compat_media_set(), tipc_nl_compat_bearer_set(), etc.

In fact tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set() don't
really change media or bearer's properties, instead they only format the
contents pointed by their "msg" parameter.

> 
> Therefore, you have to lock down the tipc configuration state around
> this entire operation, from media/bearer probe to the building of the
> netlink message(s).
> 

Sorry, we cannot hold RTNL lock in the entire operation path because
TIPC now supports two different sets of netlink APIs:

One set of API's execution path:

genl_family_rcv_msg()
  tipc_nl_media_set()
    rtnl_lock()
    tipc_media_find()
    //set media properties

genl_family_rcv_msg()
  tipc_nl_bearer_set()
    rtnl_lock()
    tipc_bearer_find()
    //set bearer properties

Another set of API's execution path:

genl_family_rcv_msg()
  tipc_nl_compat_recv()
    tipc_nl_compat_handle net()
      __tipc_nl_compat_doit net()
        tipc_nl_compat_link_set()
        tipc_nl_media_set()

genl_family_rcv_msg()
  tipc_nl_compat_recv()
    tipc_nl_compat_handle net()
      __tipc_nl_compat_doit net()
        tipc_nl_compat_link_set()
        tipc_nl_bearer_set()

As we see in above call chains, tipc_nl_media_set() and
tipc_nl_bearer_set() are shared by the two sets of netlink APIs. If we
hold RTNL lock from tipc_nl_compat_recv(), it means we cannot directly
call tipc_nl_media_set() or tipc_nl_bearer_set() in
__tipc_nl_compat_doit net().

> Either this entire code path must execute with the bearer/media entry
> present, or without.  If you drop the RTNL mutex in the middle, this
> invariant is not held.