[systemd-devel] [PATCH] udev: Add hidraw_id and a rule file to invoke it

[Tom Gundersen]

Hi Andy,

On Tue, Oct 28, 2014 at 11:46 PM, Andy Lutomirski <luto at amacapital.net> wrote:
> So far, hidraw_id detects U2F tokens and sets:
> ID_U2F_TOKEN=1
> ID_SECURITY_TOKEN=1
>
> This causes the uaccess rules to apply to U2F devices.
> ---
>
> I've never written any udev code before.  Feedback welcome.
>
> If you think this doesn't belong in udev, I can try to find it another home.

As mentioned, I don't think we should do this in core udev code (udev
rule would be ok), but making it generic and exposing all the usages
sounds useful. I don't have a strong opinion on whether this should be
in udev or in the kernel, but if you end up going with udev, here are
some comments.

Also, we don't want to add any more external helpers to the udev
codebase, so we should just make this a builtin (analogous to
udev-builtin-usb_id.c).

Minor comments inline.

[...]

> diff --git a/src/udev/hidraw_id/hidraw_id.c b/src/udev/hidraw_id/hidraw_id.c
> new file mode 100644
> index 000000000000..e32f222f22f9
> --- /dev/null
> +++ b/src/udev/hidraw_id/hidraw_id.c
> @@ -0,0 +1,144 @@
> +/*
> + * Copyright (c) Andrew Lutomirski, 2014
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by

New udev code must be LGPL2+.

[...]

> +int main(int argc, char **argv)
> +{
> +        struct udev *udev;
> +        struct udev_device *dev, *hiddev;
> +        char path[4096];

Maybe, avoid hardcoding the length, and use "_cleaunp_free_ char *path
= NULL;" and asprintf() like in the other builtins?

> +        unsigned char desc[4096];

I guess we can use "unsigned char desc[HID_MAX_DESCRIPTOR_SIZE];" here?

[...]

> +        /* Parse the report descriptor. */
> +        for (i = 0; i < desclen; ) {
> +                unsigned char tag = desc[i] >> 4;
> +                unsigned char type = (desc[i] >> 2) & 0x3;
> +                unsigned char sizecode = desc[i] & 0x3;
> +                int size, j;
> +                unsigned int value = 0;
> +
> +                if (desc[i] == 0xfe) {

Please introduce defines for any magic constants to make the code a
bit more self-documenting.
#define HIDRAW_REPORT_DESCRIPTOR_LONG_ITEM 0xfe

Same below, please introduce something like

#define HIDRAW_REPORT_DESCRIPTOR_TYPE_GLOBAL 0x1
#define HIDRAW_REPORT_DESCRIPTOR_TYPE_LOCAL 0x2
#define HIDRAW_REPORT_DESCRIPTOR_GLOBAL_ITEM_USAGE_PAGE 0x0
#define HIDRAW_REPORT_DESCRIPTOR_LOCAL_ITEM_USAGE 0x0

(you can probably think of better/shorter names though).

> +                        /* Long item; skip it. */
> +                        if (i + 1 >= desclen) {
> +                                log_error("bad report_descriptor");
> +                                goto out;
> +                        }
> +                        i += (desc[i+1] + 3);  /* Can't overflow. */
> +                        continue;
> +                }
> +
> +                size = (sizecode < 3 ? sizecode : 4);
> +
> +                if (i + 1 + size > desclen) {
> +                        log_error("bad report_descriptor");
> +                        goto out;
> +                }
> +
> +                for (j = 0; j < size; j++)
> +                        value |= (desc[i + 1 + j] << 8*j);
> +
> +                if (type == 1 && tag == 0)
> +                        usage_page = value;

Should we verify that the data has the right length (2 or 4 bytes)? I
guess we also need to handle 2-byte usage pages specially?

> +
> +                /*
> +                 * Detect U2F tokens.  See:
> +                 * https://fidoalliance.org/specs/fido-u2f-HID-protocol-v1.0-rd-20141008.pdf
> +                 * http://www.usb.org/developers/hidpage/HUTRR48.pdf
> +                 */
> +
> +                if (type == 2 && tag == 0) {
> +                        if (usage_page == 0xf1d0 && value == 0x1)
> +                                is_u2f_token = 1;
> +                }
> +
> +                i += 1 + size;
> +        }

[...]

Cheers,

Tom