netdev - Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info

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]

Date:   Thu, 1 Jun 2017 07:29:15 +0200
From:   Michael Büsch <m@...s.ch>
To:     Kalle Valo <kvalo@...eaurora.org>
Cc:     Jia-Ju Bai <baijiaju1990@....com>, netdev@...r.kernel.org,
        linux-wireless@...r.kernel.org, b43-dev@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Larry.Finger@...inger.net
Subject: Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in
 b43legacy_op_bss_info_changed

On Thu, 01 Jun 2017 07:27:20 +0300
Kalle Valo <kvalo@...eaurora.org> wrote:

> Michael Büsch <m@...s.ch> writes:
> 
> >> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct ieee80211_hw *hw,
> >> >  	b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >> >  
> >> >  	if (changed & BSS_CHANGED_BSSID) {
> >> > +		spin_unlock_irqrestore(&wl->irq_lock, flags);
> >> >  		b43legacy_synchronize_irq(dev);
> >> > +		spin_lock_irqsave(&wl->irq_lock, flags);    
> >> 
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>   
> >
> >
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.  
> 
> Sure, but IMHO in general I think the practise of releasing the lock
> like this in a middle of function is dangerous as one can easily miss
> that upper and lower halves of the function are not actually atomic
> anymore. And in this case that it's under a conditional makes it even
> worse.
> 


Yes in general I agree. Releasing and re-acquiring a lock is dangerous.
But I think in this special case here it might be harmless.
The irq_lock is used mostly (if not exclusively; I don't fully
remember) to protect against the IRQ top and bottom half.
But we disabled the device IRQs a line above and the purpose of this
synchronize is to make sure the handler will finish and thus make
dropping the lock save.
Of course it does not make sense to do this with the lock held :)

-- 
Michael

Content of type "application/pgp-signature" skipped