linux-kernel - Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi

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]

Date:	Wed, 2 Nov 2011 16:21:21 +1100
From:	Andrew Worsley <amworsley@...il.com>
To:	Alan Cox <alan@...ux.intel.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>,
	Uwe Bonnes <bon@...ktron.ikp.physik.tu-darmstadt.de>,
	Johan Hovold <jhovold@...il.com>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

Avoid unnecessary Control URBs that reset the data/parity or baud rate
to the currently set settings which can cause the FTDI chip to glitch
it's serial output and cause a corruption of a character it is
currently outputting.

Signed-off-by: amworsley@...il.com

---

On 1 November 2011 22:36, Alan Cox <alan@...ux.intel.com> wrote:
>> I don't know why it repeatedly tries to set this all the time - it
>> would appear to be quite a lot of work so perhaps there is something
>> else that could be cleaned up. This was the simplest safe change that
>> fixed my problem. It appears this code hasn't changed very much since
>> the first history information in git that I could see - so perhaps
>> nobody else is really noticing this issue for some reason?
>
> It could be a problem specific to some firmware or revision. We've had
> a similar quirk with a different USB adapter. The actual calls to keep
> changing it are coming from your application however.

"My" application is getty via inittab e.g.
T0:23:respawn:/sbin/getty -L ttyUSB0 115200 vt100

  Something I would expect to be fairly common. Actually after login
it is "bash" which is running presumably the getty set the tty
parameters like speed and flow control.

>
>>         cflag = termios->c_cflag;
>>
>> -       /* FIXME -For this cut I don't care if the line is really
>> changing or
>> -          not  - so just do the change regardless  - should be able
>> to
>> -          compare old_termios and tty->termios */
>> +       /* compare old_termios and tty->termios */
>> +    if (old_termios->c_cflag == termios->c_cflag)
>> +        goto no_c_cflag_changes;
>
> You can't do it this way because the speed data is not entirely within
> c_cflag. Check c_ispeed and c_ospeed match and for the parity if you want to skip
> that check if the parity bits change specifically.
>
This is getting into magic flags I don't understand. There are so few
bits in c_cflag not related to speed and data/parity I am hesitant  to
write a complex check I might well get wrong. But flow control appears
to be switched off / on frequently during data flow,
my guess as part of flow controlling the input and I suggest a speed
change is likely to be a once per login or line usage occurrence.

Here's a more sophisticated version of this which attempts to do this.
It works on my 2.6.32 kernel and compiles the latest kernel.

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 8fe034d..66a2d09 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2104,13 +2104,20 @@ static void ftdi_set_termios(struct tty_struct *tty,

        cflag = termios->c_cflag;

-       /* FIXME -For this cut I don't care if the line is really changing or
-          not  - so just do the change regardless  - should be able to
-          compare old_termios and tty->termios */
+       /* compare old_termios and tty->termios */
+    if (old_termios->c_cflag == termios->c_cflag
+      && old_termios->c_ispeed == termios->c_ispeed
+      && old_termios->c_ospeed == termios->c_ospeed
+      )
+        goto no_c_cflag_changes;
+
        /* NOTE These routines can get interrupted by
           ftdi_sio_read_bulk_callback  - need to examine what this means -
           don't see any problems yet */

+    if ((old_termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)) ==
+        (termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)))
+        goto no_data_parity_stop_changes;
        /* Set number of data bits, parity, stop bits */

        urb_value = 0;
@@ -2150,6 +2157,7 @@ static void ftdi_set_termios(struct tty_struct *tty,
                        "databits/stopbits/parity\n", __func__);
        }

+no_data_parity_stop_changes:
        /* Now do the baudrate */
        if ((cflag & CBAUD) == B0) {
                /* Disable flow control */
@@ -2176,6 +2184,7 @@ static void ftdi_set_termios(struct tty_struct *tty,
                        set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
        }

+no_c_cflag_changes:
        /* Set flow control */
        /* Note device also supports DTR/CD (ugh) and Xon/Xoff in hardware */
        if (cflag & CRTSCTS) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/