netdev - Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr

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: <cba18775-af46-4ae5-ad29-28687401781b@redhat.com>
Date: Wed, 23 Oct 2024 13:04:58 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Rémi Denis-Courmont <remi@...lab.net>,
 Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for
 getaddr_dumpit().

On 10/19/24 09:48, Rémi Denis-Courmont wrote:
> Le perjantaina 18. lokakuuta 2024, 20.16.29 EEST Kuniyuki Iwashima a écrit :
>> From: "Rémi Denis-Courmont" <remi@...lab.net>
>> Date: Thu, 17 Oct 2024 21:49:18 +0300
>>
>>>> diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
>>>> index 5996141e258f..14928fa04675 100644
>>>> --- a/net/phonet/pn_netlink.c
>>>> +++ b/net/phonet/pn_netlink.c
>>>> @@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32
>>>> ifindex, u8 addr,
>>>>
>>>>  static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback
>>>>  *cb)
>>>>
>>>> {
>>>> +	int addr_idx = 0, addr_start_idx = cb->args[1];
>>>> +	int dev_idx = 0, dev_start_idx = cb->args[0];
>>>>
>>>>  	struct phonet_device_list *pndevs;
>>>>  	struct phonet_device *pnd;
>>>>
>>>> -	int dev_idx = 0, dev_start_idx = cb->args[0];
>>>> -	int addr_idx = 0, addr_start_idx = cb->args[1];
>>>> +	int err = 0;
>>>>
>>>>  	pndevs = phonet_device_list(sock_net(skb->sk));
>>>>
>>>> +
>>>>
>>>>  	rcu_read_lock();
>>>>  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
>>>>
>>>> +		DECLARE_BITMAP(addrs, 64);
>>>>
>>>>  		u8 addr;
>>>>  		
>>>>  		if (dev_idx > dev_start_idx)
>>>>
>>>> @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb,
>>>> struct
>>>> netlink_callback *cb) continue;
>>>>
>>>>  		addr_idx = 0;
>>>>
>>>> -		for_each_set_bit(addr, pnd->addrs, 64) {
>>>> +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
>>>
>>> Is that really safe? Are we sure that the bit-field writers are atomic
>>> w.r.t. memcpy() on all platforms? If READ_ONCE is needed for an integer,
>>> using memcpy() seems sketchy, TBH.
>>
>> I think bit-field read/write need not be atomic here because even
>> if a data-race happens, for_each_set_bit() iterates each bit, which
>> is the real data, regardless of whether data-race happened or not.
> 
> Err, it looks to me that a corrupt bit would lead to the index getting corrupt 
> and addresses getting skipped or repeated. AFAICT, the RTNL lock is still 
> needed here.

To wrap-up Kuniyuki's reply: addresses can't be repeated in dump. They
can be 'skipped' meaning the dump can race with writer reading an 'old'
address bitmask, still not containing the 'new' address. Exactly as
could happen with racing dump/writer both protected by the lock.

The bottom line is that this code looks safe to me.

Thanks,

Paolo