* usb cdc-acm TIOCSSERIAL may wrongly report EPERM
@ 2020-02-18 16:48 Anthony Mallet
2020-02-19 9:56 ` Oliver Neukum
0 siblings, 1 reply; 7+ messages in thread
From: Anthony Mallet @ 2020-02-18 16:48 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-usb
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM 2020-02-18 16:48 usb cdc-acm TIOCSSERIAL may wrongly report EPERM Anthony Mallet @ 2020-02-19 9:56 ` Oliver Neukum 2020-02-19 11:21 ` Anthony Mallet 0 siblings, 1 reply; 7+ messages in thread From: Oliver Neukum @ 2020-02-19 9:56 UTC (permalink / raw) To: Anthony Mallet; +Cc: linux-usb [-- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM 2020-02-19 9:56 ` Oliver Neukum @ 2020-02-19 11:21 ` Anthony Mallet 2020-02-20 9:46 ` Oliver Neukum ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Anthony Mallet @ 2020-02-19 11:21 UTC (permalink / raw) To: Oliver Neukum; +Cc: linux-usb [-- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM 2020-02-19 11:21 ` Anthony Mallet @ 2020-02-20 9:46 ` Oliver Neukum 2020-02-20 10:00 ` Greg KH 2020-02-20 10:27 ` Oliver Neukum 2020-03-04 9:01 ` Greg KH 2 siblings, 1 reply; 7+ messages in thread From: Oliver Neukum @ 2020-02-20 9:46 UTC (permalink / raw) To: Anthony Mallet; +Cc: linux-usb ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM 2020-02-20 9:46 ` Oliver Neukum @ 2020-02-20 10:00 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2020-02-20 10:00 UTC (permalink / raw) To: Oliver Neukum; +Cc: Anthony Mallet, linux-usb On Thu, Feb 20, 2020 at 10:46:43AM +0100, Oliver Neukum wrote: > empty message??? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM 2020-02-19 11:21 ` Anthony Mallet 2020-02-20 9:46 ` Oliver Neukum @ 2020-02-20 10:27 ` Oliver Neukum 2020-03-04 9:01 ` Greg KH 2 siblings, 0 replies; 7+ messages in thread From: Oliver Neukum @ 2020-02-20 10:27 UTC (permalink / raw) To: Anthony Mallet; +Cc: linux-usb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: usb cdc-acm TIOCSSERIAL may wrongly report EPERM 2020-02-19 11:21 ` Anthony Mallet 2020-02-20 9:46 ` Oliver Neukum 2020-02-20 10:27 ` Oliver Neukum @ 2020-03-04 9:01 ` Greg KH 2 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2020-03-04 9:01 UTC (permalink / raw) To: Anthony Mallet; +Cc: Oliver Neukum, linux-usb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-04 9:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-18 16:48 usb cdc-acm TIOCSSERIAL may wrongly report EPERM Anthony Mallet 2020-02-19 9:56 ` Oliver Neukum 2020-02-19 11:21 ` Anthony Mallet 2020-02-20 9:46 ` Oliver Neukum 2020-02-20 10:00 ` Greg KH 2020-02-20 10:27 ` Oliver Neukum 2020-03-04 9:01 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).