netdev - Re: [Patch net] net: fix dev_ifsioc

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]

Date:   Fri, 29 Jan 2021 10:00:07 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Cong Wang <cong.wang@...edance.com>,
        "Gong, Sishuai" <sishuai@...due.edu>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [Patch net] net: fix dev_ifsioc_locked() race condition

On Thu, 28 Jan 2021 21:47:04 -0800 Cong Wang wrote:
> On Thu, Jan 28, 2021 at 9:21 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > On Thu, 28 Jan 2021 21:08:05 -0800 Cong Wang wrote:  
> > > On Thu, Jan 28, 2021 at 12:55 PM Jakub Kicinski <kuba@...nel.org> wrote:  
> > > >
> > > > On Sat, 23 Jan 2021 17:30:49 -0800 Cong Wang wrote:  
> > > > > From: Cong Wang <cong.wang@...edance.com>
> > > > >
> > > > > dev_ifsioc_locked() is called with only RCU read lock, so when
> > > > > there is a parallel writer changing the mac address, it could
> > > > > get a partially updated mac address, as shown below:
> > > > >
> > > > > Thread 1                      Thread 2
> > > > > // eth_commit_mac_addr_change()
> > > > > memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
> > > > >                               // dev_ifsioc_locked()
> > > > >                               memcpy(ifr->ifr_hwaddr.sa_data,
> > > > >                                       dev->dev_addr,...);
> > > > >
> > > > > Close this race condition by guarding them with a RW semaphore,
> > > > > like netdev_get_name(). The writers take RTNL anyway, so this
> > > > > will not affect the slow path.
> > > > >
> > > > > Fixes: 3710becf8a58 ("net: RCU locking for simple ioctl()")
> > > > > Reported-by: "Gong, Sishuai" <sishuai@...due.edu>
> > > > > Cc: Eric Dumazet <eric.dumazet@...il.com>
> > > > > Signed-off-by: Cong Wang <cong.wang@...edance.com>  
> > > >
> > > > The addition of the write lock scares me a little for a fix, there's a
> > > > lot of code which can potentially run under the callbacks and notifiers
> > > > there.
> > > >
> > > > What about using a seqlock?  
> > >
> > > Actually I did use seqlock in my initial version (not posted), it does not
> > > allow blocking inside write_seqlock() protection, so I have to change
> > > to rwsem.  
> >
> > Argh, you're right. No way we can construct something that tries to
> > read once and if it fails falls back to waiting for RTNL?  
> 
> I don't think there is any way to tell whether the read fails, a partially
> updated address can not be detected without additional flags etc..

Let me pseudo code it, I can't English that well:

void reader(obj)
{
	unsigned int seq;

	seq = READ_ONCE(seqcnt);
	if (seq & 1)
		goto slow_path;
	smb_rmb();

	obj = read_the_thing();

	smb_rmb();
	if (seq == READ_ONCE(seqcnt))
		return;

slow_path:
	rtnl_lock();
	obj = read_the_thing();
	rtnl_unlock();
}

void writer()
{
	ASSERT_RNTL();

	seqcnt++;
	smb_wmb();

	modify_the_thing();

	smb_wmb();
	seqcnt++;
}


I think raw_seqcount helpers should do here?

> And devnet_rename_sem is already there, pretty much similar to this
> one.

Ack, I don't see rename triggering cascading notifications, tho.
I think you've seen the recent patch for locking in team, that's 
pretty much what I'm afraid will happen here. 

But if I'm missing something about the seqcount or you strongly prefer
the rwlock, we can do that, too. Although I'd rather take this patch 
to net-next in that case.