[PATCH] usb: class: cdc-wdm: fix reordering issue in read code path

[PATCH] usb: class: cdc-wdm: fix reordering issue in read code path

[Oliver Neukum]

Quoting the bug report:

Due to compiler optimization or CPU out-of-order execution, the
desc->length update can be reordered before the memmove. If this
happens, wdm_read() can see the new length and call copy_to_user() on
uninitialized memory. This also violates LKMM data race rules [1].

Fix it by using WRITE_ONCE and memory barriers.

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index f2d94cfc70af..7556c0dac908 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -225,7 +225,8 @@ static void wdm_in_callback(struct urb *urb)
 		/* we may already be in overflow */
 		if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
 			memmove(desc->ubuf + desc->length, desc->inbuf, length);
-			desc->length += length;
+			smp_wmb(); /* against wdm_read() */
+			WRITE_ONCE(desc->length, desc->length + length);
 		}
 	}
 skip_error:
@@ -533,6 +534,7 @@ static ssize_t wdm_read
 		return -ERESTARTSYS;
 
 	cntr = READ_ONCE(desc->length);
+	smp_rmb(); /* against wdm_in_callback() */
 	if (cntr == 0) {
 		desc->read = 0;
 retry:
-- 
2.53.0

Re: [PATCH] usb: class: cdc-wdm: fix reordering issue in read code path

[Gui-Dong Han]

On Wed, Mar 4, 2026 at 9:01 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> Quoting the bug report:
>
> Due to compiler optimization or CPU out-of-order execution, the
> desc->length update can be reordered before the memmove. If this
> happens, wdm_read() can see the new length and call copy_to_user() on
> uninitialized memory. This also violates LKMM data race rules [1].
>
> Fix it by using WRITE_ONCE and memory barriers.
>
> Fixes: afba937e540c9 ("USB: CDC WDM driver")

Closes: https://lore.kernel.org/linux-usb/CALbr=LbrUZn_cfp7CfR-7Z5wDTHF96qeuM=3fO2m-q4cDrnC4A@mail.gmail.com/
Reported-by: Gui-Dong Han <hanguidong02@gmail.com>

Looks good for the immediate fix.

As a long-term solution, refactoring this to use kfifo would be much
better. The race condition I reported is just one example of how this
ad-hoc lockless implementation can fail. Hand-rolling lockless
algorithms is highly error-prone and likely hides other subtle bugs.

Reviewed-by: Gui-Dong Han <hanguidong02@gmail.com>

> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/class/cdc-wdm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index f2d94cfc70af..7556c0dac908 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -225,7 +225,8 @@ static void wdm_in_callback(struct urb *urb)
>                 /* we may already be in overflow */
>                 if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
>                         memmove(desc->ubuf + desc->length, desc->inbuf, length);
> -                       desc->length += length;
> +                       smp_wmb(); /* against wdm_read() */
> +                       WRITE_ONCE(desc->length, desc->length + length);
>                 }
>         }
>  skip_error:
> @@ -533,6 +534,7 @@ static ssize_t wdm_read
>                 return -ERESTARTSYS;
>
>         cntr = READ_ONCE(desc->length);
> +       smp_rmb(); /* against wdm_in_callback() */
>         if (cntr == 0) {
>                 desc->read = 0;
>  retry:
> --
> 2.53.0
>

Re: [PATCH] usb: class: cdc-wdm: fix reordering issue in read code path

[Gui-Dong Han]

On Wed, Mar 4, 2026 at 9:01 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> Quoting the bug report:
>
> Due to compiler optimization or CPU out-of-order execution, the
> desc->length update can be reordered before the memmove. If this
> happens, wdm_read() can see the new length and call copy_to_user() on
> uninitialized memory. This also violates LKMM data race rules [1].
>
> Fix it by using WRITE_ONCE and memory barriers.
>
> Fixes: afba937e540c9 ("USB: CDC WDM driver")
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/class/cdc-wdm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index f2d94cfc70af..7556c0dac908 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -225,7 +225,8 @@ static void wdm_in_callback(struct urb *urb)
>                 /* we may already be in overflow */
>                 if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
>                         memmove(desc->ubuf + desc->length, desc->inbuf, length);
> -                       desc->length += length;
> +                       smp_wmb(); /* against wdm_read() */
> +                       WRITE_ONCE(desc->length, desc->length + length);

smp_store_release(&desc->length, desc->length + length);

>                 }
>         }
>  skip_error:
> @@ -533,6 +534,7 @@ static ssize_t wdm_read
>                 return -ERESTARTSYS;
>
>         cntr = READ_ONCE(desc->length);
> +       smp_rmb(); /* against wdm_in_callback() */

cntr = smp_load_acquire(&desc->length);

I just realized one minor detail.

Instead of using smp_wmb() + WRITE_ONCE() and smp_rmb() + READ_ONCE(),
we could use smp_store_release() and smp_load_acquire().

It perfectly matches the acquire/release semantics here and might
bring slight performance benefits in some scenarios.

Feel free to send a v2 if you like the idea. But no pressure at all—I
am totally fine with the current v1 as well, so it is entirely up to
you.

>         if (cntr == 0) {
>                 desc->read = 0;
>  retry:
> --
> 2.53.0
>

Re: [PATCH] usb: class: cdc-wdm: fix reordering issue in read code path

[Oliver Neukum]

On 04.03.26 15:54, Gui-Dong Han wrote:

Hi,

thank you for the suggestion.

> Instead of using smp_wmb() + WRITE_ONCE() and smp_rmb() + READ_ONCE(),
> we could use smp_store_release() and smp_load_acquire().
> 
> It perfectly matches the acquire/release semantics here and might
> bring slight performance benefits in some scenarios.

CDC-WDM is not really critical to performance. Hence I'd like
as obvious and simple a fix as possible.
  
> Feel free to send a v2 if you like the idea. But no pressure at all—I
> am totally fine with the current v1 as well, so it is entirely up to
> you.

The clean fix is to go to kfifo, isn't it? This is the best
long term solution IMHO. First I'll review reordering issue
with set_bit() and related operations.

In defense of the driver, if I may: It is older than the kfifo
API.

	Regards
		Oliver

Re: [PATCH] usb: class: cdc-wdm: fix reordering issue in read code path

[Greg KH]

On Wed, Mar 04, 2026 at 09:43:47PM +0800, Gui-Dong Han wrote:
> On Wed, Mar 4, 2026 at 9:01 PM Oliver Neukum <oneukum@suse.com> wrote:
> >
> > Quoting the bug report:
> >
> > Due to compiler optimization or CPU out-of-order execution, the
> > desc->length update can be reordered before the memmove. If this
> > happens, wdm_read() can see the new length and call copy_to_user() on
> > uninitialized memory. This also violates LKMM data race rules [1].
> >
> > Fix it by using WRITE_ONCE and memory barriers.
> >
> > Fixes: afba937e540c9 ("USB: CDC WDM driver")
> 
> Closes: https://lore.kernel.org/linux-usb/CALbr=LbrUZn_cfp7CfR-7Z5wDTHF96qeuM=3fO2m-q4cDrnC4A@mail.gmail.com/
> Reported-by: Gui-Dong Han <hanguidong02@gmail.com>
> 
> Looks good for the immediate fix.
> 
> As a long-term solution, refactoring this to use kfifo would be much
> better. The race condition I reported is just one example of how this
> ad-hoc lockless implementation can fail. Hand-rolling lockless
> algorithms is highly error-prone and likely hides other subtle bugs.

Again, great, please submit patches to do this.  It was done "recently"
for the tty layer, so perhaps take a look at that work to see how it can
be done, and the issues involved.  I'd be interested if it changes the
throughput and code size any (probably not on throughput given the slow
speeds of USB, but code size might be more interesting...)

thanks,

greg k-h