Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200909193419.2006744-1-james.hilliard1@gmail.com>
 <1599706954.10822.3.camel@suse.de> <CADvTj4oznyY=sfbmtmdcYMgkYriV_Ujk=+0Fz--XumNa8ZvD=A@mail.gmail.com>
 <36F0CEB4-DE2E-41DA-9569-4706B00FCF7D@marcansoft.com>
In-Reply-To: <36F0CEB4-DE2E-41DA-9569-4706B00FCF7D@marcansoft.com>
From:   James Hilliard <james.hilliard1@gmail.com>
Date:   Wed, 9 Sep 2020 22:07:36 -0600
Message-ID: <CADvTj4omztaEz09qXYCvspNowqavOSOM1qaXvtAt1KQZuWDisw@mail.gmail.com>
Subject: Re: [PATCH v2] usb: serial: Repair FTDI FT232R bricked eeprom
To:     Hector Martin marcan <hector@marcansoft.com>
Cc:     Oliver Neukum <oneukum@suse.de>, linux-usb@vger.kernel.org,
        Johan Hovold <johan@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Russ Dill <Russ.Dill@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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