usb cdc-acm TIOCSSERIAL may wrongly report EPERM

usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[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.

This happens line 921 of file cdc-acm.c
https://github.com/torvalds/linux/blob/master/drivers/usb/class/cdc-acm.c#L921
with for instance the following pseudo-code snippet when it is run as a
non CAP_SYS_ADMIN user. The code does not try to change anything but
EPERM might still be returned in some configurations (in particular
with CONFIG_HZ = 250):

    struct serial_struct s;
    int fd;
    int e;

    e = ioctl(fd, TIOCGSERIAL, &s);
    if (e != 0) abort();
    e = ioctl(fd, TIOCSSERIAL, &s);
    if (e != 0) printf("wrong EPERM report\n");

The issue is caused by the rounding to 10 (integer division or
multiplication) of the closing_wait and close_delay members of the
struct serial_struct information, lines 899 and 912 of the file
cdc-acm.c:
https://github.com/torvalds/linux/blob/master/drivers/usb/class/cdc-acm.c#L899
https://github.com/torvalds/linux/blob/master/drivers/usb/class/cdc-acm.c#L912

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

This is particularly misleading for doing proper error handling when
trying to set another setting, like e.g. the ASYNC_LOW_LATENCY flag
which is not supported by the driver and should thus report EOPNOTSUPP
as expected from the line 923 of cdc-acm.c:
https://github.com/torvalds/linux/blob/master/drivers/usb/class/cdc-acm.c#L923

The fix might be to just round the default values to a multiple of 10
in the intialization part, like right after line 1301 of file cdc-acm.c
https://github.com/torvalds/linux/blob/master/drivers/usb/class/cdc-acm.c#L1301
If a patch is needed to clarify my point, I can of course provide one.

Cheers,
Anthony Mallet

Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Oliver Neukum]

[-- 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

Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Anthony Mallet]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1232 bytes --]

On Wednesday 19 Feb 2020, at 10:56, Oliver Neukum wrote:
> thank you for the detailed report about an unusual bug. Could you
> test the attached patch?

I did not actually run it yet, but at first sight it seems that it will
mishandle the ASYNC_CLOSING_WAIT_NONE case.

I attach patch 0002 which should handle all cases. I also inverted the
logic of the CAP_SYS_ADMIN test, so that the delays are changed only
if the rounded values (in 1/100th of seconds) are actually changed.

While I was there, I checked what other drivers do (it seems that many
have the same issue). Something I also noticed is that the units are
also maybe mishandled: internally the values seem to be in 'jiffies',
not in msecs. See for instance:
https://github.com/torvalds/linux/blob/701a9c8092ddf299d7f90ab2d66b19b4526d1186/drivers/tty/serial/serial_core.c#L780

So I added patch 0001 that uses the jiffies conversion functions. If
you prefer to discuss this in a separate thread please tell me, I will
redo just patch 0002 without this change.


> I have taken the liberty of presuming to add your 'Reported-and-tested-by'.

Yes, no problem. Thank you.
I hope my patches follow the required format, I'm not sure about all
the details.

Regards,
Anthony


[-- Attachment #2: 0001-USB-cdc-acm-fix-close_delay-and-closing_wait-units-i.patch --]
[-- Type: text/x-patch, Size: 1777 bytes --]

From 3bc102eaa325fec8c181a51853ce3168dd89befa Mon Sep 17 00:00:00 2001
From: Anthony Mallet <anthony.mallet@laas.fr>
Date: Wed, 19 Feb 2020 11:39:12 +0100
Subject: [PATCH 1/2] USB: cdc-acm: fix close_delay and closing_wait units in
 TIOCSSERIAL

close_delay and closing_wait are specified in hundredth of a second but stored
internally in jiffies. Use the jiffies_to_msecs() and msecs_to_jiffies()
functions to convert from each other.

Reported-and-tested-by: Anthony Mallet <anthony.mallet@laas.fr>
---
 drivers/usb/class/cdc-acm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 62f4fb9b3..da619176d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -896,10 +896,10 @@ static int get_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 
 	ss->xmit_fifo_size = acm->writesize;
 	ss->baud_base = le32_to_cpu(acm->line.dwDTERate);
-	ss->close_delay	= acm->port.close_delay / 10;
+	ss->close_delay	= jiffies_to_msecs(acm->port.close_delay) / 10;
 	ss->closing_wait = acm->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
 				ASYNC_CLOSING_WAIT_NONE :
-				acm->port.closing_wait / 10;
+				jiffies_to_msecs(acm->port.closing_wait) / 10;
 	return 0;
 }
 
@@ -909,9 +909,10 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 	unsigned int closing_wait, close_delay;
 	int retval = 0;
 
-	close_delay = ss->close_delay * 10;
+	close_delay = msecs_to_jiffies(ss->close_delay * 10);
 	closing_wait = ss->closing_wait == ASYNC_CLOSING_WAIT_NONE ?
-			ASYNC_CLOSING_WAIT_NONE : ss->closing_wait * 10;
+			ASYNC_CLOSING_WAIT_NONE :
+			msecs_to_jiffies(ss->closing_wait * 10);
 
 	mutex_lock(&acm->port.mutex);
 
-- 
2.17.1


[-- Attachment #3: 0002-USB-cdc-acm-fix-rounding-error-in-TIOCSSERIAL.patch --]
[-- Type: text/x-patch, Size: 3028 bytes --]

From 448f387419a5970021b096248aa8650d8c03392d Mon Sep 17 00:00:00 2001
From: Anthony Mallet <anthony.mallet@laas.fr>
Date: Wed, 19 Feb 2020 11:47:03 +0100
Subject: [PATCH 2/2] 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

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 | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index da619176d..b43820fb2 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -907,6 +907,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 {
 	struct acm *acm = tty->driver_data;
 	unsigned int closing_wait, close_delay;
+	unsigned int old_closing_wait, old_close_delay;
 	int retval = 0;
 
 	close_delay = msecs_to_jiffies(ss->close_delay * 10);
@@ -914,18 +915,24 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 			ASYNC_CLOSING_WAIT_NONE :
 			msecs_to_jiffies(ss->closing_wait * 10);
 
+	/* we must redo the rounding here, so that the values match */
+	old_close_delay	= jiffies_to_msecs(acm->port.close_delay) / 10;
+	old_closing_wait = acm->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
+				ASYNC_CLOSING_WAIT_NONE :
+				jiffies_to_msecs(acm->port.closing_wait) / 10;
+
 	mutex_lock(&acm->port.mutex);
 
-	if (!capable(CAP_SYS_ADMIN)) {
-		if ((close_delay != acm->port.close_delay) ||
-		    (closing_wait != acm->port.closing_wait))
+	if ((ss->close_delay != old_close_delay) ||
+            (ss->closing_wait != old_closing_wait)) {
+		if (!capable(CAP_SYS_ADMIN)) {
 			retval = -EPERM;
-		else
-			retval = -EOPNOTSUPP;
-	} else {
-		acm->port.close_delay  = close_delay;
-		acm->port.closing_wait = closing_wait;
-	}
+		else {
+			acm->port.close_delay  = close_delay;
+			acm->port.closing_wait = closing_wait;
+		}
+	} else
+		retval = -EOPNOTSUPP;
 
 	mutex_unlock(&acm->port.mutex);
 	return retval;
@@ -969,7 +976,7 @@ static int wait_serial_change(struct acm *acm, unsigned long arg)
 		}
 	} while (!rv);
 
-	
+
 
 	return rv;
 }
-- 
2.17.1

Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Oliver Neukum]

Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Greg KH]

On Thu, Feb 20, 2020 at 10:46:43AM +0100, Oliver Neukum wrote:
> 

empty message???

Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Oliver Neukum]

Am Mittwoch, den 19.02.2020, 12:21 +0100 schrieb Anthony Mallet:
> On Wednesday 19 Feb 2020, at 10:56, Oliver Neukum wrote:
> > thank you for the detailed report about an unusual bug. Could you
> > test the attached patch?
> 
> I did not actually run it yet, but at first sight it seems that it will
> mishandle the ASYNC_CLOSING_WAIT_NONE case.

You are right.

> Yes, no problem. Thank you.
> I hope my patches follow the required format, I'm not sure about all
> the details.

The patches themselves look good. Please follow
Documentation/SubmittingPatches

	Regards
		Oliver

PS: my mailer seems to suffer a malfunction with HTML.
Sorry for the erstwhiole empty post

Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM

[Greg KH]

On Wed, Feb 19, 2020 at 12:21:55PM +0100, Anthony Mallet wrote:
> On Wednesday 19 Feb 2020, at 10:56, Oliver Neukum wrote:
> > thank you for the detailed report about an unusual bug. Could you
> > test the attached patch?
> 
> I did not actually run it yet, but at first sight it seems that it will
> mishandle the ASYNC_CLOSING_WAIT_NONE case.
> 
> I attach patch 0002 which should handle all cases. I also inverted the
> logic of the CAP_SYS_ADMIN test, so that the delays are changed only
> if the rounded values (in 1/100th of seconds) are actually changed.
> 
> While I was there, I checked what other drivers do (it seems that many
> have the same issue). Something I also noticed is that the units are
> also maybe mishandled: internally the values seem to be in 'jiffies',
> not in msecs. See for instance:
> https://github.com/torvalds/linux/blob/701a9c8092ddf299d7f90ab2d66b19b4526d1186/drivers/tty/serial/serial_core.c#L780
> 
> So I added patch 0001 that uses the jiffies conversion functions. If
> you prefer to discuss this in a separate thread please tell me, I will
> redo just patch 0002 without this change.
> 
> 
> > I have taken the liberty of presuming to add your 'Reported-and-tested-by'.
> 
> Yes, no problem. Thank you.
> I hope my patches follow the required format, I'm not sure about all
> the details.


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch was attached, please place it inline so that it can be
  applied directly from the email message itself.

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/SubmittingPatches and resend it after
  adding that line.  Note, the line needs to be in the body of the
  email, before the patch, not at the bottom of the patch or in the
  email signature.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot