linux-kernel - Re: WARNING in cm109_urb_irq_callback/usb_submit

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]

Message-ID: <07f2ec1a-0363-4734-97ff-a129699b1907@rowland.harvard.edu>
Date: Thu, 20 Mar 2025 13:25:05 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Oliver Neukum <oneukum@...e.com>
Cc: 白烁冉 <baishuoran@...eu.edu.cn>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Kun Hu <huk23@...udan.edu.cn>, Jiaji Qin <jjtan24@...udan.edu.cn>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-input@...r.kernel.org, syzkaller@...glegroups.com
Subject: Re: WARNING in cm109_urb_irq_callback/usb_submit_urb

On Thu, Mar 20, 2025 at 04:42:33PM +0100, Oliver Neukum wrote:
> On 20.03.25 15:25, Alan Stern wrote:
> 
> > This test must itself be subject to the same race, right?  There needs
> > to be some kind of synchronization between the two tasks (i.e., a mutex,
> > spinlock, or something similar).
> 
> Hi,
> 
> there is:
> 
> static void cm109_stop_traffic(struct cm109_dev *dev)
> {
>         dev->shutdown = 1;
>         /*
>          * Make sure other CPUs see this
>          */
>         smp_wmb();
>         usb_kill_urb(dev->urb_ctl);
>         usb_kill_urb(dev->urb_irq);
>         cm109_toggle_buzzer_sync(dev, 0);
>         dev->shutdown = 0;
>         smp_wmb();

I don't know anything about this driver, but the placement of the second 
smp_wmb() looks odd.  Should it really come before the line that sets 
dev->shutdown to 0?  In general, smp_wmb() is used to separate two sets 
of stores; if it comes after all the relevant stores have been performed 
then it won't accomplish anything.

> }
> 
> This driver has a tough job as the two completion
> handlers submitted each other's as well as their own
> URBs based on the data they get.
> That scheme is rather complex, but as far as I can tell correct,
> but you need to test that flag everywhere.

However, it's quite noticeable that the code you want to change in 
cm109_submit_buzz_toggle() doesn't have any memory barriers to pair with 
the smb_wmb()'s above.  Shouldn't there at least be an smp_rmb() after 
you read dev->shutdown?

As long you're updating the synchronization, it might be a good idea to 
also improve the comment describing the memory barriers.  "Make sure 
other CPUs see this" doesn't mean anything -- of course all the other 
CPUs will eventually see the changes made here.  The point is that they 
should see the new value of dev->shutdown before seeing the final 
completion of the two URBs.  Also, the comment should say which other 
memory barriers pair with the ones here.

Alan Stern