Re: [PATCH RFC] usb: adutux: fix unlocked read_buffer_length write in adu_open() (data race)

[Greg KH]

On Mon, Jul 13, 2026 at 10:15:05AM +0530, Kanishka De Silva wrote:
> Hi Greg,
> 
> While auditing drivers/usb/misc/adutux.c in Linux 7.1.3, I found that
> adu_open() writes dev->read_buffer_length without holding
> dev->buflock, while every other path that touches this field
> (adu_interrupt_in_callback() and adu_read()) correctly takes buflock
> as documented in the locking comment at the top of the file:
> 
>   * The locking scheme is a vanilla 3-lock:
>   *   adu_device.buflock: A spinlock, covers what IRQs touch.
> 
> adu_open() (drivers/usb/misc/adutux.c:268):
> 
>     /* initialize in direction */
>     dev->read_buffer_length = 0;
> 
> This is reached while holding only adutux_mutex, not buflock. Meanwhile:
> 
> - adu_interrupt_in_callback() (lines 165-196) holds buflock while
> reading and incrementing read_buffer_length from IRQ context.
> - adu_read() (lines 394-404) holds buflock while reading, swapping,
> and zeroing read_buffer_length.
> 
> So two different locks (adutux_mutex vs buflock) are used to protect
> the same field, which is a textbook data race: if an IRQ callback
> increments read_buffer_length concurrently with a re-open() clearing
> it, the increment can be silently lost, corrupting buffer state.
> Repeated open/close under interrupt load can produce stale or dropped
> USB read data from the ADU device.
> 
> I reproduced the race by extracting the exact code paths (locking
> primitives, field layout, and control flow) into a standalone
> userspace program and running it under ThreadSanitizer with pthreads
> simulating the IRQ callback and open() paths concurrently. TSan
> reports the expected data race between the two lock domains on
> read_buffer_length. I'm happy to send the verifier program if useful,
> though I want to be clear it's a userspace reconstruction for
> lock-discipline verification, not the compiled kernel driver itself.
> 
> Suggested fix:
> 
>     static int adu_open(struct inode *inode, struct file *file)
>     {
>     +   unsigned long flags;
>         ...
>     -   dev->read_buffer_length = 0;
>     +   spin_lock_irqsave(&dev->buflock, flags);
>     +   dev->read_buffer_length = 0;
>     +   spin_unlock_irqrestore(&dev->buflock, flags);
> 
> Impact is data corruption / stale reads on the ADU USB device class,
> not memory corruption or privilege escalation, so I'd call this Medium
> severity. Reachable by any unprivileged user who can open
> /dev/usb/adutuxN.
> 
> Happy to send a proper patch with commit message if this looks correct
> to you — let me know if you'd like it in a different form.

Just send a proper patch, you never have to ask if you should do so or
not :)