Re: Possible bug in the cp210x kernel driver?

Re: Possible bug in the cp210x kernel driver?

[Johan Hovold]

On Tue, Sep 21, 2021 at 06:55:21PM +0200, Malte Di Donato wrote:
> Hi Johan,
> 
> Sorry about the html crap.
> (and double excuse that I've totally forgotten to change my webmail
> account surname since wedding ;)

Heh, good that you noticed in time so I can give you proper credit in
the commit message. ;)

Unfortunately your mails won't be seen by anyone else one the list
unless you can disable that html.

> Placed the patch code into my running debian kernel version
> 5.10.0-8-amd64 and put the two invocations to the
> cp210x_init_max_speed function (cp210x_get_fw_version not existent in
> my kernel source file).

Sounds good, we need to do the same when backporting the fix.

> Here's the dmesg with the patched driver:
> 
> [19089.502913] usbcore: registered new interface driver cp210x
> [19089.502951] usbserial: USB Serial support registered for cp210x
> [19096.084910] usb 3-1.2: new full-speed USB device number 9 using ehci-pci
> [19096.224973] usb 3-1.2: New USB device found, idVendor=10c4, idProduct=ea60, bcdDevice= 1.00
> [19096.225034] usb 3-1.2: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> [19096.225037] usb 3-1.2: Product: CP2102 USB to UART Bridge Controller
> [19096.225040] usb 3-1.2: Manufacturer: Silicon Labs
> [19096.225043] usb 3-1.2: SerialNumber: 0001
> [19096.233460] cp210x 3-1.2:1.0: cp210x converter detected
> [19096.233939] cp210x 3-1.2:1.0: cp210x_init_max_speed - type = 0x02
> [19096.234458] cp210x 3-1.2:1.0: failed to get vendor val 0x370b size 8: 1

So the device returns one byte (instead of eight) as I suspected. Then
we can use this to detect these devices.

> [19096.236568] usb 3-1.2: cp210x converter now attached to ttyUSB0
> 
> I also compiled the cp2102-det and run it against the chip:
> 09: Got 128 bytes:  FF F0 FF FA 01 00 60 E3 16 00 FF F0 FF FA 01 00 60 E3 16 00 FF EC FF F8 01 00 80 4F 12 00 FF E6 FF F6 01 00 00 10 0E 00 FF D6 FF F0 01 00 00 CA 08 00 FF D0 FF EE 01 00 20 A1 07 00 FF CC FF EC 01 00 00 08 07 00 FF A2 FF DC 01 00 00 E8 03 00 FF A0 FF DC 01 00 90 D0 03 00 FF 98 FF D9 01 00 00 84 03 00 FF 64 FF C5 01 00 00 58 02 00 FF 44 FF B9 01 00 00 F4 01 00 FF 30 FF B2 01 00 00 C2 
> 0A: Got 1 bytes:  FF 
> 0B: Got 1 bytes:  02 
> Done.
> 
> Hope this helps?

It does, thanks. The 0B request above is the part-number request. The
original CP2102 I have here returns 8 bytes if that's requested.

> As I look on the "SerialNumber: 0001" in dmesg it may be possible that
> I've got a cloned chip on the chinesium usb/ttl converter...

Mine has 0001 as serial number as well, but the lack of event-mode and
different vendor request behaviour seems to suggest it could be a
counterfeit.

I'll reply to this mail with a fix that fixes the dropped character
issue by disabling event-insertion mode (and thus parity reporting) for
devices such as yours.

I've verified it here as well as I could, but could you give it a spin
with your device as well? If you want you can reply to the patch mail
with a Tested-by tag and I'll add that to the commit message as well.

Johan

[PATCH 1/2] USB: serial: cp210x: fix dropped characters with CP2102

[Johan Hovold]

Some CP2102 do not support event-insertion mode but return no error when
attempting to enable it.

This means that any event escape characters in the input stream will not
be escaped by the device and consequently regular data may be
interpreted as escape sequences and be removed from the stream by the
driver.

The reporter's device has batch number DCL00X etched into it and as
discovered by the SHA2017 Badge team, counterfeit devices with that
marking can be detected by sending malformed vendor requests. [1][2]

Tests confirm that the possibly counterfeit CP2102 returns a single byte
in response to a malformed two-byte part-number request, while an
original CP2102 returns two bytes. Assume that every CP2102 that behaves
this way also does not support event-insertion mode (e.g. cannot report
parity errors).

[1] https://mobile.twitter.com/sha2017badge/status/1167902087289532418
[2] https://hackaday.com/2017/08/14/hands-on-with-the-shacamp-2017-badge/#comment-3903376

Reported-by: Malte Di Donato <malte@neo-soft.org>
Fixes: a7207e9835a4 ("USB: serial: cp210x: add support for line-status events")
Cc: stable@vger.kernel.org	# 5.9
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 66a6ac50a4cd..b98454fe08ea 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -258,6 +258,7 @@ struct cp210x_serial_private {
 	speed_t			max_speed;
 	bool			use_actual_rate;
 	bool			no_flow_control;
+	bool			no_event_mode;
 };
 
 enum cp210x_event_state {
@@ -1113,12 +1114,16 @@ static void cp210x_change_speed(struct tty_struct *tty,
 
 static void cp210x_enable_event_mode(struct usb_serial_port *port)
 {
+	struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
 	int ret;
 
 	if (port_priv->event_mode)
 		return;
 
+	if (priv->no_event_mode)
+		return;
+
 	port_priv->event_state = ES_DATA;
 	port_priv->event_mode = true;
 
@@ -2074,6 +2079,33 @@ static void cp210x_init_max_speed(struct usb_serial *serial)
 	priv->use_actual_rate = use_actual_rate;
 }
 
+static void cp2102_determine_quirks(struct usb_serial *serial)
+{
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+	u8 *buf;
+	int ret;
+
+	buf = kmalloc(2, GFP_KERNEL);
+	if (!buf)
+		return;
+	/*
+	 * Some (possibly counterfeit) CP2102 do not support event-insertion
+	 * mode and respond differently to malformed vendor requests.
+	 * Specifically, they return one instead of two bytes when sent a
+	 * two-byte part-number request.
+	 */
+	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			CP210X_VENDOR_SPECIFIC, REQTYPE_DEVICE_TO_HOST,
+			CP210X_GET_PARTNUM, 0, buf, 2, USB_CTRL_GET_TIMEOUT);
+	if (ret == 1) {
+		dev_dbg(&serial->interface->dev,
+				"device does not support event-insertion mode\n");
+		priv->no_event_mode = true;
+	}
+
+	kfree(buf);
+}
+
 static int cp210x_get_fw_version(struct usb_serial *serial, u16 value)
 {
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
@@ -2109,6 +2141,9 @@ static void cp210x_determine_type(struct usb_serial *serial)
 	}
 
 	switch (priv->partnum) {
+	case CP210X_PARTNUM_CP2102:
+		cp2102_determine_quirks(serial);
+		break;
 	case CP210X_PARTNUM_CP2105:
 	case CP210X_PARTNUM_CP2108:
 		cp210x_get_fw_version(serial, CP210X_GET_FW_VER);
-- 
2.32.0

[PATCH 2/2] USB: serial: cp210x: add part-number debug printk

[Johan Hovold]

Add a part-number debug printk to facilitate debugging.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/cp210x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index b98454fe08ea..fd51498ab108 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -2140,6 +2140,8 @@ static void cp210x_determine_type(struct usb_serial *serial)
 		return;
 	}
 
+	dev_dbg(&serial->interface->dev, "partnum = 0x%02x\n", priv->partnum);
+
 	switch (priv->partnum) {
 	case CP210X_PARTNUM_CP2102:
 		cp2102_determine_quirks(serial);
-- 
2.32.0

Re: [PATCH 1/2] USB: serial: cp210x: fix dropped characters with CP2102

[Malte Di Donato]

Tested-by: Malte Di Donato <malte@neo-soft.org>

# dmesg (cp2102 connect)

[57018.586990] usb 3-1.2: new full-speed USB device number 17 using ehci-pci
[57018.727102] usb 3-1.2: New USB device found, idVendor=10c4, 
idProduct=ea60, bcdDevice= 1.00
[57018.727109] usb 3-1.2: New USB device strings: Mfr=1, Product=2, 
SerialNumber=3
[57018.727112] usb 3-1.2: Product: CP2102 USB to UART Bridge Controller
[57018.727115] usb 3-1.2: Manufacturer: Silicon Labs
[57018.727117] usb 3-1.2: SerialNumber: 0001
[57018.735520] cp210x 3-1.2:1.0: cp210x converter detected
[57018.736071] cp210x 3-1.2:1.0: partnum = 0x02
[57018.736529] cp210x 3-1.2:1.0: device does not support event-insertion 
mode
[57018.738780] usb 3-1.2: cp210x converter now attached to ttyUSB0

# sucessfully did a complete mem dump of a device which sends several 
valid 0xEC 0x00
# libdivecomputer/examples/dctool -d "Mares Puck Pro" dump -o 
/tmp/test.bin /dev/ttyUSB0

Opening the I/O stream (serial, /dev/ttyUSB0).
Opening the device (Mares Puck Pro).
Registering the event handler.
Registering the cancellation handler.
Downloading the memory dump.
Event: 
vendor=000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005075636B2050726F000000000000000030342E30302E30300400000031342D30342D31352B8B4443303234000000000000000000000000000000000000000000000000000000
Event: progress 0.00% (0/262144)
Event: progress 0.10% (256/262144)
...
Event: progress 100.00% (262144/262144)



On 22.09.21 13:30, Johan Hovold wrote:
> Some CP2102 do not support event-insertion mode but return no error when
> attempting to enable it.
> 
> This means that any event escape characters in the input stream will not
> be escaped by the device and consequently regular data may be
> interpreted as escape sequences and be removed from the stream by the
> driver.
> 
> The reporter's device has batch number DCL00X etched into it and as
> discovered by the SHA2017 Badge team, counterfeit devices with that
> marking can be detected by sending malformed vendor requests. [1][2]
> 
> Tests confirm that the possibly counterfeit CP2102 returns a single byte
> in response to a malformed two-byte part-number request, while an
> original CP2102 returns two bytes. Assume that every CP2102 that behaves
> this way also does not support event-insertion mode (e.g. cannot report
> parity errors).
> 
> [1] https://mobile.twitter.com/sha2017badge/status/1167902087289532418
> [2] https://hackaday.com/2017/08/14/hands-on-with-the-shacamp-2017-badge/#comment-3903376
> 
> Reported-by: Malte Di Donato <malte@neo-soft.org>
> Fixes: a7207e9835a4 ("USB: serial: cp210x: add support for line-status events")
> Cc: stable@vger.kernel.org	# 5.9
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/usb/serial/cp210x.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 66a6ac50a4cd..b98454fe08ea 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -258,6 +258,7 @@ struct cp210x_serial_private {
>   	speed_t			max_speed;
>   	bool			use_actual_rate;
>   	bool			no_flow_control;
> +	bool			no_event_mode;
>   };
>   
>   enum cp210x_event_state {
> @@ -1113,12 +1114,16 @@ static void cp210x_change_speed(struct tty_struct *tty,
>   
>   static void cp210x_enable_event_mode(struct usb_serial_port *port)
>   {
> +	struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
>   	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
>   	int ret;
>   
>   	if (port_priv->event_mode)
>   		return;
>   
> +	if (priv->no_event_mode)
> +		return;
> +
>   	port_priv->event_state = ES_DATA;
>   	port_priv->event_mode = true;
>   
> @@ -2074,6 +2079,33 @@ static void cp210x_init_max_speed(struct usb_serial *serial)
>   	priv->use_actual_rate = use_actual_rate;
>   }
>   
> +static void cp2102_determine_quirks(struct usb_serial *serial)
> +{
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmalloc(2, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +	/*
> +	 * Some (possibly counterfeit) CP2102 do not support event-insertion
> +	 * mode and respond differently to malformed vendor requests.
> +	 * Specifically, they return one instead of two bytes when sent a
> +	 * two-byte part-number request.
> +	 */
> +	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +			CP210X_VENDOR_SPECIFIC, REQTYPE_DEVICE_TO_HOST,
> +			CP210X_GET_PARTNUM, 0, buf, 2, USB_CTRL_GET_TIMEOUT);
> +	if (ret == 1) {
> +		dev_dbg(&serial->interface->dev,
> +				"device does not support event-insertion mode\n");
> +		priv->no_event_mode = true;
> +	}
> +
> +	kfree(buf);
> +}
> +
>   static int cp210x_get_fw_version(struct usb_serial *serial, u16 value)
>   {
>   	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> @@ -2109,6 +2141,9 @@ static void cp210x_determine_type(struct usb_serial *serial)
>   	}
>   
>   	switch (priv->partnum) {
> +	case CP210X_PARTNUM_CP2102:
> +		cp2102_determine_quirks(serial);
> +		break;
>   	case CP210X_PARTNUM_CP2105:
>   	case CP210X_PARTNUM_CP2108:
>   		cp210x_get_fw_version(serial, CP210X_GET_FW_VER);
> 

Re: [PATCH 1/2] USB: serial: cp210x: fix dropped characters with CP2102

[Johan Hovold]

On Wed, Sep 22, 2021 at 10:12:48PM +0200, Malte Di Donato wrote:
> Tested-by: Malte Di Donato <malte@neo-soft.org>

Thanks for testing. Now applied.

Johan

Re: Possible bug in the cp210x kernel driver?

[Malte Di Donato]

On 22.09.21 13:28, Johan Hovold wrote:
> Heh, good that you noticed in time so I can give you proper credit in
> the commit message. ;)
yay \/ ;)

> Unfortunately your mails won't be seen by anyone else one the list
> unless you can disable that html.
finally it seems that I've got fixed that by now.
Not so easy with "limited everything" on vacation :/

> I'll reply to this mail with a fix that fixes the dropped character
> issue by disabling event-insertion mode (and thus parity reporting) for
> devices such as yours.
> 
> I've verified it here as well as I could, but could you give it a spin
> with your device as well? If you want you can reply to the patch mail
> with a Tested-by tag and I'll add that to the commit message as well.
> 
> Johan

Successfully tested the patch in debian kernel 5.10.0-8-amd64!


Malte

Re: Possible bug in the cp210x kernel driver?

[Johan Hovold]

On Wed, Sep 22, 2021 at 10:25:24PM +0200, Malte Di Donato wrote:
> On 22.09.21 13:28, Johan Hovold wrote:

> > Unfortunately your mails won't be seen by anyone else one the list
> > unless you can disable that html.
> finally it seems that I've got fixed that by now.
> Not so easy with "limited everything" on vacation :/

I understand. Good you got it sorted before your next diving trip. :)

> > I'll reply to this mail with a fix that fixes the dropped character
> > issue by disabling event-insertion mode (and thus parity reporting) for
> > devices such as yours.
> > 
> > I've verified it here as well as I could, but could you give it a spin
> > with your device as well? If you want you can reply to the patch mail
> > with a Tested-by tag and I'll add that to the commit message as well.

> Successfully tested the patch in debian kernel 5.10.0-8-amd64!

Thanks, and enjoy your vacation!

Johan