| Message ID | 20200909193419.2006744-1-james.hilliard1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: serial: Repair FTDI FT232R bricked eeprom | expand |
Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > This patch detects and reverses the effects of the malicious FTDI > Windows driver version 2.12.00(FTDIgate). Hi, this raises questions. Should we do this unconditionally without asking? Does this belong into kernel space? > +static int ftdi_repair_brick(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + int orig_latency; > + int rv; > + u16 *eeprom_data; > + u16 checksum; > + int eeprom_size; > + int result; > + > + switch (priv->chip_type) { > + case FT232RL: > + eeprom_size = 0x40; > + break; > + default: > + /* Unsupported for brick repair */ > + return 0; > + } > + > + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ > + if (priv->latency != 0x77) { > + orig_latency = priv->latency; > + priv->latency = 0x77; > + rv = write_latency_timer(port); > + priv->latency = orig_latency; > + if (rv < 0) > + return -EIO; > + } Do you really want to change this without returning to the original? Regards Oliver
On September 10, 2020 12:02:34 PM GMT+09:00, Oliver Neukum <oneukum@suse.de> wrote: >Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: >> This patch detects and reverses the effects of the malicious FTDI >> Windows driver version 2.12.00(FTDIgate). > >Hi, > >this raises questions. >Should we do this unconditionally without asking? >Does this belong into kernel space? I agree; this is very cute, but does it really need to be an automatic Linux feature? Presumably someone looking to fix a bricked FTDI chip can just run my script, and those who just want to use those chips with Linux already can since the driver binds to the zero PID. I am deeply amused by the idea of Linux automatically fixing problems caused by malicious Windows drivers, but thinking objectively, I'm not sure if that's the right thing to do. > >> +static int ftdi_repair_brick(struct usb_serial_port *port) >> +{ >> + struct ftdi_private *priv = usb_get_serial_port_data(port); >> + int orig_latency; >> + int rv; >> + u16 *eeprom_data; >> + u16 checksum; >> + int eeprom_size; >> + int result; >> + >> + switch (priv->chip_type) { >> + case FT232RL: >> + eeprom_size = 0x40; >> + break; >> + default: >> + /* Unsupported for brick repair */ >> + return 0; >> + } >> + >> + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ >> + if (priv->latency != 0x77) { >> + orig_latency = priv->latency; >> + priv->latency = 0x77; >> + rv = write_latency_timer(port); >> + priv->latency = orig_latency; >> + if (rv < 0) >> + return -EIO; >> + } > >Do you really want to change this without returning to the original? > > Regards > Oliver
On Wed, Sep 9, 2020 at 9:02 PM Oliver Neukum <oneukum@suse.de> wrote: > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > > This patch detects and reverses the effects of the malicious FTDI > > Windows driver version 2.12.00(FTDIgate). > > Hi, > > this raises questions. > Should we do this unconditionally without asking? Well I think since we can reliably detect devices that have been bricked by the malicious windows driver it's fine. I was careful to ensure that this will bail out and not try to change anything unless all conditions match this specific brick attack. > Does this belong into kernel space? This seemed to be by far the simplest option for embedded systems that need to automatically detect and repair the bricked eeproms. People seem to have plugged a bunch of counterfeit FTDI Arduino's that normally attach to an embedded Linux host into windows for some reason for a kiosk platform of mine which messed up the userspace detection/mappings. This seemed like the best way to avoid this being a support issue requiring manual unbricking prochedures. > > > +static int ftdi_repair_brick(struct usb_serial_port *port) > > +{ > > + struct ftdi_private *priv = usb_get_serial_port_data(port); > > + int orig_latency; > > + int rv; > > + u16 *eeprom_data; > > + u16 checksum; > > + int eeprom_size; > > + int result; > > + > > + switch (priv->chip_type) { > > + case FT232RL: > > + eeprom_size = 0x40; > > + break; > > + default: > > + /* Unsupported for brick repair */ > > + return 0; > > + } > > + > > + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ > > + if (priv->latency != 0x77) { > > + orig_latency = priv->latency; > > + priv->latency = 0x77; > > + rv = write_latency_timer(port); > > + priv->latency = orig_latency; > > + if (rv < 0) > > + return -EIO; > > + } > > Do you really want to change this without returning to the original? > @@ -2255,6 +2364,12 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > ftdi_set_max_packet_size(port); > if (read_latency_timer(port) < 0) > priv->latency = 16; > + vendor_id = le16_to_cpu(port->serial->dev->descriptor.idVendor); > + product_id = le16_to_cpu(port->serial->dev->descriptor.idProduct); > + if (vendor_id == FTDI_VID && > + product_id == FTDI_BRICK_PID && > + priv->chip_type == FT232RL) > + ftdi_repair_brick(port); > write_latency_timer(port); It should get restored here. > create_sysfs_attrs(port); > > > Regards > Oliver >
On Wed, Sep 9, 2020 at 9:17 PM Hector Martin "marcan" <hector@marcansoft.com> wrote: > > > > On September 10, 2020 12:02:34 PM GMT+09:00, Oliver Neukum <oneukum@suse.de> wrote: > >Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > >> This patch detects and reverses the effects of the malicious FTDI > >> Windows driver version 2.12.00(FTDIgate). > > > >Hi, > > > >this raises questions. > >Should we do this unconditionally without asking? > >Does this belong into kernel space? > > I agree; this is very cute, but does it really need to be an automatic Linux feature? Presumably someone looking to fix a bricked FTDI chip can just run my script, and those who just want to use those chips with Linux already can since the driver binds to the zero PID. Well for one your script is not easily useable with embedded platforms like mine where I ran into this issue, I have no python2 interpreter available in my production builds. > > I am deeply amused by the idea of Linux automatically fixing problems caused by malicious Windows drivers, but thinking objectively, I'm not sure if that's the right thing to do. From my understanding Linux fixing up hardware issues caused by faulty/weird Windows drivers isn't exactly unusual. > > > > >> +static int ftdi_repair_brick(struct usb_serial_port *port) > >> +{ > >> + struct ftdi_private *priv = usb_get_serial_port_data(port); > >> + int orig_latency; > >> + int rv; > >> + u16 *eeprom_data; > >> + u16 checksum; > >> + int eeprom_size; > >> + int result; > >> + > >> + switch (priv->chip_type) { > >> + case FT232RL: > >> + eeprom_size = 0x40; > >> + break; > >> + default: > >> + /* Unsupported for brick repair */ > >> + return 0; > >> + } > >> + > >> + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ > >> + if (priv->latency != 0x77) { > >> + orig_latency = priv->latency; > >> + priv->latency = 0x77; > >> + rv = write_latency_timer(port); > >> + priv->latency = orig_latency; > >> + if (rv < 0) > >> + return -EIO; > >> + } > > > >Do you really want to change this without returning to the original? > > > > Regards > > Oliver > > -- > Hector Martin "marcan" (hector@marcansoft.com) > Public key: https://mrcn.st/pub
On September 10, 2020 12:40:59 PM GMT+09:00, James Hilliard <james.hilliard1@gmail.com> wrote: >On Wed, Sep 9, 2020 at 9:02 PM Oliver Neukum <oneukum@suse.de> wrote: >> >> Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: >> > This patch detects and reverses the effects of the malicious FTDI >> > Windows driver version 2.12.00(FTDIgate). >> >> Hi, >> >> this raises questions. >> Should we do this unconditionally without asking? >Well I think since we can reliably detect devices that have been >bricked by the malicious windows driver it's fine. I was careful to >ensure that this will bail out and not try to change anything unless >all >conditions match this specific brick attack. >> Does this belong into kernel space? >This seemed to be by far the simplest option for embedded systems >that need to automatically detect and repair the bricked eeproms. > >People seem to have plugged a bunch of counterfeit FTDI Arduino's >that normally attach to an embedded Linux host into windows for >some reason for a kiosk platform of mine which messed up the >userspace detection/mappings. This seemed like the best way to >avoid this being a support issue requiring manual unbricking >prochedures. If you need to update the kernel on this platform anyway to use this feature, why not just introduce a userspace script to fix the bricked units instead? Needing this autofixed seems like somewhat of a niche use case better served by a script on platforms where it might be a problem, rather than upstream kernel code. >> >> > +static int ftdi_repair_brick(struct usb_serial_port *port) >> > +{ >> > + struct ftdi_private *priv = usb_get_serial_port_data(port); >> > + int orig_latency; >> > + int rv; >> > + u16 *eeprom_data; >> > + u16 checksum; >> > + int eeprom_size; >> > + int result; >> > + >> > + switch (priv->chip_type) { >> > + case FT232RL: >> > + eeprom_size = 0x40; >> > + break; >> > + default: >> > + /* Unsupported for brick repair */ >> > + return 0; >> > + } >> > + >> > + /* Latency timer needs to be 0x77 to unlock EEPROM >programming */ >> > + if (priv->latency != 0x77) { >> > + orig_latency = priv->latency; >> > + priv->latency = 0x77; >> > + rv = write_latency_timer(port); >> > + priv->latency = orig_latency; >> > + if (rv < 0) >> > + return -EIO; >> > + } >> >> Do you really want to change this without returning to the original? >> @@ -2255,6 +2364,12 @@ static int ftdi_sio_port_probe(struct >usb_serial_port *port) >> ftdi_set_max_packet_size(port); >> if (read_latency_timer(port) < 0) >> priv->latency = 16; >> + vendor_id = >le16_to_cpu(port->serial->dev->descriptor.idVendor); >> + product_id = >le16_to_cpu(port->serial->dev->descriptor.idProduct); >> + if (vendor_id == FTDI_VID && >> + product_id == FTDI_BRICK_PID && >> + priv->chip_type == FT232RL) >> + ftdi_repair_brick(port); >> write_latency_timer(port); >It should get restored here. >> create_sysfs_attrs(port); >> >> >> Regards >> Oliver >>
On September 10, 2020 12:46:20 PM GMT+09:00, James Hilliard <james.hilliard1@gmail.com> wrote: >On Wed, Sep 9, 2020 at 9:17 PM Hector Martin "marcan" ><hector@marcansoft.com> wrote: >> >> >> >> On September 10, 2020 12:02:34 PM GMT+09:00, Oliver Neukum ><oneukum@suse.de> wrote: >> >Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: >> >> This patch detects and reverses the effects of the malicious FTDI >> >> Windows driver version 2.12.00(FTDIgate). >> > >> >Hi, >> > >> >this raises questions. >> >Should we do this unconditionally without asking? >> >Does this belong into kernel space? >> >> I agree; this is very cute, but does it really need to be an >automatic Linux feature? Presumably someone looking to fix a bricked >FTDI chip can just run my script, and those who just want to use those >chips with Linux already can since the driver binds to the zero PID. >Well for one your script is not easily useable with embedded platforms >like mine where I ran into this issue, I have no python2 interpreter >available in my production builds. Surely you can port the exact same algorithm to plain userspace C, as you did to kernel space C :) >> >> I am deeply amused by the idea of Linux automatically fixing problems >caused by malicious Windows drivers, but thinking objectively, I'm not >sure if that's the right thing to do. >From my understanding Linux fixing up hardware issues caused >by faulty/weird Windows drivers isn't exactly unusual. I'm not aware of any instances like this where nonvolatile memory is modified. At most you'll get things like resetting devices that a previous windows warm boot misconfigured, I think? >> >> > >> >> +static int ftdi_repair_brick(struct usb_serial_port *port) >> >> +{ >> >> + struct ftdi_private *priv = usb_get_serial_port_data(port); >> >> + int orig_latency; >> >> + int rv; >> >> + u16 *eeprom_data; >> >> + u16 checksum; >> >> + int eeprom_size; >> >> + int result; >> >> + >> >> + switch (priv->chip_type) { >> >> + case FT232RL: >> >> + eeprom_size = 0x40; >> >> + break; >> >> + default: >> >> + /* Unsupported for brick repair */ >> >> + return 0; >> >> + } >> >> + >> >> + /* Latency timer needs to be 0x77 to unlock EEPROM >programming */ >> >> + if (priv->latency != 0x77) { >> >> + orig_latency = priv->latency; >> >> + priv->latency = 0x77; >> >> + rv = write_latency_timer(port); >> >> + priv->latency = orig_latency; >> >> + if (rv < 0) >> >> + return -EIO; >> >> + } >> > >> >Do you really want to change this without returning to the original? >> > >> > Regards >> > Oliver >> >> -- >> Hector Martin "marcan" (hector@marcansoft.com) >> Public key: https://mrcn.st/pub
On Wed, Sep 9, 2020 at 9:47 PM Hector Martin "marcan" <hector@marcansoft.com> wrote: > > > > On September 10, 2020 12:40:59 PM GMT+09:00, James Hilliard <james.hilliard1@gmail.com> wrote: > >On Wed, Sep 9, 2020 at 9:02 PM Oliver Neukum <oneukum@suse.de> wrote: > >> > >> Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > >> > This patch detects and reverses the effects of the malicious FTDI > >> > Windows driver version 2.12.00(FTDIgate). > >> > >> Hi, > >> > >> this raises questions. > >> Should we do this unconditionally without asking? > >Well I think since we can reliably detect devices that have been > >bricked by the malicious windows driver it's fine. I was careful to > >ensure that this will bail out and not try to change anything unless > >all > >conditions match this specific brick attack. > >> Does this belong into kernel space? > >This seemed to be by far the simplest option for embedded systems > >that need to automatically detect and repair the bricked eeproms. > > > >People seem to have plugged a bunch of counterfeit FTDI Arduino's > >that normally attach to an embedded Linux host into windows for > >some reason for a kiosk platform of mine which messed up the > >userspace detection/mappings. This seemed like the best way to > >avoid this being a support issue requiring manual unbricking > >prochedures. > > If you need to update the kernel on this platform anyway to use this feature, why not just introduce a userspace script to fix the bricked units instead? I considered that but it appeared to be far more complex as I would have to validate all the interactions with my other userspace apps. I haven't tested this but I suspect there may also be issues having userspace code access these control messages, especially since I build the ftdi_sio driver into my kernels rather than using a module. > > Needing this autofixed seems like somewhat of a niche use case better served by a script on platforms where it might be a problem, rather than upstream kernel code. I figured this would be useful for anyone with these bricked devices as I have wasted a lot of time debugging this issue since I wasn't expecting the ID's to have changed. I'm assuming it happened because I use USB hubs and firmware was being flashed for different hub attached hardware devices by moving the entire hub to Windows even though the userspace arduino control application doesn't run on Windows at the moment. I can't think of any case where this would be harmful as I have plenty of sanity checks before fixing the brick to ensure it only unbricks devices that have the preimage attack signature. This is an annoying enough issue that I think autofixing makes sense simply so that everyone can avoid wasting time debugging the issue manually. > > >> > >> > +static int ftdi_repair_brick(struct usb_serial_port *port) > >> > +{ > >> > + struct ftdi_private *priv = usb_get_serial_port_data(port); > >> > + int orig_latency; > >> > + int rv; > >> > + u16 *eeprom_data; > >> > + u16 checksum; > >> > + int eeprom_size; > >> > + int result; > >> > + > >> > + switch (priv->chip_type) { > >> > + case FT232RL: > >> > + eeprom_size = 0x40; > >> > + break; > >> > + default: > >> > + /* Unsupported for brick repair */ > >> > + return 0; > >> > + } > >> > + > >> > + /* Latency timer needs to be 0x77 to unlock EEPROM > >programming */ > >> > + if (priv->latency != 0x77) { > >> > + orig_latency = priv->latency; > >> > + priv->latency = 0x77; > >> > + rv = write_latency_timer(port); > >> > + priv->latency = orig_latency; > >> > + if (rv < 0) > >> > + return -EIO; > >> > + } > >> > >> Do you really want to change this without returning to the original? > >> @@ -2255,6 +2364,12 @@ static int ftdi_sio_port_probe(struct > >usb_serial_port *port) > >> ftdi_set_max_packet_size(port); > >> if (read_latency_timer(port) < 0) > >> priv->latency = 16; > >> + vendor_id = > >le16_to_cpu(port->serial->dev->descriptor.idVendor); > >> + product_id = > >le16_to_cpu(port->serial->dev->descriptor.idProduct); > >> + if (vendor_id == FTDI_VID && > >> + product_id == FTDI_BRICK_PID && > >> + priv->chip_type == FT232RL) > >> + ftdi_repair_brick(port); > >> write_latency_timer(port); > >It should get restored here. > >> create_sysfs_attrs(port); > >> > >> > >> Regards > >> Oliver > >> > > -- > Hector Martin "marcan" (hector@marcansoft.com) > Public key: https://mrcn.st/pub
On Wed, Sep 9, 2020 at 9:49 PM Hector Martin "marcan" <hector@marcansoft.com> wrote: > > > > On September 10, 2020 12:46:20 PM GMT+09:00, James Hilliard <james.hilliard1@gmail.com> wrote: > >On Wed, Sep 9, 2020 at 9:17 PM Hector Martin "marcan" > ><hector@marcansoft.com> wrote: > >> > >> > >> > >> On September 10, 2020 12:02:34 PM GMT+09:00, Oliver Neukum > ><oneukum@suse.de> wrote: > >> >Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > >> >> This patch detects and reverses the effects of the malicious FTDI > >> >> Windows driver version 2.12.00(FTDIgate). > >> > > >> >Hi, > >> > > >> >this raises questions. > >> >Should we do this unconditionally without asking? > >> >Does this belong into kernel space? > >> > >> I agree; this is very cute, but does it really need to be an > >automatic Linux feature? Presumably someone looking to fix a bricked > >FTDI chip can just run my script, and those who just want to use those > >chips with Linux already can since the driver binds to the zero PID. > >Well for one your script is not easily useable with embedded platforms > >like mine where I ran into this issue, I have no python2 interpreter > >available in my production builds. > > Surely you can port the exact same algorithm to plain userspace C, as you did to kernel space C :) Sure, but it would be significantly more complex, require a lot more code and testing since there can be other userspace apps interacting with the hardware, in addition to being less reliable and potentially difficult to install for some setups. Detecting and dealing with this issue in the kernel is very simple and reliable in comparison. There's also potentially permissions issues if one wants to do this from userspace from my understanding. > > >> > >> I am deeply amused by the idea of Linux automatically fixing problems > >caused by malicious Windows drivers, but thinking objectively, I'm not > >sure if that's the right thing to do. > >From my understanding Linux fixing up hardware issues caused > >by faulty/weird Windows drivers isn't exactly unusual. > > I'm not aware of any instances like this where nonvolatile memory is modified. At most you'll get things like resetting devices that a previous windows warm boot misconfigured, I think? Yeah, I think it's mostly nonvolatile memory, I've seen this issue quite a bit with some of the Realtek ethernet drivers. I think user experience for devices should be that one can move a USB device from Linux to Windows and back without having to manually reprogram an eeprom. The sheer amount of resources FTDI has wasted with their malicious Windows driver is crazy and likely far exceeds any losses from counterfeiting. I think due to how widespread this issue is it makes sense to aggressively and automatically mitigate the damages wherever possible, it's also likely a major source of ewaste since people may throw out perfectly functional hardware without knowing it can be fixed easily. > > >> > >> > > >> >> +static int ftdi_repair_brick(struct usb_serial_port *port) > >> >> +{ > >> >> + struct ftdi_private *priv = usb_get_serial_port_data(port); > >> >> + int orig_latency; > >> >> + int rv; > >> >> + u16 *eeprom_data; > >> >> + u16 checksum; > >> >> + int eeprom_size; > >> >> + int result; > >> >> + > >> >> + switch (priv->chip_type) { > >> >> + case FT232RL: > >> >> + eeprom_size = 0x40; > >> >> + break; > >> >> + default: > >> >> + /* Unsupported for brick repair */ > >> >> + return 0; > >> >> + } > >> >> + > >> >> + /* Latency timer needs to be 0x77 to unlock EEPROM > >programming */ > >> >> + if (priv->latency != 0x77) { > >> >> + orig_latency = priv->latency; > >> >> + priv->latency = 0x77; > >> >> + rv = write_latency_timer(port); > >> >> + priv->latency = orig_latency; > >> >> + if (rv < 0) > >> >> + return -EIO; > >> >> + } > >> > > >> >Do you really want to change this without returning to the original? > >> > > >> > Regards > >> > Oliver > >> > >> -- > >> Hector Martin "marcan" (hector@marcansoft.com) > >> Public key: https://mrcn.st/pub > > -- > Hector Martin "marcan" (hector@marcansoft.com) > Public key: https://mrcn.st/pub
On 9/10/2020 10:02, Oliver Neukum wrote: > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: >> This patch detects and reverses the effects of the malicious FTDI >> Windows driver version 2.12.00(FTDIgate). > > Hi, > > this raises questions. > Should we do this unconditionally without asking? > Does this belong into kernel space? > My answer to both of those question is a strong NO. The patch author tries to justify the patch with egoistical arguments (easier for him and his customers) without thinking of all other users of memory constrained embedded hardware that doesn't need the patch code but have to carry it. The bricked PID is btw already supported by the linux ftdi driver so there is no functionality gain in the patch. br Lars
On Wed, Sep 09, 2020 at 01:34:18PM -0600, James Hilliard wrote: > This patch detects and reverses the effects of the malicious FTDI > Windows driver version 2.12.00(FTDIgate). > > While we currently load the ftdi_sio driver for devices with > FTDI_BRICK_PID(0x0000) userspace applications that expect the > appropriate FTDI_8U232AM_PID(0x6001) PID may not work properly. > > Since the malicious FTDI driver modifies the PID without modifying > the normal checksum field we can detect and limit the unbricking > to devices that were bricked specifically using the FTDI checksum > preimage attack technique used by the official Windows drivers. > > This should have no effect on devices where the PID was deliberately > set to FTDI_BRICK_PID(0x0000) as the checksum would normally change > and the preimage target(address 62) should be 0. We validate that > the preimage target is not 0 before attempting to unbrick. > > Since we only write to even addresses this should have no effect at > all on non-counterfeit FTDI hardware due to the hardware only > committing EEPROM writes when odd addresses are written. > > References: > https://marcan.st/transf/detect_ftdi_clone.py > https://hackaday.com/2014/10/22/watch-that-windows-update-ftdi-drivers-are-killing-fake-chips/ > https://www.eevblog.com/forum/reviews/ftdi-driver-kills-fake-ftdi-ft232/msg535270/#msg535270 > https://lkml.org/lkml/2014/10/23/266 > https://lore.kernel.org/patchwork/patch/509929/ > https://lore.kernel.org/patchwork/patch/510097/ > > Signed-off-by: Russ Dill <Russ.Dill@gmail.com> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > Cc: Hector Martin <hector@marcansoft.com> > --- > Changes v1 -> v2: > - Move ftdi_read_eeprom and ftdi_write_eeprom outside #ifdef CONFIG_GPIOLIB > --- > drivers/usb/serial/ftdi_sio.c | 181 +++++++++++++++++++++++++++------- > drivers/usb/serial/ftdi_sio.h | 4 + > 2 files changed, 152 insertions(+), 33 deletions(-) This is interesting, but not something that is really needed on a Linux machine right? The "bricked" devices work just fine on Linux, and Linux does not cause these devices to ever be "bricked", so why build into the Linux kernel an option that only is needed by another operating system? If you have "bricked" devices, you can use the userspace tool to fix them up so that they can work properly on other operating systems, as a utility. But to build this into Linux feels very odd to me. That being said, code review: > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index 871cdccf3a5f..85324e2ea107 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -1062,6 +1062,9 @@ static const char *ftdi_chip_name[] = { > /* function prototypes for a FTDI serial converter */ > static int ftdi_sio_probe(struct usb_serial *serial, > const struct usb_device_id *id); > +static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, > + u16 nbytes); No need for this, just keep this function in the same place in the file, right? > +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data); > static int ftdi_sio_port_probe(struct usb_serial_port *port); > static int ftdi_sio_port_remove(struct usb_serial_port *port); > static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port); > @@ -1996,39 +1999,6 @@ static int ftdi_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, > return result; > } > > -static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, > - u16 nbytes) > -{ > - int read = 0; > - > - if (addr % 2 != 0) > - return -EINVAL; > - if (nbytes % 2 != 0) > - return -EINVAL; > - > - /* Read EEPROM two bytes at a time */ > - while (read < nbytes) { > - int rv; > - > - rv = usb_control_msg(serial->dev, > - usb_rcvctrlpipe(serial->dev, 0), > - FTDI_SIO_READ_EEPROM_REQUEST, > - FTDI_SIO_READ_EEPROM_REQUEST_TYPE, > - 0, (addr + read) / 2, dst + read, 2, > - WDR_TIMEOUT); > - if (rv < 2) { > - if (rv >= 0) > - return -EIO; > - else > - return rv; > - } > - > - read += rv; > - } > - > - return 0; > -} Why move this function? > - > static int ftdi_gpio_init_ft232h(struct usb_serial_port *port) > { > struct ftdi_private *priv = usb_get_serial_port_data(port); > @@ -2234,10 +2204,149 @@ static int ftdi_sio_probe(struct usb_serial *serial, > return 0; > } > > +static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, > + u16 nbytes) > +{ > + int read = 0; > + > + if (addr % 2 != 0) > + return -EINVAL; > + if (nbytes % 2 != 0) > + return -EINVAL; > + > + /* Read EEPROM two bytes at a time */ > + while (read < nbytes) { > + int rv; > + > + rv = usb_control_msg(serial->dev, > + usb_rcvctrlpipe(serial->dev, 0), > + FTDI_SIO_READ_EEPROM_REQUEST, > + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, > + 0, (addr + read) / 2, dst + read, 2, > + WDR_TIMEOUT); > + if (rv < 2) { > + if (rv >= 0) > + return -EIO; > + else > + return rv; > + } > + > + read += rv; > + } > + > + return 0; > +} > + > +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data) > +{ > + struct usb_device *udev = port->serial->dev; > + int rv; > + > + rv = usb_control_msg(udev, > + usb_sndctrlpipe(udev, 0), > + FTDI_SIO_WRITE_EEPROM_REQUEST, > + FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE, > + data, addr, > + NULL, 0, WDR_TIMEOUT); > + if (rv < 0) > + dev_err(&port->dev, "Unable to write EEPROM: %i\n", rv); You don't check for a "short write"? > + return rv; > +} > + > +static u16 ftdi_checksum(u16 *data, int n) > +{ > + u16 checksum; > + int i; > + > + checksum = 0xaaaa; > + for (i = 0; i < n - 1; i++) { > + checksum ^= be16_to_cpu(data[i]); > + checksum = (checksum << 1) | (checksum >> 15); > + } What type of function is this, don't we have all of the needed checksum functions in the kernel already? > + > + return cpu_to_be16(checksum); > +} > + > +static int ftdi_repair_brick(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + int orig_latency; > + int rv; > + u16 *eeprom_data; > + u16 checksum; > + int eeprom_size; > + int result; > + > + switch (priv->chip_type) { > + case FT232RL: > + eeprom_size = 0x40; > + break; > + default: > + /* Unsupported for brick repair */ > + return 0; > + } > + > + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ > + if (priv->latency != 0x77) { > + orig_latency = priv->latency; > + priv->latency = 0x77; > + rv = write_latency_timer(port); > + priv->latency = orig_latency; So you change the latency, and then lie that it is changed? > + if (rv < 0) > + return -EIO; > + } > + > + eeprom_data = kmalloc(eeprom_size * 2, GFP_KERNEL); > + if (!eeprom_data) > + return -ENOMEM; > + > + /* Read in EEPROM */ > + result = ftdi_read_eeprom(port->serial, eeprom_data, 0x00, eeprom_size * 2); > + if (result < 0) > + goto end_repair_brick; > + > + /* Verify EEPROM is valid */ > + checksum = ftdi_checksum(eeprom_data, eeprom_size); > + if (checksum != eeprom_data[eeprom_size - 1]) > + goto end_repair_brick; > + > + /* Skip if no preimage attack against target address 62 */ > + if (eeprom_data[62] == 0) > + goto end_repair_brick; Here, and later on, if you error out, you are not setting the return value to be an error, why? > + > + /* Attempt to restore Product ID to 0x6001 */ > + eeprom_data[2] = FTDI_8U232AM_PID; > + > + /* Clear preimage attack target address */ > + eeprom_data[62] = 0; > + > + /* Calculate and verify new checksum */ > + checksum = ftdi_checksum(eeprom_data, eeprom_size); > + if (checksum != eeprom_data[eeprom_size - 1]) > + goto end_repair_brick; > + > + /* Restore EEPROM PID to original pre-brick state */ > + if (ftdi_write_eeprom(port, 2, eeprom_data[2]) < 0) > + goto end_repair_brick; > + > + /* Restore EEPROM preimage target address to original pre-brick state */ > + if (ftdi_write_eeprom(port, 62, eeprom_data[62]) < 0) > + goto end_repair_brick; > + > + dev_info(&port->dev, "Successfully repaired eeprom bricked by FTDI's malicious Windows driver.\n"); > + > +end_repair_brick: > + kfree(eeprom_data); > + > + return result; > +} > + > static int ftdi_sio_port_probe(struct usb_serial_port *port) > { > struct ftdi_private *priv; > const struct ftdi_sio_quirk *quirk = usb_get_serial_data(port->serial); > + u16 vendor_id; > + u16 product_id; > int result; > > priv = kzalloc(sizeof(struct ftdi_private), GFP_KERNEL); > @@ -2255,6 +2364,12 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > ftdi_set_max_packet_size(port); > if (read_latency_timer(port) < 0) > priv->latency = 16; > + vendor_id = le16_to_cpu(port->serial->dev->descriptor.idVendor); > + product_id = le16_to_cpu(port->serial->dev->descriptor.idProduct); > + if (vendor_id == FTDI_VID && > + product_id == FTDI_BRICK_PID && > + priv->chip_type == FT232RL) > + ftdi_repair_brick(port); So now you are forcing all devices with that id to go through this function? How long does this function take? What about devices that were working just fine with Linux with that id and did not want their eeproms to be "fixed"? thanks, greg k-h
On Wed, Sep 9, 2020 at 11:49 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 09, 2020 at 01:34:18PM -0600, James Hilliard wrote: > > This patch detects and reverses the effects of the malicious FTDI > > Windows driver version 2.12.00(FTDIgate). > > > > While we currently load the ftdi_sio driver for devices with > > FTDI_BRICK_PID(0x0000) userspace applications that expect the > > appropriate FTDI_8U232AM_PID(0x6001) PID may not work properly. > > > > Since the malicious FTDI driver modifies the PID without modifying > > the normal checksum field we can detect and limit the unbricking > > to devices that were bricked specifically using the FTDI checksum > > preimage attack technique used by the official Windows drivers. > > > > This should have no effect on devices where the PID was deliberately > > set to FTDI_BRICK_PID(0x0000) as the checksum would normally change > > and the preimage target(address 62) should be 0. We validate that > > the preimage target is not 0 before attempting to unbrick. > > > > Since we only write to even addresses this should have no effect at > > all on non-counterfeit FTDI hardware due to the hardware only > > committing EEPROM writes when odd addresses are written. > > > > References: > > https://marcan.st/transf/detect_ftdi_clone.py > > https://hackaday.com/2014/10/22/watch-that-windows-update-ftdi-drivers-are-killing-fake-chips/ > > https://www.eevblog.com/forum/reviews/ftdi-driver-kills-fake-ftdi-ft232/msg535270/#msg535270 > > https://lkml.org/lkml/2014/10/23/266 > > https://lore.kernel.org/patchwork/patch/509929/ > > https://lore.kernel.org/patchwork/patch/510097/ > > > > Signed-off-by: Russ Dill <Russ.Dill@gmail.com> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > Cc: Hector Martin <hector@marcansoft.com> > > --- > > Changes v1 -> v2: > > - Move ftdi_read_eeprom and ftdi_write_eeprom outside #ifdef CONFIG_GPIOLIB > > --- > > drivers/usb/serial/ftdi_sio.c | 181 +++++++++++++++++++++++++++------- > > drivers/usb/serial/ftdi_sio.h | 4 + > > 2 files changed, 152 insertions(+), 33 deletions(-) > > This is interesting, but not something that is really needed on a Linux > machine right? The "bricked" devices work just fine on Linux, and Linux > does not cause these devices to ever be "bricked", so why build into the > Linux kernel an option that only is needed by another operating system? Well the PID's being wrong messes up my Linux userspace applications that match against the device VID/PID's, so while the driver does load the serial interface the userspace application may not see it properly. Effectively my setup relies on stable USB identifiers, if the Windows driver messes those up my applications don't work correctly, and I have little control over these devices getting plugged into Windows systems so I need to automatically detect and set the eeprom changes back to their expected values. > > If you have "bricked" devices, you can use the userspace tool to fix > them up so that they can work properly on other operating systems, as a > utility. But to build this into Linux feels very odd to me. Yeah, I originally looked into doing this in userspace but I didn't see a clean way to fix it there without having a bunch of side effects and complex service management. By doing it in the driver we can fix it before userspace even sees the device. > > That being said, code review: > > > > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > > index 871cdccf3a5f..85324e2ea107 100644 > > --- a/drivers/usb/serial/ftdi_sio.c > > +++ b/drivers/usb/serial/ftdi_sio.c > > @@ -1062,6 +1062,9 @@ static const char *ftdi_chip_name[] = { > > /* function prototypes for a FTDI serial converter */ > > static int ftdi_sio_probe(struct usb_serial *serial, > > const struct usb_device_id *id); > > +static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, > > + u16 nbytes); > > No need for this, just keep this function in the same place in the file, > right? It needs to be outside of the #ifdef CONFIG_GPIOLIB. > > > +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data); > > static int ftdi_sio_port_probe(struct usb_serial_port *port); > > static int ftdi_sio_port_remove(struct usb_serial_port *port); > > static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port); > > @@ -1996,39 +1999,6 @@ static int ftdi_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, > > return result; > > } > > > > -static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, > > - u16 nbytes) > > -{ > > - int read = 0; > > - > > - if (addr % 2 != 0) > > - return -EINVAL; > > - if (nbytes % 2 != 0) > > - return -EINVAL; > > - > > - /* Read EEPROM two bytes at a time */ > > - while (read < nbytes) { > > - int rv; > > - > > - rv = usb_control_msg(serial->dev, > > - usb_rcvctrlpipe(serial->dev, 0), > > - FTDI_SIO_READ_EEPROM_REQUEST, > > - FTDI_SIO_READ_EEPROM_REQUEST_TYPE, > > - 0, (addr + read) / 2, dst + read, 2, > > - WDR_TIMEOUT); > > - if (rv < 2) { > > - if (rv >= 0) > > - return -EIO; > > - else > > - return rv; > > - } > > - > > - read += rv; > > - } > > - > > - return 0; > > -} > > Why move this function? So that it still gets included when building without CONFIG_GPIOLIB. > > > - > > static int ftdi_gpio_init_ft232h(struct usb_serial_port *port) > > { > > struct ftdi_private *priv = usb_get_serial_port_data(port); > > @@ -2234,10 +2204,149 @@ static int ftdi_sio_probe(struct usb_serial *serial, > > return 0; > > } > > > > +static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, > > + u16 nbytes) > > +{ > > + int read = 0; > > + > > + if (addr % 2 != 0) > > + return -EINVAL; > > + if (nbytes % 2 != 0) > > + return -EINVAL; > > + > > + /* Read EEPROM two bytes at a time */ > > + while (read < nbytes) { > > + int rv; > > + > > + rv = usb_control_msg(serial->dev, > > + usb_rcvctrlpipe(serial->dev, 0), > > + FTDI_SIO_READ_EEPROM_REQUEST, > > + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, > > + 0, (addr + read) / 2, dst + read, 2, > > + WDR_TIMEOUT); > > + if (rv < 2) { > > + if (rv >= 0) > > + return -EIO; > > + else > > + return rv; > > + } > > + > > + read += rv; > > + } > > + > > + return 0; > > +} > > + > > +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data) > > +{ > > + struct usb_device *udev = port->serial->dev; > > + int rv; > > + > > + rv = usb_control_msg(udev, > > + usb_sndctrlpipe(udev, 0), > > + FTDI_SIO_WRITE_EEPROM_REQUEST, > > + FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE, > > + data, addr, > > + NULL, 0, WDR_TIMEOUT); > > + if (rv < 0) > > + dev_err(&port->dev, "Unable to write EEPROM: %i\n", rv); > > You don't check for a "short write"? From my understanding the hardware only accepts 2 byte writes, and the non-counterfeits actually only commit writes on odd addresses while they buffer writes on even(this difference is what FTDI's windows driver exploits). So I guess this should be "if (rv < 2)"? > > > + return rv; > > +} > > + > > +static u16 ftdi_checksum(u16 *data, int n) > > +{ > > + u16 checksum; > > + int i; > > + > > + checksum = 0xaaaa; > > + for (i = 0; i < n - 1; i++) { > > + checksum ^= be16_to_cpu(data[i]); > > + checksum = (checksum << 1) | (checksum >> 15); > > + } > > What type of function is this, don't we have all of the needed checksum > functions in the kernel already? Some custom crc16 style checksum I guess, I'm not seeing anything in the kernel that's the same, although I might not be looking in the right places. > > > + > > + return cpu_to_be16(checksum); > > +} > > + > > +static int ftdi_repair_brick(struct usb_serial_port *port) > > +{ > > + struct ftdi_private *priv = usb_get_serial_port_data(port); > > + int orig_latency; > > + int rv; > > + u16 *eeprom_data; > > + u16 checksum; > > + int eeprom_size; > > + int result; > > + > > + switch (priv->chip_type) { > > + case FT232RL: > > + eeprom_size = 0x40; > > + break; > > + default: > > + /* Unsupported for brick repair */ > > + return 0; > > + } > > + > > + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ > > + if (priv->latency != 0x77) { > > + orig_latency = priv->latency; > > + priv->latency = 0x77; > > + rv = write_latency_timer(port); > > + priv->latency = orig_latency; > > So you change the latency, and then lie that it is changed? This value will get applied immediately after ftdi_repair_brick finishes, we run ftdi_repair_brick before the original latency ever actually gets applied. See: https://github.com/torvalds/linux/blob/v5.8/drivers/usb/serial/ftdi_sio.c#L2258 > > > + if (rv < 0) > > + return -EIO; > > + } > > + > > + eeprom_data = kmalloc(eeprom_size * 2, GFP_KERNEL); > > + if (!eeprom_data) > > + return -ENOMEM; > > + > > + /* Read in EEPROM */ > > + result = ftdi_read_eeprom(port->serial, eeprom_data, 0x00, eeprom_size * 2); > > + if (result < 0) > > + goto end_repair_brick; > > + > > + /* Verify EEPROM is valid */ > > + checksum = ftdi_checksum(eeprom_data, eeprom_size); > > + if (checksum != eeprom_data[eeprom_size - 1]) > > + goto end_repair_brick; > > + > > + /* Skip if no preimage attack against target address 62 */ > > + if (eeprom_data[62] == 0) > > + goto end_repair_brick; > > Here, and later on, if you error out, you are not setting the return > value to be an error, why? This means the device does not appear to have been attacked by the Windows driver, so we bail out without an error and assume the 0x0000 PID was intentionally set in a normal way. The normal PID changing utilities should leave address 62 as the default 0x0000 and instead change the real checksum which is at address 63. > > > > + > > + /* Attempt to restore Product ID to 0x6001 */ > > + eeprom_data[2] = FTDI_8U232AM_PID; > > + > > + /* Clear preimage attack target address */ > > + eeprom_data[62] = 0; > > + > > + /* Calculate and verify new checksum */ > > + checksum = ftdi_checksum(eeprom_data, eeprom_size); > > + if (checksum != eeprom_data[eeprom_size - 1]) > > + goto end_repair_brick; > > + > > + /* Restore EEPROM PID to original pre-brick state */ > > + if (ftdi_write_eeprom(port, 2, eeprom_data[2]) < 0) > > + goto end_repair_brick; > > + > > + /* Restore EEPROM preimage target address to original pre-brick state */ > > + if (ftdi_write_eeprom(port, 62, eeprom_data[62]) < 0) > > + goto end_repair_brick; > > + > > + dev_info(&port->dev, "Successfully repaired eeprom bricked by FTDI's malicious Windows driver.\n"); > > + > > +end_repair_brick: > > + kfree(eeprom_data); > > + > > + return result; > > +} > > + > > static int ftdi_sio_port_probe(struct usb_serial_port *port) > > { > > struct ftdi_private *priv; > > const struct ftdi_sio_quirk *quirk = usb_get_serial_data(port->serial); > > + u16 vendor_id; > > + u16 product_id; > > int result; > > > > priv = kzalloc(sizeof(struct ftdi_private), GFP_KERNEL); > > @@ -2255,6 +2364,12 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > > ftdi_set_max_packet_size(port); > > if (read_latency_timer(port) < 0) > > priv->latency = 16; > > + vendor_id = le16_to_cpu(port->serial->dev->descriptor.idVendor); > > + product_id = le16_to_cpu(port->serial->dev->descriptor.idProduct); > > + if (vendor_id == FTDI_VID && > > + product_id == FTDI_BRICK_PID && > > + priv->chip_type == FT232RL) > > + ftdi_repair_brick(port); > > So now you are forcing all devices with that id to go through this > function? How long does this function take? What about devices that > were working just fine with Linux with that id and did not want their > eeproms to be "fixed"? The ftdi_repair_brick function has checks that will bail out if FTDI_BRICK_PID was intentionally set. This is because intentionally setting the PID using normal tools changes the checksum, while FTDI's driver does a preimage attack against the checksum instead. We only fix those that had their PID changed via the preimage attack and not those that were changed intentionally. I haven't benchmarked this properly but it's under 100ms from the looks of it, if that is too slow I can optimize it to bail out much faster pretty easily for intentionally configured FTDI_BRICK_PID's as a fast preimage sanity check would be a single read. Keep in mind though that it would be pretty rare for a device to intentionally use FTDI_BRICK_PID. > > thanks, > > greg k-h
On Wed, Sep 9, 2020 at 11:34 PM Lars Melin <larsm17@gmail.com> wrote: > > On 9/10/2020 10:02, Oliver Neukum wrote: > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > >> This patch detects and reverses the effects of the malicious FTDI > >> Windows driver version 2.12.00(FTDIgate). > > > > Hi, > > > > this raises questions. > > Should we do this unconditionally without asking? > > Does this belong into kernel space? > > > > My answer to both of those question is a strong NO. > > The patch author tries to justify the patch with egoistical arguments > (easier for him and his customers) without thinking of all other users > of memory constrained embedded hardware that doesn't need the patch code > but have to carry it. If that's a concern it would not be difficult to add a kconfig option to allow disabling it. > > The bricked PID is btw already supported by the linux ftdi driver so > there is no functionality gain in the patch. By the kernel driver sure, but userspace is where things get messed up without something like this. > > br > Lars > > >
On Thu, Sep 10, 2020 at 12:48 AM James Hilliard <james.hilliard1@gmail.com> wrote: > > On Wed, Sep 9, 2020 at 11:34 PM Lars Melin <larsm17@gmail.com> wrote: > > > > On 9/10/2020 10:02, Oliver Neukum wrote: > > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > > >> This patch detects and reverses the effects of the malicious FTDI > > >> Windows driver version 2.12.00(FTDIgate). > > > > > > Hi, > > > > > > this raises questions. > > > Should we do this unconditionally without asking? > > > Does this belong into kernel space? > > > > > > > My answer to both of those question is a strong NO. > > > > The patch author tries to justify the patch with egoistical arguments > > (easier for him and his customers) without thinking of all other users > > of memory constrained embedded hardware that doesn't need the patch code > > but have to carry it. > If that's a concern it would not be difficult to add a kconfig option to allow > disabling it. I should maybe add that the reason I'm trying to upstream this is because I have been repeatedly bitten by this issue for years over many totally unrelated projects, as have many people I know. If this was a one-off issue I would not have spent the time writing a kernel patch to fix it. The supply chains must be heavily contaminated with counterfeits with how often I've personally run into this problem. The damage done by FTDI with their malicious drivers really is hard to quantify, both in terms of wasted man hours working around this issue and in the inevitable mountains of e-waste they create by bricking end user hardware. Most of these customers have likely never even heard of FTDI, they even intentionally designed their malicious drivers to make it non-obvious that the failures are due to counterfeits, completely unethical behavior IMO. FTDI may very well be one of the least environmentally friendly companies in terms of environmental damage per dollar of revenue. > > > > The bricked PID is btw already supported by the linux ftdi driver so > > there is no functionality gain in the patch. > By the kernel driver sure, but userspace is where things get messed up > without something like this. > > > > br > > Lars > > > > > >
On Thu, Sep 10, 2020 at 12:33:55PM +0700, Lars Melin wrote: > On 9/10/2020 10:02, Oliver Neukum wrote: > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > >> This patch detects and reverses the effects of the malicious FTDI > >> Windows driver version 2.12.00(FTDIgate). > > > > Hi, > > > > this raises questions. > > Should we do this unconditionally without asking? > > Does this belong into kernel space? > > > > My answer to both of those question is a strong NO. > > The patch author tries to justify the patch with egoistical arguments > (easier for him and his customers) without thinking of all other users > of memory constrained embedded hardware that doesn't need the patch code > but have to carry it. > > The bricked PID is btw already supported by the linux ftdi driver so > there is no functionality gain in the patch. I fully agree. This doesn't belong in the kernel. If the Windows driver breaks someones device on purpose they should know about it, and *if* they want they can reprogram the device using the tools mentioned in the thread. But the kernel shouldn't be playing such games and reprogram eeproms behind people's backs. Johan
On 10/09/2020 15.45, James Hilliard wrote: >>> +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data) >>> +{ >>> + struct usb_device *udev = port->serial->dev; >>> + int rv; >>> + >>> + rv = usb_control_msg(udev, >>> + usb_sndctrlpipe(udev, 0), >>> + FTDI_SIO_WRITE_EEPROM_REQUEST, >>> + FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE, >>> + data, addr, >>> + NULL, 0, WDR_TIMEOUT); >>> + if (rv < 0) >>> + dev_err(&port->dev, "Unable to write EEPROM: %i\n", rv); >> >> You don't check for a "short write"? > From my understanding the hardware only accepts 2 byte writes, and > the non-counterfeits actually only commit writes on odd addresses > while they buffer writes on even(this difference is what FTDI's windows > driver exploits). So I guess this should be "if (rv < 2)"? It's not "data" anyway, the data word gets sent in control message headers. Unless I'm mistaken rv == 0 on success, so the code should be correct as-is. >> >>> + return rv; >>> +} >>> + >>> +static u16 ftdi_checksum(u16 *data, int n) >>> +{ >>> + u16 checksum; >>> + int i; >>> + >>> + checksum = 0xaaaa; >>> + for (i = 0; i < n - 1; i++) { >>> + checksum ^= be16_to_cpu(data[i]); >>> + checksum = (checksum << 1) | (checksum >> 15); >>> + } >> >> What type of function is this, don't we have all of the needed checksum >> functions in the kernel already? > Some custom crc16 style checksum I guess, I'm not seeing anything > in the kernel that's the same, although I might not be looking in the > right places. This isn't a CRC, it's some random xor all the words thing with a somewhat pointless rotation in the way. I'd be surprised if anything elses uses this particular function. Pretty sure other drivers are littered with stuff like this too, hardware manufacturers love to reinvent checksums.
On Thu, Sep 10, 2020 at 2:08 AM Johan Hovold <johan@kernel.org> wrote: > > On Thu, Sep 10, 2020 at 12:33:55PM +0700, Lars Melin wrote: > > On 9/10/2020 10:02, Oliver Neukum wrote: > > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > > >> This patch detects and reverses the effects of the malicious FTDI > > >> Windows driver version 2.12.00(FTDIgate). > > > > > > Hi, > > > > > > this raises questions. > > > Should we do this unconditionally without asking? > > > Does this belong into kernel space? > > > > > > > My answer to both of those question is a strong NO. > > > > The patch author tries to justify the patch with egoistical arguments > > (easier for him and his customers) without thinking of all other users > > of memory constrained embedded hardware that doesn't need the patch code > > but have to carry it. > > > > The bricked PID is btw already supported by the linux ftdi driver so > > there is no functionality gain in the patch. > > I fully agree. This doesn't belong in the kernel. If the Windows driver > breaks someones device on purpose they should know about it, and *if* > they want they can reprogram the device using the tools mentioned in the > thread. But the kernel shouldn't be playing such games and reprogram > eeproms behind people's backs. One of the main issues is that this issue is very often not-obvious, FTDI specifically designed their malicious driver to make it appear that the hardware failed, they intentionally do not provide proper feedback to the user when they soft-brick it. I assume this is because they want to push the support costs related to their malicious driver onto the integrator rather than themselves. > > Johan
On Thu, Sep 10, 2020 at 02:17:44AM -0600, James Hilliard wrote: > On Thu, Sep 10, 2020 at 2:08 AM Johan Hovold <johan@kernel.org> wrote: > > > > On Thu, Sep 10, 2020 at 12:33:55PM +0700, Lars Melin wrote: > > > On 9/10/2020 10:02, Oliver Neukum wrote: > > > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > > > >> This patch detects and reverses the effects of the malicious FTDI > > > >> Windows driver version 2.12.00(FTDIgate). > > > > > > > > Hi, > > > > > > > > this raises questions. > > > > Should we do this unconditionally without asking? > > > > Does this belong into kernel space? > > > > > > > > > > My answer to both of those question is a strong NO. > > > > > > The patch author tries to justify the patch with egoistical arguments > > > (easier for him and his customers) without thinking of all other users > > > of memory constrained embedded hardware that doesn't need the patch code > > > but have to carry it. > > > > > > The bricked PID is btw already supported by the linux ftdi driver so > > > there is no functionality gain in the patch. > > > > I fully agree. This doesn't belong in the kernel. If the Windows driver > > breaks someones device on purpose they should know about it, and *if* > > they want they can reprogram the device using the tools mentioned in the > > thread. But the kernel shouldn't be playing such games and reprogram > > eeproms behind people's backs. > One of the main issues is that this issue is very often not-obvious, FTDI > specifically designed their malicious driver to make it appear that the > hardware failed, they intentionally do not provide proper feedback to > the user when they soft-brick it. I assume this is because they want > to push the support costs related to their malicious driver onto the > integrator rather than themselves. That's fine, but why is it the Linux kernel's job to fix up this mess? There is already a userspace tool that can be run to resolve this for devices that wish to have this fixed up for. Use that. We want to keep things that can be done in userspace, in userspace, whenever possible. And again, Linux runs just fine with these devices so why is it Linux's I'm with Johan here, reprogramming eeproms when people least expect it is not nice, and in a way, is much the same thing that the Windows drivers are doing. thanks, greg k-h
On Thu, Sep 10, 2020 at 2:55 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Sep 10, 2020 at 02:17:44AM -0600, James Hilliard wrote: > > On Thu, Sep 10, 2020 at 2:08 AM Johan Hovold <johan@kernel.org> wrote: > > > > > > On Thu, Sep 10, 2020 at 12:33:55PM +0700, Lars Melin wrote: > > > > On 9/10/2020 10:02, Oliver Neukum wrote: > > > > > Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard: > > > > >> This patch detects and reverses the effects of the malicious FTDI > > > > >> Windows driver version 2.12.00(FTDIgate). > > > > > > > > > > Hi, > > > > > > > > > > this raises questions. > > > > > Should we do this unconditionally without asking? > > > > > Does this belong into kernel space? > > > > > > > > > > > > > My answer to both of those question is a strong NO. > > > > > > > > The patch author tries to justify the patch with egoistical arguments > > > > (easier for him and his customers) without thinking of all other users > > > > of memory constrained embedded hardware that doesn't need the patch code > > > > but have to carry it. > > > > > > > > The bricked PID is btw already supported by the linux ftdi driver so > > > > there is no functionality gain in the patch. > > > > > > I fully agree. This doesn't belong in the kernel. If the Windows driver > > > breaks someones device on purpose they should know about it, and *if* > > > they want they can reprogram the device using the tools mentioned in the > > > thread. But the kernel shouldn't be playing such games and reprogram > > > eeproms behind people's backs. > > One of the main issues is that this issue is very often not-obvious, FTDI > > specifically designed their malicious driver to make it appear that the > > hardware failed, they intentionally do not provide proper feedback to > > the user when they soft-brick it. I assume this is because they want > > to push the support costs related to their malicious driver onto the > > integrator rather than themselves. > > That's fine, but why is it the Linux kernel's job to fix up this mess? Well the kernel seems to be the place a fix would be most effective. Not like it's unusual for the kernel to work around hardware issues in general. :P > > There is already a userspace tool that can be run to resolve this for > devices that wish to have this fixed up for. Use that. We want to keep > things that can be done in userspace, in userspace, whenever possible. So I'm having trouble coming up with a reliable way to fix this in userspace, I've already got quite a few moving parts there as is and most of what I come up with seems like it would not work reliably, at least for automatically repairing the eeprom. > > And again, Linux runs just fine with these devices so why is it Linux's > > I'm with Johan here, reprogramming eeproms when people least expect it > is not nice, and in a way, is much the same thing that the Windows > drivers are doing. Yeah, it does seem a bit sketchy at first, I went with this approach mostly since I couldn't think of a practical scenario where fixing it automatically would be a real issue, assuming we can reliably detect the preimage attack. So maybe identify the preimage attack and log a message instead? From my understanding false positives should be nearly impossible with the signature identification technique I'm using. Maybe we could expose an interface that triggers the eeprom repair, one which doesn't require userspace to implement low level USB handling? > > thanks, > > greg k-h
On 10/09/2020 18.52, James Hilliard wrote: > So I'm having trouble coming up with a reliable way to fix this in userspace, > I've already got quite a few moving parts there as is and most of what I > come up with seems like it would not work reliably, at least for automatically > repairing the eeprom. I'm confused as to why this is hard to fix in userspace. You already said you have userspace code binding to the proper VID/PID, so your code won't find the bricked device. Then it's just a matter of having a udev rule run the unbricker when it detects the bad device (which should issue a USB reset when it's done reprogramming, making the device appear as the right VID/PID), thus effectively doing the same thing the kernel does. If this is embedded and not using udev, then substitute whatever equivalent you have. If you're polling for the device at runtime instead and don't have a device manager, just poll for the VID 0 device too and apply the fix. I can't think of a scenario where this would be difficult to fix in userspace...
On Thu, Sep 10, 2020 at 3:57 AM Hector Martin <hector@marcansoft.com> wrote: > > On 10/09/2020 18.52, James Hilliard wrote: > > So I'm having trouble coming up with a reliable way to fix this in userspace, > > I've already got quite a few moving parts there as is and most of what I > > come up with seems like it would not work reliably, at least for automatically > > repairing the eeprom. > > I'm confused as to why this is hard to fix in userspace. You already > said you have userspace code binding to the proper VID/PID, so your code > won't find the bricked device. Then it's just a matter of having a udev > rule run the unbricker when it detects the bad device (which should > issue a USB reset when it's done reprogramming, making the device appear > as the right VID/PID), thus effectively doing the same thing the kernel > does. If this is embedded and not using udev, then substitute whatever > equivalent you have. If you're polling for the device at runtime instead > and don't have a device manager, just poll for the VID 0 device too and > apply the fix. Wouldn't you have to do a bunch of stuff like unbind the ftdi_sio driver before you can issue usb control commands from userspace? I haven't tested this yet but my assumption was that either a kernel driver or libusb can issue usb control messages, but both can not be bound to a device at the same time. I figured this wouldn't have come up when you tested your python script since the script likely predated adding the brick PID to the ftdi_sio Linux kernel driver. Maybe it makes sense to add a sysfs interface for reading/writing eeprom values without having to unbind the ftdi_sio driver. > > I can't think of a scenario where this would be difficult to fix in > userspace... > > -- > Hector Martin (hector@marcansoft.com) > Public Key: https://mrcn.st/pub
On 11/09/2020 03.51, James Hilliard wrote: > I haven't tested this yet but my assumption was that either a kernel driver > or libusb can issue usb control messages, but both can not be bound to > a device at the same time. I figured this wouldn't have come up when you > tested your python script since the script likely predated adding the brick PID > to the ftdi_sio Linux kernel driver. Binding to interfaces is exclusive, but global device control messages are not issued to an interface. I think it should work even if the kernel driver is bound (this is how lsusb works too, since it issues control requests even to devices bound to drivers). Even if it is necessary to unbind it, though, libusb already provides a single function to do that (libusb_detach_kernel_driver).
On Fri, Sep 11, 2020 at 04:54:08AM +0900, Hector Martin wrote: > On 11/09/2020 03.51, James Hilliard wrote: > > I haven't tested this yet but my assumption was that either a kernel driver > > or libusb can issue usb control messages, but both can not be bound to > > a device at the same time. I figured this wouldn't have come up when you > > tested your python script since the script likely predated adding the brick PID > > to the ftdi_sio Linux kernel driver. > > Binding to interfaces is exclusive, but global device control messages are > not issued to an interface. I think it should work even if the kernel driver > is bound (this is how lsusb works too, since it issues control requests even > to devices bound to drivers). Even if it is necessary to unbind it, though, > libusb already provides a single function to do that > (libusb_detach_kernel_driver). You really should unbind the device from the driver when doing stuff like this, so the kernel doesn't get confused. thanks, greg k-h
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index 871cdccf3a5f..85324e2ea107 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -1062,6 +1062,9 @@ static const char *ftdi_chip_name[] = { /* function prototypes for a FTDI serial converter */ static int ftdi_sio_probe(struct usb_serial *serial, const struct usb_device_id *id); +static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, + u16 nbytes); +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data); static int ftdi_sio_port_probe(struct usb_serial_port *port); static int ftdi_sio_port_remove(struct usb_serial_port *port); static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port); @@ -1996,39 +1999,6 @@ static int ftdi_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, return result; } -static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, - u16 nbytes) -{ - int read = 0; - - if (addr % 2 != 0) - return -EINVAL; - if (nbytes % 2 != 0) - return -EINVAL; - - /* Read EEPROM two bytes at a time */ - while (read < nbytes) { - int rv; - - rv = usb_control_msg(serial->dev, - usb_rcvctrlpipe(serial->dev, 0), - FTDI_SIO_READ_EEPROM_REQUEST, - FTDI_SIO_READ_EEPROM_REQUEST_TYPE, - 0, (addr + read) / 2, dst + read, 2, - WDR_TIMEOUT); - if (rv < 2) { - if (rv >= 0) - return -EIO; - else - return rv; - } - - read += rv; - } - - return 0; -} - static int ftdi_gpio_init_ft232h(struct usb_serial_port *port) { struct ftdi_private *priv = usb_get_serial_port_data(port); @@ -2234,10 +2204,149 @@ static int ftdi_sio_probe(struct usb_serial *serial, return 0; } +static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr, + u16 nbytes) +{ + int read = 0; + + if (addr % 2 != 0) + return -EINVAL; + if (nbytes % 2 != 0) + return -EINVAL; + + /* Read EEPROM two bytes at a time */ + while (read < nbytes) { + int rv; + + rv = usb_control_msg(serial->dev, + usb_rcvctrlpipe(serial->dev, 0), + FTDI_SIO_READ_EEPROM_REQUEST, + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, + 0, (addr + read) / 2, dst + read, 2, + WDR_TIMEOUT); + if (rv < 2) { + if (rv >= 0) + return -EIO; + else + return rv; + } + + read += rv; + } + + return 0; +} + +static int ftdi_write_eeprom(struct usb_serial_port *port, u8 addr, u16 data) +{ + struct usb_device *udev = port->serial->dev; + int rv; + + rv = usb_control_msg(udev, + usb_sndctrlpipe(udev, 0), + FTDI_SIO_WRITE_EEPROM_REQUEST, + FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE, + data, addr, + NULL, 0, WDR_TIMEOUT); + if (rv < 0) + dev_err(&port->dev, "Unable to write EEPROM: %i\n", rv); + return rv; +} + +static u16 ftdi_checksum(u16 *data, int n) +{ + u16 checksum; + int i; + + checksum = 0xaaaa; + for (i = 0; i < n - 1; i++) { + checksum ^= be16_to_cpu(data[i]); + checksum = (checksum << 1) | (checksum >> 15); + } + + return cpu_to_be16(checksum); +} + +static int ftdi_repair_brick(struct usb_serial_port *port) +{ + struct ftdi_private *priv = usb_get_serial_port_data(port); + int orig_latency; + int rv; + u16 *eeprom_data; + u16 checksum; + int eeprom_size; + int result; + + switch (priv->chip_type) { + case FT232RL: + eeprom_size = 0x40; + break; + default: + /* Unsupported for brick repair */ + return 0; + } + + /* Latency timer needs to be 0x77 to unlock EEPROM programming */ + if (priv->latency != 0x77) { + orig_latency = priv->latency; + priv->latency = 0x77; + rv = write_latency_timer(port); + priv->latency = orig_latency; + if (rv < 0) + return -EIO; + } + + eeprom_data = kmalloc(eeprom_size * 2, GFP_KERNEL); + if (!eeprom_data) + return -ENOMEM; + + /* Read in EEPROM */ + result = ftdi_read_eeprom(port->serial, eeprom_data, 0x00, eeprom_size * 2); + if (result < 0) + goto end_repair_brick; + + /* Verify EEPROM is valid */ + checksum = ftdi_checksum(eeprom_data, eeprom_size); + if (checksum != eeprom_data[eeprom_size - 1]) + goto end_repair_brick; + + /* Skip if no preimage attack against target address 62 */ + if (eeprom_data[62] == 0) + goto end_repair_brick; + + /* Attempt to restore Product ID to 0x6001 */ + eeprom_data[2] = FTDI_8U232AM_PID; + + /* Clear preimage attack target address */ + eeprom_data[62] = 0; + + /* Calculate and verify new checksum */ + checksum = ftdi_checksum(eeprom_data, eeprom_size); + if (checksum != eeprom_data[eeprom_size - 1]) + goto end_repair_brick; + + /* Restore EEPROM PID to original pre-brick state */ + if (ftdi_write_eeprom(port, 2, eeprom_data[2]) < 0) + goto end_repair_brick; + + /* Restore EEPROM preimage target address to original pre-brick state */ + if (ftdi_write_eeprom(port, 62, eeprom_data[62]) < 0) + goto end_repair_brick; + + dev_info(&port->dev, "Successfully repaired eeprom bricked by FTDI's malicious Windows driver.\n"); + +end_repair_brick: + kfree(eeprom_data); + + return result; +} + static int ftdi_sio_port_probe(struct usb_serial_port *port) { struct ftdi_private *priv; const struct ftdi_sio_quirk *quirk = usb_get_serial_data(port->serial); + u16 vendor_id; + u16 product_id; int result; priv = kzalloc(sizeof(struct ftdi_private), GFP_KERNEL); @@ -2255,6 +2364,12 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) ftdi_set_max_packet_size(port); if (read_latency_timer(port) < 0) priv->latency = 16; + vendor_id = le16_to_cpu(port->serial->dev->descriptor.idVendor); + product_id = le16_to_cpu(port->serial->dev->descriptor.idProduct); + if (vendor_id == FTDI_VID && + product_id == FTDI_BRICK_PID && + priv->chip_type == FT232RL) + ftdi_repair_brick(port); write_latency_timer(port); create_sysfs_attrs(port); diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h index be1641e0408b..40c6c4372a34 100644 --- a/drivers/usb/serial/ftdi_sio.h +++ b/drivers/usb/serial/ftdi_sio.h @@ -39,6 +39,7 @@ #define FTDI_SIO_SET_BITMODE 0x0b /* Set bitbang mode */ #define FTDI_SIO_READ_PINS 0x0c /* Read immediate value of pins */ #define FTDI_SIO_READ_EEPROM 0x90 /* Read EEPROM */ +#define FTDI_SIO_WRITE_EEPROM 0x91 /* Write EEPROM */ /* Interface indices for FT2232, FT2232H and FT4232H devices */ #define INTERFACE_A 1 @@ -457,6 +458,9 @@ enum ftdi_sio_baudrate { #define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0 #define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM +#define FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE 0x40 +#define FTDI_SIO_WRITE_EEPROM_REQUEST FTDI_SIO_WRITE_EEPROM + #define FTDI_FTX_CBUS_MUX_GPIO 0x8 #define FTDI_FT232R_CBUS_MUX_GPIO 0xa