[PATCH 0/1] usb: gadget: f_hid: fix report descriptor allocation

[PATCH 0/1] usb: gadget: f_hid: fix report descriptor allocation

[Konstantin Aladyshev]

The commit "usb: gadget: f_hid: fix f_hidg lifetime vs cdev"
(89ff3dfac604614287ad5aad9370c3f984ea3f4b) has introduced a bug
that leads to hid device corruption after the replug operation.

The usb gadget driver bug was observed in the KVM functionality of the
OpenBMC distribution. In the test environment BMC provides KVM
functionality for the host (i.e. virtual USB keyboard) via the USB
gadget device.

The bug occurs when the KVM page is refreshed (i.e. USB device is
replugged).

Before the 89ff3dfac6 this opeartion was working without any issues.
Log messages from the host for this operation:
```
kernel: usb 1-7.4: USB disconnect, device number 3
kernel: usb 1-7.4: new high-speedUSB device number 4 using xhci_hcd
kernel: usb 1-7.4: New USB device found, idVendor=1d6b, idProduct=0104,
  bcdDevice= 1.00
kernel: usb 1-7.4: New USB device strings: Mfr=1, Produt=2, SerialNumber=3
kernel: usb 1-7.4: Product: Virtual Keyboard and Mouse
kernel: usb 1-7.4: Manufacturer: OpenBMC
kernel: usb 1-7.4: SerialNumber: OBMC0001
kernel: input: OpenBMC Virtual Keyboard and Mouse as /devices/pci0000:
  00/0000:00:140/usb1/1-7/1-7.4/1-7.4:1.0/0003:1D6B:0104.0003/input/input3
kernel: hid-generic 0003:1D6B:0104.0003: inputhidraw0: USB HID v1.01
  Keyboard [OpenBMC Virtual Keyboard and Mouse] on usb-0000:00:14.0-7.4/input0
kernel: input: OpenBMC Virtual Keyboard and Mouse as /devices/pci0000:
  00/0000:00:14.0/sb1/1-7/1-7.4/1-7.4:1.1/0003:1D6B:0104.0004/input/input4
kernel: hid-generic 0003:1D6B:0104.0004: input,hidraw1: USB HID v1.01
  Mouse [OpenBMC Virtual Keyboard and Mouse] on usb-0000:00:14.0-7.4/input1
```

After the 89ff3dfac6 the KVM page refresh (i.e. USB device replug) results
to the USB device corruction and the following messages from the driver:
```
kernel: usb 1-7.4: USB disconnect, device number 3
hid-generic 0003:1D6B:0104.0003: item fetching failed at offset 18/63
hid-generic 0003:1D6B:01040004: item fetching failed at offset 32/76
kernel: usb 1-7.4: new high-speed USB device number 4 using xhci_hcd
kernel: usb 1-7.4: New USB device found, idVendor=1d6b, idProduct=0104,
  bcdDevice= 1.00
kernel: usb 1-7.4:New USB device strings: Mfr=1, Product=2, SerialNumber=3
kernel: usb 1-7.4: Product: Virual Keyboard and Mouse
kernel: usb 1-7.4: Manufacturer: OpenBMC
kernel: usb 1-7.4: SerialNumber: OBMC0001
kernel: id-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item tag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: unknown main item ag 0x0
kernel: hid-generic 0003:1D6B:0104.0003: item fetching failed at offset 18/63
kernel: hid-eneric: probe of 0003:1D6B:0104.0003 failed with error -22
kernel: hid-generic 0003:1D6B:0104.0004: item fetching failed at offset 32/76
kernel hid-generic: probe of 0003:1D6B:0104.0004 failed with error -22
```

Reverse device managed memory allocation for the report descriptor
to fix the issue.



Konstantin Aladyshev (1):
  usb: gadget: f_hid: fix report descriptor allocation

 drivers/usb/gadget/function/f_hid.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/1] usb: gadget: f_hid: fix report descriptor allocation

[Konstantin Aladyshev]

The commit "usb: gadget: f_hid: fix f_hidg lifetime vs cdev"
(89ff3dfac604614287ad5aad9370c3f984ea3f4b) has introduced a bug
that leads to hid device corruption after the replug operation.
Reverse device managed memory allocation for the report descriptor
to fix the issue.

Tested:
This change was tested on the AMD EthanolX CRB server with the BMC
based on the OpenBMC distribution. The BMC provides KVM functionality
via the USB gadget device:
- before: KVM page refresh results in a broken USB device,
- after: KVM page refresh works without any issues.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 drivers/usb/gadget/function/f_hid.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ea85e2c701a1..3c8a9dd585c0 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -92,6 +92,7 @@ static void hidg_release(struct device *dev)
 {
 	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
 
+	kfree(hidg->report_desc);
 	kfree(hidg->set_report_buf);
 	kfree(hidg);
 }
@@ -1287,9 +1288,9 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	hidg->report_length = opts->report_length;
 	hidg->report_desc_length = opts->report_desc_length;
 	if (opts->report_desc) {
-		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
-						 opts->report_desc_length,
-						 GFP_KERNEL);
+		hidg->report_desc = kmemdup(opts->report_desc,
+					    opts->report_desc_length,
+					    GFP_KERNEL);
 		if (!hidg->report_desc) {
 			ret = -ENOMEM;
 			goto err_put_device;
-- 
2.25.1

Re: [PATCH 1/1] usb: gadget: f_hid: fix report descriptor allocation

[Greg KH]

On Tue, Dec 05, 2023 at 11:54:03AM +0300, Konstantin Aladyshev wrote:
> The commit "usb: gadget: f_hid: fix f_hidg lifetime vs cdev"
> (89ff3dfac604614287ad5aad9370c3f984ea3f4b) has introduced a bug
> that leads to hid device corruption after the replug operation.

Nit, this should be written as
	89ff3dfac604 ("usb: gadget: f_hid: fix f_hidg lifetime vs cdev")
right?

> Reverse device managed memory allocation for the report descriptor
> to fix the issue.
> 
> Tested:
> This change was tested on the AMD EthanolX CRB server with the BMC
> based on the OpenBMC distribution. The BMC provides KVM functionality
> via the USB gadget device:
> - before: KVM page refresh results in a broken USB device,
> - after: KVM page refresh works without any issues.
> 
> Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>

We need a Fixes: tag here and also a cc: stable so that this gets
properly backported.

> ---
>  drivers/usb/gadget/function/f_hid.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index ea85e2c701a1..3c8a9dd585c0 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -92,6 +92,7 @@ static void hidg_release(struct device *dev)
>  {
>  	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
>  
> +	kfree(hidg->report_desc);
>  	kfree(hidg->set_report_buf);
>  	kfree(hidg);
>  }
> @@ -1287,9 +1288,9 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  	hidg->report_length = opts->report_length;
>  	hidg->report_desc_length = opts->report_desc_length;
>  	if (opts->report_desc) {
> -		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
> -						 opts->report_desc_length,
> -						 GFP_KERNEL);
> +		hidg->report_desc = kmemdup(opts->report_desc,
> +					    opts->report_desc_length,
> +					    GFP_KERNEL);

Yet one more reason why devm_*() functions are dangerous to use :(

Nice fix.

thanks,

greg k-h