netdev - Re: [PATCH net-next] net/sched: unregister lockdep keys in qdisc_create/qdisc

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: <ZjTcNVOT9x8e4UG3@dcaratti.users.ipa.redhat.com>
Date: Fri, 3 May 2024 14:44:37 +0200
From: Davide Caratti <dcaratti@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Naresh Kamboju <naresh.kamboju@...aro.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net/sched: unregister lockdep keys in
 qdisc_create/qdisc_alloc error path

hello Eric,

On Tue, Apr 30, 2024 at 08:43:22PM +0200, Eric Dumazet wrote:
> On Tue, Apr 30, 2024 at 8:35 PM Davide Caratti <dcaratti@...hat.com> wrote:
> >

[...]

> > > For consistency with the other path, what about this instead ?
> > >
> > > This would also  allow a qdisc goten from an rcu lookup to allow its
> > > spinlock to be acquired.
> > > (I am not saying this can happen, but who knows...)
> > >
> > > Ie defer the  lockdep_unregister_key() right before the kfree()
> >
> > the problem is, qdisc_free() is called also in a RCU callback. So, if we move
> > lockdep_unregister_key() inside the function, the non-error path is
> > going to splat like this
> 
> Got it, but we do have ways of running a work queue after rcu grace period.

this would imply scheduling a work that does qdisc_free() + lockdep_unregister_key()
in qdisc_free_cb(). I can try that, but maybe the issue is different:

> Let's use your patch, but I suspect we could have other issues.
> 
> Full disclosure, I have the following syzbot report:
> 
> WARNING: bad unlock balance detected!
> 6.9.0-rc5-syzkaller-01413-gdd1941f801bc #0 Not tainted
> -------------------------------------
> kworker/u8:6/2474 is trying to release lock (&sch->root_lock_key) at:
> [<ffffffff897300c5>] spin_unlock_bh include/linux/spinlock.h:396 [inline]
> [<ffffffff897300c5>] dev_reset_queue+0x145/0x1b0 net/sched/sch_generic.c:1304
> but there are no more locks to release!

I don't understand how can this "imbalance" be caused by lockdep_unregister_key()
being called too early. I'm more inclined to think that this splat is due to UaF
similar to those that we saw a couples of days ago. Is syzbot still
generating report like the one above?

thanks,

-- 
davide