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: <20241018171629.92709-1-kuniyu@amazon.com>
Date: Fri, 18 Oct 2024 10:16:29 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <remi@...lab.net>
CC: <kuni1840@...il.com>, <netdev@...r.kernel.org>, <kuniyu@...zon.com>
Subject: Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

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.