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>] [day] [month] [year] [list]
Message-ID: <555fbc4c-043b-8932-fb9b-a208d61ffbe4@0882a8b5-c6c3-11e9-b005-00805fc181fe.uuid.home.arpa>
Date:   Sun, 20 Aug 2023 20:13:01 +0100
From:   Simon Arlott <simon@...iron.net>
To:     Oliver Neukum <oneukum@...e.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH] USB: cdc-acm: support flushing write buffers (TCOFLUSH)

If the serial device never reads data written to it (because it is "output
only") then the write buffers will still be waiting for the URB to complete
on close(), which will hang for 30s until the closing_wait timeout expires.

This can happen with the ESP32-H2/ESP32-C6 USB serial interface. Changing
the port closing_wait timeout is a privileged operation but flushing the
output buffer is not a privileged operation.

Implement the flush_buffer tty operation to cancel in-progress writes so
that tcflush(fd, TCOFLUSH) can be used to unblock the serial port before
close.

Signed-off-by: Simon Arlott <simon@...iron.net>
---
There's no guarantee that the write buffers will be cancelled in an
appropriate order because they could have been submitted in any order.

If there are successful writes in progress or if only one of the URBs
was stuck and later URBs could be completed, it's possible that an
later write could complete after cancelling an earlier one.

Should I change "struct acm_wb wb[ACM_NW]" to a list so that writes can
be cancelled in reverse order?

With some extra debug, writing 6 individual bytes and then calling
flush_buffer() looks like this:
[1850121.576468] cdc_acm 5-2.5.4:1.1: 1 bytes from tty layer
[1850121.576469] cdc_acm 5-2.5.4:1.1: writing 1 bytes
[1850121.876857] cdc_acm 5-2.5.4:1.1: 1 bytes from tty layer
[1850121.876858] cdc_acm 5-2.5.4:1.1: writing 1 bytes
[1850122.177273] cdc_acm 5-2.5.4:1.1: 1 bytes from tty layer
[1850122.177275] cdc_acm 5-2.5.4:1.1: writing 1 bytes
[1850122.477678] cdc_acm 5-2.5.4:1.1: 1 bytes from tty layer
[1850122.477679] cdc_acm 5-2.5.4:1.1: writing 1 bytes
[1850122.778101] cdc_acm 5-2.5.4:1.1: 1 bytes from tty layer
[1850122.778103] cdc_acm 5-2.5.4:1.1: writing 1 bytes
[1850123.078510] cdc_acm 5-2.5.4:1.1: 1 bytes from tty layer
[1850123.078512] cdc_acm 5-2.5.4:1.1: writing 1 bytes
[1850123.563234] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: transmitting=6
[1850123.563236] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[0] .use=1
[1850123.563245] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[1] .use=1
[1850123.563247] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[2] .use=1
[1850123.563249] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[3] .use=1
[1850123.563250] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[4] .use=1
[1850123.563252] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[5] .use=1
[1850123.563254] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[6] .use=0
[1850123.563255] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[7] .use=0
[1850123.563256] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[8] .use=0
[1850123.563256] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[9] .use=0
[1850123.563257] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[10] .use=0
[1850123.563257] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[11] .use=0
[1850123.563258] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[12] .use=0
[1850123.563258] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[13] .use=0
[1850123.563259] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[14] .use=0
[1850123.563260] cdc_acm 5-2.5.4:1.0: acm_tty_flush_buffer: wb[15] .use=0
[1850123.563568] cdc_acm 5-2.5.4:1.1: wrote len 0/1, status -104
[1850123.563571] cdc_acm 5-2.5.4:1.1: wrote len 0/1, status -104
[1850123.563572] cdc_acm 5-2.5.4:1.1: wrote len 0/1, status -104
[1850123.563574] cdc_acm 5-2.5.4:1.1: wrote len 0/1, status -104
[1850123.563575] cdc_acm 5-2.5.4:1.1: wrote len 0/1, status -104
[1850123.563577] cdc_acm 5-2.5.4:1.1: wrote len 0/1, status -104

 drivers/usb/class/cdc-acm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 745de40310d2..00db9e1fc7ed 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -863,6 +863,19 @@ static unsigned int acm_tty_write_room(struct tty_struct *tty)
 	return acm_wb_is_avail(acm) ? acm->writesize : 0;
 }
 
+static void acm_tty_flush_buffer(struct tty_struct *tty)
+{
+	struct acm *acm = tty->driver_data;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&acm->write_lock, flags);
+	for (i = 0; i < ACM_NW; i++)
+		if (acm->wb[i].use)
+			usb_unlink_urb(acm->wb[i].urb);
+	spin_unlock_irqrestore(&acm->write_lock, flags);
+}
+
 static unsigned int acm_tty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
@@ -2026,6 +2039,7 @@ static const struct tty_operations acm_ops = {
 	.hangup =		acm_tty_hangup,
 	.write =		acm_tty_write,
 	.write_room =		acm_tty_write_room,
+	.flush_buffer =		acm_tty_flush_buffer,
 	.ioctl =		acm_tty_ioctl,
 	.throttle =		acm_tty_throttle,
 	.unthrottle =		acm_tty_unthrottle,
-- 
2.37.0

-- 
Simon Arlott

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ