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

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

[Kanishka De Silva]

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.

Thanks,
Kanishka
HEXER365 / github.com/hexer365

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 :)

[PATCH] usb: adutux: fix unlocked read_buffer_length write in adu_open()

[Kanishka De Silva]

adu_open() clears dev->read_buffer_length while holding only
adutux_mutex, but every other path that touches this field
(adu_interrupt_in_callback() and adu_read()) correctly holds
dev->buflock, per the locking scheme documented at the top of
this file. This is a data race: an IRQ callback incrementing
read_buffer_length concurrently with adu_open() clearing it can
have its update silently lost, corrupting buffer state.

Take buflock around the reset to bring adu_open() in line with
the documented locking contract.

Cc: stable@vger.kernel.org
Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
---
 drivers/usb/misc/adutux.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 369d0d2..606ef91 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -228,6 +228,7 @@ static int adu_open(struct inode *inode, struct file *file)
 {
 	struct adu_device *dev = NULL;
 	struct usb_interface *interface;
+	unsigned long flags;
 	int subminor;
 	int retval;
 
@@ -265,7 +266,9 @@ static int adu_open(struct inode *inode, struct file *file)
 	file->private_data = dev;
 
 	/* initialize in direction */
+	spin_lock_irqsave(&dev->buflock, flags);
 	dev->read_buffer_length = 0;
+	spin_unlock_irqrestore(&dev->buflock, flags);
 
 	/* fixup first read by having urb waiting for it */
 	usb_fill_int_urb(dev->interrupt_in_urb, dev->udev,
-- 
2.43.0

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

[Oliver Neukum]

On 13.07.26 06:45, Kanishka De Silva wrote:

> 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

Hi,

thank you for this elaborate testing.
However, are you sure of your simulation?

That is, can the circumstances you are simulating really arise?
adu_interrupt_in_callback() cannot really run after adu_release_internal()
as that calls adu_abort_transfers(), which in turn uses usb_kill_urb()
on interrupt_in_urb.

As the driver implements an exclusive open policy and uses a mutex
to guard that, it seems to me that adu_open() can race only against
itself and adu_release, both taking a mutex precisely to prevent
that.

	Regards
		Oliver

[PATCH v2] usb: adutux: take buflock when resetting read_buffer_length in adu_open()

[Kanishka De Silva]

dev->read_buffer_length is otherwise only ever touched under
dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
locking scheme documented in the comment above struct adu_device.
adu_open() resets it to 0 without holding buflock.

In the current code this is not a reachable race. adu_open() and
adu_release() are fully serialized by adutux_mutex, and
adu_release_internal() calls usb_kill_urb() (via
adu_abort_transfers()) before that mutex is dropped, which blocks
until any in-flight adu_interrupt_in_callback() has returned. The
write in adu_open() also precedes the urb (re)submission in program
order, so the callback cannot observe or race with it there either.

This change brings the assignment under buflock purely so the
field's locking is locally consistent with the driver's documented
scheme, making the invariant easy to verify without having to reason
across adu_open(), adu_release_internal(), and usb_kill_urb()'s
blocking semantics. No behavioral or functional change intended.

v2:
 - Retitled and reframed from "fix unlocked read_buffer_length write
   in adu_open() (data race)" to a lock-discipline consistency
   change. Discussion on the RFC (with Oliver Neukum) established
   that adutux_mutex serialization plus usb_kill_urb()'s blocking
   semantics rule out any runtime-reachable race here, so this is
   no longer presented as a bugfix. (Oliver Neukum, Greg
   Kroah-Hartman)

Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
---
 drivers/usb/misc/adutux.c | 3 ++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 1111111..2222222 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
 	file->private_data = dev;
 
 	/* initialize in direction */
-	dev->read_buffer_length = 0;
+	spin_lock_irq(&dev->buflock);
+	dev->read_buffer_length = 0;
+	spin_unlock_irq(&dev->buflock);
 
 	/* fixup first read by having urb waiting for it */
 	usb_fill_int_urb(dev->interrupt_in_urb, dev->udev,
--
2.43.0

Re: [PATCH v2] usb: adutux: take buflock when resetting read_buffer_length in adu_open()

[Greg KH]

On Mon, Jul 13, 2026 at 01:26:16PM +0530, Kanishka De Silva wrote:
> dev->read_buffer_length is otherwise only ever touched under
> dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
> locking scheme documented in the comment above struct adu_device.
> adu_open() resets it to 0 without holding buflock.
> 
> In the current code this is not a reachable race. adu_open() and
> adu_release() are fully serialized by adutux_mutex, and
> adu_release_internal() calls usb_kill_urb() (via
> adu_abort_transfers()) before that mutex is dropped, which blocks
> until any in-flight adu_interrupt_in_callback() has returned. The
> write in adu_open() also precedes the urb (re)submission in program
> order, so the callback cannot observe or race with it there either.
> 
> This change brings the assignment under buflock purely so the
> field's locking is locally consistent with the driver's documented
> scheme, making the invariant easy to verify without having to reason
> across adu_open(), adu_release_internal(), and usb_kill_urb()'s
> blocking semantics. No behavioral or functional change intended.
> 
> v2:
>  - Retitled and reframed from "fix unlocked read_buffer_length write
>    in adu_open() (data race)" to a lock-discipline consistency
>    change. Discussion on the RFC (with Oliver Neukum) established
>    that adutux_mutex serialization plus usb_kill_urb()'s blocking
>    semantics rule out any runtime-reachable race here, so this is
>    no longer presented as a bugfix. (Oliver Neukum, Greg
>    Kroah-Hartman)

Version info goes below the --- line please.

> 
> Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
> ---
>  drivers/usb/misc/adutux.c | 3 ++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
> index 1111111..2222222 100644
> --- a/drivers/usb/misc/adutux.c
> +++ b/drivers/usb/misc/adutux.c
> @@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
>  	file->private_data = dev;
>  
>  	/* initialize in direction */
> -	dev->read_buffer_length = 0;
> +	spin_lock_irq(&dev->buflock);
> +	dev->read_buffer_length = 0;
> +	spin_unlock_irq(&dev->buflock);

This makes no sense, the single write here isn't going to need a lock,
it can't "tear" and if someone else is doing something with it it can
change right after the lock is released, right?

So what exactly is this supposed to be "fixing"?

Ah, your changelog says "not really fixing anything", so then it's not
needed at all, so why add this at all?

thanks,

greg k-h

Re: [PATCH v2] usb: adutux: take buflock when resetting read_buffer_length in adu_open()

[Alan Stern]

On Mon, Jul 13, 2026 at 02:03:03PM +0200, Greg KH wrote:
> On Mon, Jul 13, 2026 at 01:26:16PM +0530, Kanishka De Silva wrote:
> > dev->read_buffer_length is otherwise only ever touched under
> > dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
> > locking scheme documented in the comment above struct adu_device.
> > adu_open() resets it to 0 without holding buflock.
> > 
> > In the current code this is not a reachable race. adu_open() and
> > adu_release() are fully serialized by adutux_mutex, and
> > adu_release_internal() calls usb_kill_urb() (via
> > adu_abort_transfers()) before that mutex is dropped, which blocks
> > until any in-flight adu_interrupt_in_callback() has returned. The
> > write in adu_open() also precedes the urb (re)submission in program
> > order, so the callback cannot observe or race with it there either.
> > 
> > This change brings the assignment under buflock purely so the
> > field's locking is locally consistent with the driver's documented
> > scheme, making the invariant easy to verify without having to reason
> > across adu_open(), adu_release_internal(), and usb_kill_urb()'s
> > blocking semantics. No behavioral or functional change intended.
> > 
> > v2:
> >  - Retitled and reframed from "fix unlocked read_buffer_length write
> >    in adu_open() (data race)" to a lock-discipline consistency
> >    change. Discussion on the RFC (with Oliver Neukum) established
> >    that adutux_mutex serialization plus usb_kill_urb()'s blocking
> >    semantics rule out any runtime-reachable race here, so this is
> >    no longer presented as a bugfix. (Oliver Neukum, Greg
> >    Kroah-Hartman)
> 
> Version info goes below the --- line please.
> 
> > 
> > Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
> > ---
> >  drivers/usb/misc/adutux.c | 3 ++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
> > index 1111111..2222222 100644
> > --- a/drivers/usb/misc/adutux.c
> > +++ b/drivers/usb/misc/adutux.c
> > @@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
> >  	file->private_data = dev;
> >  
> >  	/* initialize in direction */
> > -	dev->read_buffer_length = 0;
> > +	spin_lock_irq(&dev->buflock);
> > +	dev->read_buffer_length = 0;
> > +	spin_unlock_irq(&dev->buflock);
> 
> This makes no sense, the single write here isn't going to need a lock,
> it can't "tear" and if someone else is doing something with it it can
> change right after the lock is released, right?
> 
> So what exactly is this supposed to be "fixing"?
> 
> Ah, your changelog says "not really fixing anything", so then it's not
> needed at all, so why add this at all?

A better solution would be to leave the code as it is, and change the 
comment instead.  Say that buflock protects the things touched by IRQ 
handlers, but only after they have been initialized in adu_open().

Alan Stern