BUG: iPNPstring in f_printer USB gadget is reduced by two bytes

BUG: iPNPstring in f_printer USB gadget is reduced by two bytes

[Volodymyr Lisivka]

Description:

Printer USB gadget uses iPNPstring to communicate device name and
command language with host. Linux driver for printer gadget sends
GET_DEVICE_ID response packet without 2 last bytes, which may cause
trouble for the host driver.

Steps to reproduce:

Use Raspberry Pi, or an other device with USB OTG plug. Raspberry Pi 4
was used by issue author.
Configure plug to be in peripheral mode, e.g. by adding
dtoverlay=dwc2,dr_mode=peripheral to /boot/config.txt.
Connect OTG port to host and reboot Raspberry Pi.
Load g_printer module using command sudo modprobe g_printer.
Use command ./get-iPNPstring.pl /dev/usb/lp1 to get iPNPstring from
the device. (See get-iPNPstring.pl.gz ). As alternative, kernel usbmon
or WireShark can be used to capture raw USB packet for GET_DEVICE_ID
response.

Expected result:

It's expected to receive same string as defined in module:
iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:1;'

Actual result:

iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:'

(Notice that last 2 chars are missing).

Workarround:

Just add two space to the end of printer gadget iPNPstring.

Root cause:

In drivers/usb/gadget/function/f_printer.c, length of iPNPstring is
used as length of the whole packet, without length of 2 byte size
field.

Patch:

--- f_printer.c.orig 2021-11-26 19:12:21.632221126 +0200
+++ f_printer.c 2021-11-26 19:09:19.454991670 +0200
@@ -1003,11 +1003,11 @@
  value = 0;
  break;
  }
- value = strlen(dev->pnp_string) ;
+ value = strlen(dev->pnp_string) + 2;
  buf[0] = (value >> 8) & 0xFF;
  buf[1] = value & 0xFF;
- memcpy(buf + 2, dev->pnp_string, value);
- DBG(dev, "1284 PNP String: %x %s\n", value,
+ memcpy(buf + 2, dev->pnp_string, value - 2);
+ DBG(dev, "1284 PNP String: %x %s\n", value - 2,
      dev->pnp_string);
  /* Length of packet is length of length field and length of iPNPstring. */
  break;

Re: BUG: iPNPstring in f_printer USB gadget is reduced by two bytes

[John Keeping]

On Tue, Nov 30, 2021 at 08:01:46PM +0200, Volodymyr Lisivka wrote:
> Description:
> 
> Printer USB gadget uses iPNPstring to communicate device name and
> command language with host. Linux driver for printer gadget sends
> GET_DEVICE_ID response packet without 2 last bytes, which may cause
> trouble for the host driver.
> 
> Steps to reproduce:
> 
> Use Raspberry Pi, or an other device with USB OTG plug. Raspberry Pi 4
> was used by issue author.
> Configure plug to be in peripheral mode, e.g. by adding
> dtoverlay=dwc2,dr_mode=peripheral to /boot/config.txt.
> Connect OTG port to host and reboot Raspberry Pi.
> Load g_printer module using command sudo modprobe g_printer.
> Use command ./get-iPNPstring.pl /dev/usb/lp1 to get iPNPstring from
> the device. (See get-iPNPstring.pl.gz ). As alternative, kernel usbmon
> or WireShark can be used to capture raw USB packet for GET_DEVICE_ID
> response.
> 
> Expected result:
> 
> It's expected to receive same string as defined in module:
> iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:1;'
> 
> Actual result:
> 
> iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:'
> 
> (Notice that last 2 chars are missing).
> 
> Workarround:
> 
> Just add two space to the end of printer gadget iPNPstring.
> 
> Root cause:
> 
> In drivers/usb/gadget/function/f_printer.c, length of iPNPstring is
> used as length of the whole packet, without length of 2 byte size
> field.

If I understand correctly, the length should be inclusive of the two
length bytes, but currently this driver encodes it exclusive.

The USB printer class specification says:

	... a device ID string that is compatible with IEEE 1284.  See
	IEEE 1284 for syntax and formatting information

and goes on to specify that this includes the length in the first two
bytes as big endian.

I don't have access to IEEE 1284, but looking in drivers/parport/probe.c
which implements the host side of IEEE 1284, we find
parport_read_device_id() with the comment:

	/* Some devices wrongly send LE length, and some send it two
	 * bytes short. Construct a sorted array of lengths to try. */

and code that assumes the values are inclusive of the length bytes.

So the patch below looks like it does the right thing to me, although it
appears whitespace damaged and it may be clearer to introduce a separate
variable for the string length compared to the value.

Are you interested in working up a proper patch, as described in
Documentation/process/submitting-patches.rst ?

> Patch:
> 
> --- f_printer.c.orig 2021-11-26 19:12:21.632221126 +0200
> +++ f_printer.c 2021-11-26 19:09:19.454991670 +0200
> @@ -1003,11 +1003,11 @@
>   value = 0;
>   break;
>   }
> - value = strlen(dev->pnp_string) ;
> + value = strlen(dev->pnp_string) + 2;
>   buf[0] = (value >> 8) & 0xFF;
>   buf[1] = value & 0xFF;
> - memcpy(buf + 2, dev->pnp_string, value);
> - DBG(dev, "1284 PNP String: %x %s\n", value,
> + memcpy(buf + 2, dev->pnp_string, value - 2);
> + DBG(dev, "1284 PNP String: %x %s\n", value - 2,
>       dev->pnp_string);
>   /* Length of packet is length of length field and length of iPNPstring. */
>   break;

Re: BUG: iPNPstring in f_printer USB gadget is reduced by two bytes

[John Keeping]

On Fri, Dec 03, 2021 at 04:46:17PM +0200, Volodymyr Lisivka wrote:
> > On Tue, Nov 30, 2021 at 08:01:46PM +0200, Volodymyr Lisivka wrote:
> > > Description:
> > >
> > > Printer USB gadget uses iPNPstring to communicate device name and
> > > command language with host. Linux driver for printer gadget sends
> > > GET_DEVICE_ID response packet without 2 last bytes, which may cause
> > > trouble for the host driver.
> > >
> > > Steps to reproduce:
> > >
> > > Use Raspberry Pi, or an other device with USB OTG plug. Raspberry Pi 4
> > > was used by issue author.
> > > Configure plug to be in peripheral mode, e.g. by adding
> > > dtoverlay=dwc2,dr_mode=peripheral to /boot/config.txt.
> > > Connect OTG port to host and reboot Raspberry Pi.
> > > Load g_printer module using command sudo modprobe g_printer.
> > > Use command ./get-iPNPstring.pl /dev/usb/lp1 to get iPNPstring from
> > > the device. (See get-iPNPstring.pl.gz ). As alternative, kernel usbmon
> > > or WireShark can be used to capture raw USB packet for GET_DEVICE_ID
> > > response.
> > >
> > > Expected result:
> > >
> > > It's expected to receive same string as defined in module:
> > > iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:1;'
> > >
> > > Actual result:
> > >
> > > iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:'
> > >
> > > (Notice that last 2 chars are missing).
> > >
> > > Workarround:
> > >
> > > Just add two space to the end of printer gadget iPNPstring.
> > >
> > > Root cause:
> > >
> > > In drivers/usb/gadget/function/f_printer.c, length of iPNPstring is
> > > used as length of the whole packet, without length of 2 byte size
> > > field.
> >
> > If I understand correctly, the length should be inclusive of the two
> > length bytes, but currently this driver encodes it exclusive.
> >
> > The USB printer class specification says:
> >
> > ... a device ID string that is compatible with IEEE 1284.  See
> > IEEE 1284 for syntax and formatting information
> >
> > and goes on to specify that this includes the length in the first two
> > bytes as big endian.
> >
> > I don't have access to IEEE 1284, but looking in drivers/parport/probe.c
> > which implements the host side of IEEE 1284, we find
> > parport_read_device_id() with the comment:
> >
> > /* Some devices wrongly send LE length, and some send it two
> > * bytes short. Construct a sorted array of lengths to try. */
> >
> > and code that assumes the values are inclusive of the length bytes.
> >
> > So the patch below looks like it does the right thing to me, although it
> > appears whitespace damaged and it may be clearer to introduce a separate
> > variable for the string length compared to the value.
> 
> diff --git a/drivers/usb/gadget/function/f_printer.c
> b/drivers/usb/gadget/function/f_printer.c
> index 236ecc968..403faa05b 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -987,6 +987,7 @@ static int printer_func_setup(struct usb_function *f,
>         u16                     wIndex = le16_to_cpu(ctrl->wIndex);
>         u16                     wValue = le16_to_cpu(ctrl->wValue);
>         u16                     wLength = le16_to_cpu(ctrl->wLength);
> +       u16                     pnp_length;
> 
>         DBG(dev, "ctrl req%02x.%02x v%04x i%04x l%d\n",
>                 ctrl->bRequestType, ctrl->bRequest, wValue, wIndex, wLength);
> @@ -1003,11 +1004,12 @@ static int printer_func_setup(struct usb_function *f,
>                                 value = 0;
>                                 break;
>                         }
> -                       value = strlen(dev->pnp_string);
> +                       pnp_length = strlen(dev->pnp_string);
> +                       value = pnp_length + 2;
>                         buf[0] = (value >> 8) & 0xFF;
>                         buf[1] = value & 0xFF;
> -                       memcpy(buf + 2, dev->pnp_string, value);
> -                       DBG(dev, "1284 PNP String: %x %s\n", value,
> +                       memcpy(buf + 2, dev->pnp_string, pnp_length);
> +                       DBG(dev, "1284 PNP String: %x %s\n", pnp_length,
>                             dev->pnp_string);
>                         break;
> 
> 
> 
> >
> > Are you interested in working up a proper patch, as described in
> > Documentation/process/submitting-patches.rst ?
> >
> 
> No. It's my second patch in 15 years. If you have a proper procedure
> for diffing/patching, then make corresponding targets in the top
> Makefile, e.g. `make diff` or `make patch`.

Do you mean `git format-patch`? ;-)

Note that the diff is only part of a patch, the commit message and
Signed-off-by line ceritifying the developer certificate of origin [1]
are also important.

[1] https://developercertificate.org/

Re: BUG: iPNPstring in f_printer USB gadget is reduced by two bytes

[Volodymyr Lisivka]

> On Tue, Nov 30, 2021 at 08:01:46PM +0200, Volodymyr Lisivka wrote:
> > Description:
> >
> > Printer USB gadget uses iPNPstring to communicate device name and
> > command language with host. Linux driver for printer gadget sends
> > GET_DEVICE_ID response packet without 2 last bytes, which may cause
> > trouble for the host driver.
> >
> > Steps to reproduce:
> >
> > Use Raspberry Pi, or an other device with USB OTG plug. Raspberry Pi 4
> > was used by issue author.
> > Configure plug to be in peripheral mode, e.g. by adding
> > dtoverlay=dwc2,dr_mode=peripheral to /boot/config.txt.
> > Connect OTG port to host and reboot Raspberry Pi.
> > Load g_printer module using command sudo modprobe g_printer.
> > Use command ./get-iPNPstring.pl /dev/usb/lp1 to get iPNPstring from
> > the device. (See get-iPNPstring.pl.gz ). As alternative, kernel usbmon
> > or WireShark can be used to capture raw USB packet for GET_DEVICE_ID
> > response.
> >
> > Expected result:
> >
> > It's expected to receive same string as defined in module:
> > iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:1;'
> >
> > Actual result:
> >
> > iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:'
> >
> > (Notice that last 2 chars are missing).
> >
> > Workarround:
> >
> > Just add two space to the end of printer gadget iPNPstring.
> >
> > Root cause:
> >
> > In drivers/usb/gadget/function/f_printer.c, length of iPNPstring is
> > used as length of the whole packet, without length of 2 byte size
> > field.
>
> If I understand correctly, the length should be inclusive of the two
> length bytes, but currently this driver encodes it exclusive.
>
> The USB printer class specification says:
>
> ... a device ID string that is compatible with IEEE 1284.  See
> IEEE 1284 for syntax and formatting information
>
> and goes on to specify that this includes the length in the first two
> bytes as big endian.
>
> I don't have access to IEEE 1284, but looking in drivers/parport/probe.c
> which implements the host side of IEEE 1284, we find
> parport_read_device_id() with the comment:
>
> /* Some devices wrongly send LE length, and some send it two
> * bytes short. Construct a sorted array of lengths to try. */
>
> and code that assumes the values are inclusive of the length bytes.
>
> So the patch below looks like it does the right thing to me, although it
> appears whitespace damaged and it may be clearer to introduce a separate
> variable for the string length compared to the value.

diff --git a/drivers/usb/gadget/function/f_printer.c
b/drivers/usb/gadget/function/f_printer.c
index 236ecc968..403faa05b 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -987,6 +987,7 @@ static int printer_func_setup(struct usb_function *f,
        u16                     wIndex = le16_to_cpu(ctrl->wIndex);
        u16                     wValue = le16_to_cpu(ctrl->wValue);
        u16                     wLength = le16_to_cpu(ctrl->wLength);
+       u16                     pnp_length;

        DBG(dev, "ctrl req%02x.%02x v%04x i%04x l%d\n",
                ctrl->bRequestType, ctrl->bRequest, wValue, wIndex, wLength);
@@ -1003,11 +1004,12 @@ static int printer_func_setup(struct usb_function *f,
                                value = 0;
                                break;
                        }
-                       value = strlen(dev->pnp_string);
+                       pnp_length = strlen(dev->pnp_string);
+                       value = pnp_length + 2;
                        buf[0] = (value >> 8) & 0xFF;
                        buf[1] = value & 0xFF;
-                       memcpy(buf + 2, dev->pnp_string, value);
-                       DBG(dev, "1284 PNP String: %x %s\n", value,
+                       memcpy(buf + 2, dev->pnp_string, pnp_length);
+                       DBG(dev, "1284 PNP String: %x %s\n", pnp_length,
                            dev->pnp_string);
                        break;



>
> Are you interested in working up a proper patch, as described in
> Documentation/process/submitting-patches.rst ?
>

No. It's my second patch in 15 years. If you have a proper procedure
for diffing/patching, then make corresponding targets in the top
Makefile, e.g. `make diff` or `make patch`.