linux-kernel - Re: [3.16.1 BISECTED REGRESSION]: Simtec Entropy Key (cdc-acm) broken in 3.16

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]

Message-ID: <87eguayalj.fsf@spindle.srvr.nix>
Date:	Tue, 14 Oct 2014 15:44:40 +0100
From:	Nix <nix@...eri.org.uk>
To:	Johan Hovold <johan@...nel.org>
Cc:	Paul Martin <pm@...ian.org>, Oliver Neukum <oliver@...kum.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [3.16.1 BISECTED REGRESSION]: Simtec Entropy Key (cdc-acm) broken in 3.16

On 14 Oct 2014, Johan Hovold spake thusly:

> On Sun, Oct 12, 2014 at 10:36:30PM +0100, Nix wrote:
>> On 12 Oct 2014, Johan Hovold verbalised:
>> 
>> > On Sat, Oct 11, 2014 at 11:24:59PM +0100, Nix wrote:
>> >> On 11 Oct 2014, Paul Martin spake thusly:
>> >> 
>> >> > Having been privy to the firmware of the eKey, it is very simplisting,
>> >> > with no implementation whatsoever of any flow control.
>> >> 
>> >> That's what I thought. (Why would something that just provides data at a
>> >> constant rate way below that of even the slowest USB bus *need* flow
>> >> control?)
>> >> 
>> >> One presumes therefore that the kernel suddenly trying to do flow
>> >> control on shutdown would fubar the firmware's internal state, leading
>> >> to the symptoms I see.
>> >
>> > The cdc-acm driver was dropping DTR/RTS on shutdown (close) also before
>> > the commit you refer to. One thing it did change however is that this is
>> > now only done if HUPCL is set. Might setting that flag be enough to
>> > prevent the device firmware from crashing?
>> 
>> If I read the ekeyd 1.1.5 source code correctly, this is already
>> happening:
>> 
>> ,----[ host/stream.c:estream_open() ]
>> |     } else if (S_ISCHR(sbuf.st_mode)) {
>> |         /* Open the file as a character device/tty */
>> |         fd = open(uri, O_RDWR | O_NOCTTY);
>> |         if ((fd != -1) && (isatty(fd))) {
>> |             if (tcgetattr(fd, &settings) == 0 ) {
>> |                 settings.c_cflag &= ~(CSIZE | CSTOPB | PARENB | CLOCAL |
>> |                                       CREAD | PARODD | CRTSCTS);
>> |                 settings.c_iflag &= ~(BRKINT | IGNPAR | PARMRK | INPCK |
>> |                                       ISTRIP | INLCR | IGNCR | ICRNL | IXON |
>> |                                       IXOFF  | IXANY | IMAXBEL);
>> |                 settings.c_iflag |= IGNBRK;
>> |                 settings.c_oflag &= ~(OPOST | OCRNL | ONOCR | ONLRET);
>> |                 settings.c_lflag &= ~(ISIG | ICANON | IEXTEN | ECHO |
>> |                                       ECHOE | ECHOK | ECHONL | NOFLSH |
>> |                                       TOSTOP | ECHOPRT | ECHOCTL | ECHOKE);
>> |                 settings.c_cflag |= CS8 | HUPCL | CREAD | CLOCAL;
>> | #ifdef EKEY_FULL_TERMIOS
>> |                 settings.c_cflag &= ~(CBAUD);
>> | 		  settings.c_iflag &= ~(IUTF8 | IUCLC);
>> |                 settings.c_oflag &= ~(OFILL | OFDEL | NLDLY | CRDLY | TABDLY |
>> |                                       BSDLY | VTDLY | FFDLY | OLCUC );
>> |                 settings.c_oflag |= NL0 | CR0 | TAB0 | BS0 | VT0 | FF0;
>> |                 settings.c_lflag &= ~(XCASE);
>> | #endif
>> |                 settings.c_cflag |= B115200;
>> |                 if (tcsetattr(fd, TCSANOW, &settings) < 0) {
>> `----
>> 
>> Note the HUPCL in there.
>> 
>> I have checked: this code is being executed against a symlink that
>> points to /dev/ttyACM0, and the tcsetattr() succeeds. (At least, it's
>> succeeding on the kernel I'm running now, but of course that's 3.16.5
>> with this commit reverted...)
>
> You could verify that by enabling debugging in the cdc-acm driver and
> making sure that the corresponding control messages are indeed sent on
> close.

Yeah, good idea -- in the specific context of the system rebooting, in
particular (though I'll also see if just killing the daemon causes this
problem, something I haven't tested). I was assuming it would be hard to
see it before the reboot process cleared the screen -- but of course
this machine has a serial console so that's not important.

> But you haven't seen any fw crashes since you reverted the commit in
> question?

Not one.

> Another thing you could try is to add back the 
>
> 	acm_set_control(acm, 0);
>
> just after the dev_info message in probe.

I'll try that tonight.

-- 
NULL && (void)
--
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/