netdev - Re: [PATCH net] netlink: fix potential deadlock in netlink_set

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: <ad471e9fe6a9b3812497c40456cba6e0c8a152ee.camel@sipsolutions.net>
Date: Thu, 22 Jun 2023 10:25:57 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Eric Dumazet <edumazet@...gle.com>, Jiri Pirko <jiri@...nulli.us>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
 "eric.dumazet@...il.com" <eric.dumazet@...il.com>, 
 "syzbot+a7d200a347f912723e5c@...kaller.appspotmail.com"
 <syzbot+a7d200a347f912723e5c@...kaller.appspotmail.com>
Subject: Re: [PATCH net] netlink: fix potential deadlock in netlink_set_err()

On Thu, 2023-06-22 at 08:14 +0000, Eric Dumazet wrote:
> On Thu, Jun 22, 2023 at 10:04 AM Jiri Pirko <jiri@...nulli.us> wrote:
> > 
> > Wed, Jun 21, 2023 at 05:43:37PM CEST, edumazet@...gle.com wrote:
> > > syzbot reported a possible deadlock in netlink_set_err() [1]
> > > 
> > > A similar issue was fixed in commit 1d482e666b8e ("netlink: disable IRQs
> > > for netlink_lock_table()") in netlink_lock_table()
> > > 
> > > This patch adds IRQ safety to netlink_set_err() and __netlink_diag_dump()
> > > which were not covered by cited commit.
> > > 
> > > [1]
> > > 
> > > WARNING: possible irq lock inversion dependency detected
> > > 6.4.0-rc6-syzkaller-00240-g4e9f0ec38852 #0 Not tainted
> > > 
> > > syz-executor.2/23011 just changed the state of lock:
> > > ffffffff8e1a7a58 (nl_table_lock){.+.?}-{2:2}, at: netlink_set_err+0x2e/0x3a0 net/netlink/af_netlink.c:1612
> > > but this lock was taken by another, SOFTIRQ-safe lock in the past:
> > > (&local->queue_stop_reason_lock){..-.}-{2:2}
> > > 
> > > and interrupts could create inverse lock ordering between them.
> > > 
> > > other info that might help us debug this:
> > > Possible interrupt unsafe locking scenario:
> > > 
> > >       CPU0                    CPU1
> > >       ----                    ----
> > >  lock(nl_table_lock);
> > >                               local_irq_disable();
> > >                               lock(&local->queue_stop_reason_lock);
> > >                               lock(nl_table_lock);
> > >  <Interrupt>
> > >    lock(&local->queue_stop_reason_lock);
> > > 
> > > *** DEADLOCK ***
> > > 
> > > Fixes: 1d482e666b8e ("netlink: disable IRQs for netlink_lock_table()")
> > 
> > I don't think that this "fixes" tag is correct. The referenced commit
> > is a fix to the same issue on a different codepath, not the one who
> > actually introduced the issue.
> > 
> > The code itself looks fine to me.
> 
> Note that the 1d482e666b8e had no Fixes: tag, otherwise I would have taken it.
> 
> I presume that it would make no sense to backport my patch on stable branches
> if the cited commit was not backported yet.

I'd tend to even say it doesn't make sense to backport this at all, it's
very unlikely to happen in practice since that code path ...

> Now, if you think we can be more precise, I will let Johannes do the
> archeology in ieee80211 code.

I first thought that'd be commit d4fa14cd62bd ("mac80211: use
ieee80211_free_txskb in a few more places") then, but that didn't call
to netlink yet ... so commit 8a2fbedcdc9b ("mac80211: combine
status/drop reporting"), but that's almost as old (and really old too,
kernel 3.8).

But again, I'm not sure it's worth worrying about ... Actually I'm
pretty sure it's _not_ worth worrying about :)

johannes