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: <adc98602aba4ca1d3251da8279abbad98ef6a184.camel@sipsolutions.net>
Date: Thu, 22 Jun 2023 10:44:15 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jiri Pirko <jiri@...nulli.us>, "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 10:39 +0200, Eric Dumazet wrote:
> > 
> > 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 :)
> 
> OK, should we revert your patch then ?

No no, I think we need that original commit, and also the fix now.

> I am slightly confused, you are saying this bug is not worth fixing ?

Sorry, yeah, that wasn't really clear. I'm saying it's no worth worrying
about backporting it to stable and really figuring out where it was
introduced, since it's never actually going to happen in practice. We
never even hit the code path that made lockdep show it in any prior
testing with lockdep (which we do too), and actually causing the
deadlock is far, far, less likely since you have to race simultaneously
with an interrupt from the same device (where the netdev down is
happening) doing something to the queue state ... so that already needs
multiple interfaces since one is just going down, etc.

> This will prevent syzbot from discovering other bugs, this is your call.

Oh sure, I agree we should fix it and I think your fix is fine. I'm just
not sure why we're having a discussion about backporting it and what it
really fixes when it's never really going to happen in practice, and
even syzbot couldn't figure out how to reproduce it due to the race
nature of the issue.

johannes