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: <CANn89iLeU+pBrcHZyQoSRa-X_3G-Y8cjF6FJy4XwkJc7ronqMA@mail.gmail.com>
Date: Thu, 22 Jun 2023 10:14:56 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: 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, eric.dumazet@...il.com, 
	syzbot+a7d200a347f912723e5c@...kaller.appspotmail.com, 
	Johannes Berg <johannes.berg@...el.com>
Subject: Re: [PATCH net] netlink: fix potential deadlock in netlink_set_err()

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.

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