Re: [PATCH] usb: serial: Perform verification for FTDI FT232R devices

[Perry Hung <iperry@gmail.com>]

Looks good Russ, just a minor fix:

-       /* Attempt to set Vendor ID to 0 */
-       eeprom_data[1] = 0;
-
-       /* Calculate new checksum to avoid bricking devices */
-       checksum = ftdi_checksum(eeprom_data, eeprom_size);
-
-       /* Verify EEPROM programming behavior/nonbehavior */
-       write_eeprom(port, 1, 0);
-       write_eeprom(port, eeprom_size - 1, checksum);
+    /* Attempt to set Product ID */
+    switch (priv->chip_type) {
+    case FT232BM:
+    case FT232RL:
+    case FT232H:
+        if (eeprom_data[2] == 0) {
+            eeprom_data[2] = FTDI_232RL_PID;
+            /* Calculate new checksum to avoid bricking devices */
+            checksum = ftdi_checksum(eeprom_data, eeprom_size);
+
+            /* Verify EEPROM programming behavior/nonbehavior */
+            write_eeprom(port, 2, eeprom_data[2]);
+            write_eeprom(port, 3, eeprom_data[3]);
+            write_eeprom(port, eeprom_size - 2, eeprom_data[eeprom_size - 2]);
+            write_eeprom(port, eeprom_size - 1, checksum);
+        }
+        break;
+    default:
+        break;
+    }

 end_verify:

On Thu, Oct 23, 2014 at 10:14 PM, Russ Dill <russ.dill@gmail.com> wrote:
> On Thu, Oct 23, 2014 at 5:44 AM, Hector Martin <hector@marcansoft.com> wrote:
>> NAK. This patch neither accomplishes what FTDI intended, nor what the
>> author humorously intended.
>>
>>> +     /* Attempt to set Vendor ID to 0 */
>>> +     eeprom_data[1] = 0;
>>> +
>>> +     /* Calculate new checksum to avoid bricking devices */
>>> +     checksum = ftdi_checksum(eeprom_data, eeprom_size);
>>> +
>>> +     /* Verify EEPROM programming behavior/nonbehavior */
>>> +     write_eeprom(port, 1, 0);
>>> +     write_eeprom(port, eeprom_size - 1, checksum);
>>
>> FTDI's verification routine sets the Product ID (at address 2) to 0 and
>> a dummy word (at address 0x3e) to a correctly crafted value that makes
>> the existing checksum pass. This bricks clone devices (setting PID to
>> 0), while original FT232RL devices are not affected as they only commit
>> writes when they receive a write command to an odd EEPROM address,
>> combining it with the most recently issued write to an even address and
>> writing 32 bits at a time.
>>
>> This patch instead writes the Vendor ID (at address 1) and the real
>> checksum (at address 0x3f). As amusing as bricking all devices would be,
>> unfortunately, a real FT232RL would just write garbage at addresses 0
>> and 0x3e too (as writes are still 32 bits, and no prior even-addressed
>> writes have occurred, so the holding register on the chip contains
>> garbage). Therefore, the real effect of this patch is to brick clone
>> devices (in a different way from the official driver, killing the VID
>> instead of the PID), while merely resetting original FT232RL devices to
>> defaults, due to the inadvertently corrupted even words now causing a
>> checksum mismatch.
>>
>> Props on the humor, try again with better code next time ;-). I suggest
>> the following:
>>
>> +       write_eeprom(port, 0, eeprom_data[0]);
>> +       write_eeprom(port, 1, 0);
>> +       write_eeprom(port, eeprom_size - 2, eeprom_data[eeprom_size - 2]);
>> +       write_eeprom(port, eeprom_size - 1, checksum);
>
> Damned off by one errors. Yes, it should be the product ID, not the
> vendor ID. These write u16's though, writing to wIndex 2 writes to
> bytes 4 and 5. the correct code is:
>
> write_eeprom(port, 2, 0);
> write_eeprom(port, eeprom_size - 2, checksum);
>
> And yes, the checksum code needs to be modified to create a specially
> crafted value that allows the existing checksum to pass.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/