linux-kernel - [PATCH v2] usb: serial: usb_wwan: Fix data races by protecting dtr/rts state with a mutex

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]

Message-ID: <20250701214543.981626-1-abinashsinghlalotra@gmail.com>
Date: Wed,  2 Jul 2025 03:15:32 +0530
From: Abinash Singh <abinashlalotra@...il.com>
To: gregkh@...uxfoundation.org
Cc: abinashlalotra@...il.com,
	abinashsinghlalotra@...il.com,
	johan@...nel.org,
	linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org,
	oneukum@...e.com
Subject: [PATCH v2] usb: serial: usb_wwan: Fix data races by protecting dtr/rts state with a mutex

The rts_state and dtr_state fields in usb_wwan were updated without
any locking, which could lead to data races if accessed from multiple
threads.

Fixes proper locking using guard(mutex) to ensure
safe access to these shared fields. To avoid holding the lock during
USB control message transmission, the values are passed explicitly
to usb_wwan_send_setup().

This resolves two previously marked FIXME comments and improves
the thread safety of modem control line handling.

Signed-off-by: Abinash Singh <abinashsinghlalotra@...il.com>
---
v2:
- I missed the "v2" tag in the subject line earlier — added now, sorry about that.
- Regarding the concern about locking while calling functions: I was unsure if
  it’s safe to hold the lock across `usb_wwan_send_setup()`, since it may block.
  To be safe, I’ve changed the function to take `rts_state` and `dtr_state` as
  arguments, so it no longer accesses shared state directly.
- I’ve now used `guard(mutex)` so the lock will automatically release when
  `portdata` goes out of scope.

   Is this the correct way to use gaurd if we don't want the lock held during
   usb_wwan_send_setup() ?

> How was this tested?

I haven’t been able to test this patch due to lack of hardware access. If you
have any suggestions on how to test this kind of change without actual hardware,
I’d appreciate your guidance.

Thanks for the feedback!
---
 drivers/usb/serial/usb-wwan.h |  1 +
 drivers/usb/serial/usb_wwan.c | 29 ++++++++++++++++-------------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
index 519101945769..5a990fc2e140 100644
--- a/drivers/usb/serial/usb-wwan.h
+++ b/drivers/usb/serial/usb-wwan.h
@@ -59,6 +59,7 @@ struct usb_wwan_port_private {
 	int ri_state;
 
 	unsigned long tx_start_time[N_OUT_URB];
+	struct mutex lock;  /* protects rts_state and dtr_state */
 };
 
 #endif /* __LINUX_USB_USB_WWAN */
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 0017f6e969e1..042d63aa8ec6 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -38,19 +38,16 @@
  * Generate DTR/RTS signals on the port using the SET_CONTROL_LINE_STATE request
  * in CDC ACM.
  */
-static int usb_wwan_send_setup(struct usb_serial_port *port)
+static int usb_wwan_send_setup(struct usb_serial_port *port, int rts_state, int dtr_state)
 {
 	struct usb_serial *serial = port->serial;
-	struct usb_wwan_port_private *portdata;
 	int val = 0;
 	int ifnum;
 	int res;
 
-	portdata = usb_get_serial_port_data(port);
-
-	if (portdata->dtr_state)
+	if (dtr_state)
 		val |= USB_CDC_CTRL_DTR;
-	if (portdata->rts_state)
+	if (rts_state)
 		val |= USB_CDC_CTRL_RTS;
 
 	ifnum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
@@ -80,11 +77,12 @@ void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
 		return;
 
 	portdata = usb_get_serial_port_data(port);
-	/* FIXME: locking */
+	{
+	guard(mutex)(&portdata->lock);
 	portdata->rts_state = on;
 	portdata->dtr_state = on;
-
-	usb_wwan_send_setup(port);
+	}
+	usb_wwan_send_setup(port,on,on);
 }
 EXPORT_SYMBOL(usb_wwan_dtr_rts);
 
@@ -113,14 +111,15 @@ int usb_wwan_tiocmset(struct tty_struct *tty,
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_wwan_port_private *portdata;
 	struct usb_wwan_intf_private *intfdata;
+	int rts, dtr;
 
 	portdata = usb_get_serial_port_data(port);
 	intfdata = usb_get_serial_data(port->serial);
 
 	if (!intfdata->use_send_setup)
 		return -EINVAL;
-
-	/* FIXME: what locks portdata fields ? */
+	{
+	guard(mutex)(&portdata->lock);
 	if (set & TIOCM_RTS)
 		portdata->rts_state = 1;
 	if (set & TIOCM_DTR)
@@ -130,7 +129,11 @@ int usb_wwan_tiocmset(struct tty_struct *tty,
 		portdata->rts_state = 0;
 	if (clear & TIOCM_DTR)
 		portdata->dtr_state = 0;
-	return usb_wwan_send_setup(port);
+
+	rts = portdata->rts_state;
+	dtr = portdata->dtr_state;
+	}
+	return usb_wwan_send_setup(port, rts, dtr);
 }
 EXPORT_SYMBOL(usb_wwan_tiocmset);
 
@@ -452,7 +455,7 @@ int usb_wwan_port_probe(struct usb_serial_port *port)
 	portdata = kzalloc(sizeof(*portdata), GFP_KERNEL);
 	if (!portdata)
 		return -ENOMEM;
-
+	mutex_init(&portdata->lock);
 	init_usb_anchor(&portdata->delayed);
 
 	for (i = 0; i < N_IN_URB; i++) {
-- 
2.43.0