netdev - Re: [BUG] intersil: p54: possible deadlock in p54_remove_interface() and p54

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 7 Feb 2022 22:13:08 +0100
From:   Christian Lamparter <chunkeey@...il.com>
To:     Jia-Ju Bai <baijiaju1990@...il.com>, chunkeey@...glemail.com,
        kvalo@...nel.org, davem@...emloft.net, kuba@...nel.org
Cc:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [BUG] intersil: p54: possible deadlock in p54_remove_interface()
 and p54_stop()

Hi,

On 07/02/2022 16:31, Jia-Ju Bai wrote:
> Hello,
> 
> My static analysis tool reports a possible deadlock in the p54 driver in Linux 5.16:
> 
> p54_remove_interface()
>    mutex_lock(&priv->conf_mutex); --> Line 262 (Lock A)
> wait_for_completion_interruptible_timeout(&priv->beacon_comp, HZ); --> Line 271 (Wait X)
> 
> p54_stop()
>    mutex_lock(&priv->conf_mutex); --> Line 208 (Lock A)
>    p54p_stop() (call via priv->stop)
>      p54_free_skb()
>        p54_tx_qos_accounting_free()
>          complete(&priv->beacon_comp); --> Line 230 (Wake X)
> 
> When p54_remove_interface() is executed, "Wait X" is performed by holding "Lock A". 
> If p54_stop() is executed at this time, "Wake X" cannot be performed to wake 
> up "Wait X" in p54_remove_interface(), because "Lock A" has been already hold by
> p54_remove_interface(), causing a possible deadlock.
>
> I find that "Wait X" is performed with a timeout, to relieve the possible deadlock;
> but I think this timeout can cause inefficient execution.
> 
> I am not quite sure whether this possible problem is real and how to fix it if it is real.
> Any feedback would be appreciated, thanks :)

This has been such a long long time ago. But I think I found
the right documentation entry for you:

<https://www.kernel.org/doc/html/v5.16/driver-api/80211/mac80211.html> (scroll down a bit)
| remove_interface
| 	Notifies a driver that an interface is going down.
| 	>>The stop callback is called after this if it is the last interface
|	and no monitor interfaces are present.<<
|	(it goes on a bit. But I don't think there's anything important left)

The documentation tells you not to worry. p54_stop() and p54_remove_interface()
are being serialized by mac80211.

Cheers,
Christian