Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Oliver Neukum <oneukum@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

Am Dienstag, den 18.02.2020, 17:48 +0100 schrieb Anthony Mallet:
> Hi,
> 
> A TIOCSSERIAL ioctl(2) performed on a file descriptor corresponding to
> a cdc-acm driver may wrongly report EPERM even when no CAP_SYS_ADMIN
> is required. This concerns almost all recent kernel versions, as I
> explain below.

Hi,

thank you for the detailed report about an unusual bug. Could you
test the attached patch? I have taken the liberty of presuming
to add your 'Reported-and-tested-by'.

	Regards
		Oliver

[-- Attachment #2: 0001-USB-cdc-acm-fix-rounding-error-in-TIOCSSERIAL.patch --]
[-- Type: text/x-patch, Size: 2007 bytes --]

From 78c1b02376b816108aadeba6ef9a5d69a3bfb9bb Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 19 Feb 2020 10:43:17 +0100
Subject: [PATCH] USB: cdc-acm: fix rounding error in TIOCSSERIAL

Quoting the bug reporter:

By default, tty_port_init() initializes those parameters to a multiple
of HZ. For instance in line 69 of tty_port.c:
   port->close_delay = (50 * HZ) / 100;
https://github.com/torvalds/linux/blob/master/drivers/tty/tty_port.c#L69

With e.g. CONFIG_HZ = 250 (as this is the case for Ubuntu 18.04
linux-image-4.15.0-37-generic), the default setting for close_delay is
thus 125.

When ioctl(fd, TIOCGSERIAL, &s) is executed, the setting returned in
user space is '12' (125/10). When ioctl(fd, TIOCSSERIAL, &s) is then
executed with the same setting '12', the value is interpreted as '120'
which is different from the current setting and a EPERM error may be
raised by set_serial_info() if !CAP_SYS_ADMIN.
https://github.com/torvalds/linux/blob/master/drivers/usb/class/cdc-acm.c#L919

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Reported-and-tested-by: Anthony Mallet <anthony.mallet@laas.fr>
Fixes: ba2d8ce9db0a6 ("cdc-acm: implement TIOCSSERIAL to avoid blocking close(2)")
---
 drivers/usb/class/cdc-acm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 62f4fb9b362f..f9064dbcd333 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -916,8 +916,9 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 	mutex_lock(&acm->port.mutex);
 
 	if (!capable(CAP_SYS_ADMIN)) {
-		if ((close_delay != acm->port.close_delay) ||
-		    (closing_wait != acm->port.closing_wait))
+		/* we must redo the rounding here, so that the values match */
+		if ((close_delay != (acm->port.close_delay / 10) * 10) ||
+		    (closing_wait != (acm->port.closing_wait / 10) * 10))
 			retval = -EPERM;
 		else
 			retval = -EOPNOTSUPP;
-- 
2.16.4