Re: [PATCH v3] HID: add driver for U2F Zero built-in LED and RNG

[Andrej Shadura <andrew.shadura@collabora.co.uk>]

On 01/04/2019 14:42, Andrej Shadura wrote:
> U2F Zero supports custom commands for blinking the LED and getting data
> from the internal hardware RNG. Expose the blinking function as a LED
> device, and the internal hardware RNG as an HWRNG so that it can be used
> to feed the enthropy pool.
> 
> Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>

I’ve been testing this with a different modification of U2F Zero,
Nitrokey FIDO U2F, and on that device only I’m getting a kernel warning
(see below).

> +static int u2fzero_recv(struct u2fzero_device *dev,
> +			struct u2f_hid_report *req,
> +			struct u2f_hid_msg *resp)
> +{
> +	int ret;
> +	struct hid_device *hdev = dev->hdev;
> +	struct u2fzero_transfer_context ctx;
> +
> +	mutex_lock(&dev->lock);
> +
> +	memcpy(dev->buf_out, req, sizeof(struct u2f_hid_report));
> +
> +	dev->urb->context = &ctx;
> +	init_completion(&ctx.done);
> +
> +	ret = usb_submit_urb(dev->urb, GFP_NOIO);

Here, usb_submit_urb() returns EBUSY in the first attempt to read random
numbers from the device:

URB ffff917256d5d540 submitted while active
WARNING: CPU: 3 PID: 31 at drivers/usb/core/urb.c:363
usb_submit_urb+0x4c2/0x5b0
<...>
Call Trace:
 u2fzero_rng_read+0x16e/0x340 [hid_u2fzero]
 ? ttwu_do_activate+0x67/0x90
 add_early_randomness+0x53/0xc0
 hwrng_register+0x175/0x180
 devm_hwrng_register+0x41/0x7e
 u2fzero_probe+0x2dd/0x350 [hid_u2fzero]
 hid_device_probe+0x119/0x180 [hid]
 really_probe+0xfe/0x3b0
 driver_probe_device+0xba/0x100
 __device_attach_driver+0x97/0x100

I don’t understand why since 1) it’s likely to be the first transmission
of this URB, 2) there’s a mutex locked just before it. I received a
comment from a colleague mentioning I’m probably not using the mutex
correctly, but I don’t understand why.

I’m trying to figure this out, so I’d welcome any help with it.

Thanks in advance.

> +	if (unlikely(ret)) {
> +		hid_err(hdev, "usb_submit_urb failed: %d", ret);
> +		goto err;
> +	}
> +
> +	ret = hid_hw_output_report(dev->hdev, dev->buf_out,
> +				   sizeof(struct u2f_hid_msg));
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "hid_hw_output_report failed: %d", ret);
> +		goto err;
> +	}
> +
> +	ret = (wait_for_completion_timeout(
> +		&ctx.done, msecs_to_jiffies(USB_CTRL_SET_TIMEOUT)));
> +	if (ret < 0) {
> +		usb_kill_urb(dev->urb);
> +		hid_err(hdev, "urb submission timed out");
> +	} else {
> +		ret = dev->urb->actual_length;
> +		memcpy(resp, dev->buf_in, ret);
> +	}
> +
> +err:
> +	mutex_unlock(&dev->lock);
> +
> +	return ret;
> +}

<...>

> +static int u2fzero_rng_read(struct hwrng *rng, void *data,
> +			    size_t max, bool wait)
> +{
> +	struct u2fzero_device *dev = container_of(rng,
> +		struct u2fzero_device, hwrng);
> +	struct u2f_hid_report req = {
> +		.report_type = 0,
> +		.msg.cid = CID_BROADCAST,
> +		.msg.init = {
> +			.cmd = U2F_CUSTOM_GET_RNG,
> +			.bcnth = 0,
> +			.bcntl = 0,
> +			.data  = {0},
> +		}
> +	};
> +	struct u2f_hid_msg resp;
> +	int ret;
> +	size_t actual_length;
> +
> +	if (!dev->present) {
> +		hid_dbg(dev->hdev, "device not present");
> +		return 0;
> +	}
> +
> +	ret = u2fzero_recv(dev, &req, &resp);
> +	if (ret < 0)
> +		return 0;
> +
> +	/* only take the minimum amount of data it is safe to take */
> +	actual_length = min3((size_t)ret - offsetof(struct u2f_hid_msg,
> +		init.data), U2F_HID_MSG_LEN(resp), max);
> +
> +	memcpy(data, resp.init.data, actual_length);
> +
> +	return actual_length;
> +}

<...>

> +static int u2fzero_init_hwrng(struct u2fzero_device *dev,
> +			      unsigned int minor)
> +{
> +	dev->rng_name = devm_kasprintf(&dev->hdev->dev, GFP_KERNEL,
> +		"%s-rng%u", DRIVER_SHORT, minor);
> +	if (dev->rng_name == NULL)
> +		return -ENOMEM;
> +
> +	dev->hwrng.name = dev->rng_name;
> +	dev->hwrng.read = u2fzero_rng_read;
> +	dev->hwrng.quality = 1;
> +
> +	return devm_hwrng_register(&dev->hdev->dev, &dev->hwrng);
> +}
> +
> +static int u2fzero_fill_in_urb(struct u2fzero_device *dev)
> +{
> +	struct hid_device *hdev = dev->hdev;
> +	struct usb_device *udev;
> +	struct usbhid_device *usbhid = hdev->driver_data;
> +	unsigned int pipe_in;
> +	struct usb_host_endpoint *ep;
> +
> +	if (dev->hdev->bus != BUS_USB)
> +		return -EINVAL;
> +
> +	udev = hid_to_usb_dev(hdev);
> +
> +	if (!usbhid->urbout || !usbhid->urbin)
> +		return -ENODEV;
> +
> +	ep = usb_pipe_endpoint(udev, usbhid->urbin->pipe);
> +	if (!ep)
> +		return -ENODEV;
> +
> +	dev->urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!dev->urb)
> +		return -ENOMEM;
> +
> +	pipe_in = (usbhid->urbin->pipe & ~(3 << 30)) | (PIPE_INTERRUPT << 30);
> +
> +	usb_fill_int_urb(dev->urb,
> +		udev,
> +		pipe_in,
> +		dev->buf_in,
> +		HID_REPORT_SIZE,
> +		u2fzero_read_callback,
> +		NULL,
> +		ep->desc.bInterval);
> +
> +	return 0;
> +}
> +
> +static int u2fzero_probe(struct hid_device *hdev,
> +			 const struct hid_device_id *id)
> +{
> +	struct u2fzero_device *dev;
> +	unsigned int minor;
> +	int ret;
> +
> +	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (dev == NULL)
> +		return -ENOMEM;
> +
> +	dev->buf_out = devm_kmalloc(&hdev->dev,
> +		sizeof(struct u2f_hid_report), GFP_KERNEL);
> +	if (dev->buf_out == NULL)
> +		return -ENOMEM;
> +
> +	dev->buf_in = devm_kmalloc(&hdev->dev,
> +		sizeof(struct u2f_hid_msg), GFP_KERNEL);
> +	if (dev->buf_in == NULL)
> +		return -ENOMEM;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	dev->hdev = hdev;
> +	hid_set_drvdata(hdev, dev);
> +	mutex_init(&dev->lock);
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
> +		return ret;
> +
> +	u2fzero_fill_in_urb(dev);
> +
> +	dev->present = true;
> +
> +	minor = ((struct hidraw *) hdev->hidraw)->minor;
> +
> +	ret = u2fzero_init_led(dev, minor);
> +	if (ret) {
> +		hid_hw_stop(hdev);
> +		return ret;
> +	}
> +
> +	hid_info(hdev, "U2F Zero LED initialised\n");
> +
> +	ret = u2fzero_init_hwrng(dev, minor);
> +	if (ret) {
> +		hid_hw_stop(hdev);
> +		return ret;
> +	}
> +
> +	hid_info(hdev, "U2F Zero RNG initialised\n");
> +
> +	return 0;
> +}

-- 
Cheers,
  Andrej