Re: [systemd-devel] [PATCH] udev: Add hidraw_id and a rule file to invoke it
Hi Andy,
On Tue, Oct 28, 2014 at 11:46 PM, Andy Lutomirski l...@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 ..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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] udev: Add hidraw_id and a rule file to invoke it
On Mon, Nov 3, 2014 at 5:32 AM, Tom Gundersen t...@jklm.no wrote:
Hi Andy,
On Tue, Oct 28, 2014 at 11:46 PM, Andy Lutomirski l...@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.
Thanks. I'll wait to see what Jiri thinks before sending an updated version.
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 ..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+.
OK.
[snipped things I'll change if I resubmit]
+/* 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?
I think we're not supposed to check the length in general, but I
should re-read the part about two-byte usage pages. (Although, off
the top of my head, I thought that the special case was for very long
usages, not usage pages.)
[...]
--Andy
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] udev: Add hidraw_id and a rule file to invoke it
On Mon, Nov 3, 2014 at 5:47 PM, Andy Lutomirski l...@amacapital.net wrote:
On Mon, Nov 3, 2014 at 5:32 AM, Tom Gundersen t...@jklm.no wrote:
Hi Andy,
On Tue, Oct 28, 2014 at 11:46 PM, Andy Lutomirski l...@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.
Thanks. I'll wait to see what Jiri thinks before sending an updated version.
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 ..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+.
OK.
[snipped things I'll change if I resubmit]
+/* 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?
I think we're not supposed to check the length in general, but I
should re-read the part about two-byte usage pages. (Although, off
the top of my head, I thought that the special case was for very long
usages, not usage pages.)
Ah, you are right. usage_page should always be 2 bytes, and usage can
be 4 bytes or less.
-t
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] udev: Add hidraw_id and a rule file to invoke it
On Tue, Oct 28, 2014 at 3:46 PM, Andy Lutomirski l...@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.
This works for the Plug-up security key, too.
--Andy
---
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.
.gitignore | 1 +
Makefile.am| 11
rules/60-hidraw.rules | 7 ++
src/udev/hidraw_id/Makefile| 1 +
src/udev/hidraw_id/hidraw_id.c | 144
+
5 files changed, 164 insertions(+)
create mode 100644 rules/60-hidraw.rules
create mode 12 src/udev/hidraw_id/Makefile
create mode 100644 src/udev/hidraw_id/hidraw_id.c
diff --git a/.gitignore b/.gitignore
index f119b574c777..4bd3cdf08f0d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,6 +34,7 @@
/exported
/exported-*
/gtk-doc.make
+/hidraw_id
/hostnamectl
/install-tree
/journalctl
diff --git a/Makefile.am b/Makefile.am
index fae946a388af..9f64687d32b1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3542,6 +3542,17 @@ udevlibexec_PROGRAMS += \
ata_id
#
--
+hidraw_id_SOURCES = \
+ src/udev/hidraw_id/hidraw_id.c
+
+hidraw_id_LDADD = \
+ libudev-internal.la \
+ libsystemd-shared.la
+
+udevlibexec_PROGRAMS += \
+ hidraw_id
+
+#
--
cdrom_id_SOURCES = \
src/udev/cdrom_id/cdrom_id.c
diff --git a/rules/60-hidraw.rules b/rules/60-hidraw.rules
new file mode 100644
index ..1ee9c812f711
--- /dev/null
+++ b/rules/60-hidraw.rules
@@ -0,0 +1,7 @@
+# do not edit this file, it will be overwritten on update
+
+ACTION==remove, GOTO=hidraw_end
+
+SUBSYSTEM==hidraw, IMPORT{program}=hidraw_id --udev
+
+LABEL=keyboard_end
diff --git a/src/udev/hidraw_id/Makefile b/src/udev/hidraw_id/Makefile
new file mode 12
index ..d0b0e8e0086f
--- /dev/null
+++ b/src/udev/hidraw_id/Makefile
@@ -0,0 +1 @@
+../Makefile
\ No newline at end of file
diff --git a/src/udev/hidraw_id/hidraw_id.c b/src/udev/hidraw_id/hidraw_id.c
new file mode 100644
index ..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
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+#include stdio.h
+#include string.h
+#include sys/types.h
+#include sys/stat.h
+#include fcntl.h
+#include unistd.h
+
+#include libudev.h
+#include libudev-private.h
+
+_printf_(6,0)
+static void log_fn(struct udev *udev, int priority,
+ const char *file, int line, const char *fn,
+ const char *format, va_list args)
+{
+log_metav(priority, file, line, fn, format, args);
+}
+
+int main(int argc, char **argv)
+{
+struct udev *udev;
+struct udev_device *dev, *hiddev;
+char path[4096];
+unsigned char desc[4096];
+int desclen;
+int fd = -1;
+int i;
+int ret = 1;
+unsigned int usage_page = 0;
+int is_u2f_token = 0;
+
+if (argc != 2) {
+fprintf(stderr, Usage: hidraw_id SYSFS_PATH|--udev\n);
+return 1;
+}
+
+log_parse_environment();
+log_open();
+
+udev = udev_new();
+
+udev_set_log_fn(udev, log_fn);
+
+if (!strcmp(argv[1], --udev))
+dev = udev_device_new_from_environment(udev);
+else
+dev = udev_device_new_from_syspath(udev, argv[1]);
+
+if (!dev)
+goto out;
+
+hiddev = udev_device_get_parent(dev);
+if (!hiddev)
+goto out;
+
+if (snprintf(path, sizeof(path), %s/report_descriptor,
+ udev_device_get_syspath(hiddev)) (int)sizeof(path))
+return 1;
+
+fd = open(path, O_RDONLY | O_NOFOLLOW);
+if (fd == -1)
+goto out;
+
+desclen = read(fd, desc, sizeof(desc));
+if (desclen = 0)
+
[systemd-devel] [PATCH] udev: Add hidraw_id and a rule file to invoke it
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.
.gitignore | 1 +
Makefile.am| 11
rules/60-hidraw.rules | 7 ++
src/udev/hidraw_id/Makefile| 1 +
src/udev/hidraw_id/hidraw_id.c | 144 +
5 files changed, 164 insertions(+)
create mode 100644 rules/60-hidraw.rules
create mode 12 src/udev/hidraw_id/Makefile
create mode 100644 src/udev/hidraw_id/hidraw_id.c
diff --git a/.gitignore b/.gitignore
index f119b574c777..4bd3cdf08f0d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,6 +34,7 @@
/exported
/exported-*
/gtk-doc.make
+/hidraw_id
/hostnamectl
/install-tree
/journalctl
diff --git a/Makefile.am b/Makefile.am
index fae946a388af..9f64687d32b1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3542,6 +3542,17 @@ udevlibexec_PROGRAMS += \
ata_id
#
--
+hidraw_id_SOURCES = \
+ src/udev/hidraw_id/hidraw_id.c
+
+hidraw_id_LDADD = \
+ libudev-internal.la \
+ libsystemd-shared.la
+
+udevlibexec_PROGRAMS += \
+ hidraw_id
+
+#
--
cdrom_id_SOURCES = \
src/udev/cdrom_id/cdrom_id.c
diff --git a/rules/60-hidraw.rules b/rules/60-hidraw.rules
new file mode 100644
index ..1ee9c812f711
--- /dev/null
+++ b/rules/60-hidraw.rules
@@ -0,0 +1,7 @@
+# do not edit this file, it will be overwritten on update
+
+ACTION==remove, GOTO=hidraw_end
+
+SUBSYSTEM==hidraw, IMPORT{program}=hidraw_id --udev
+
+LABEL=keyboard_end
diff --git a/src/udev/hidraw_id/Makefile b/src/udev/hidraw_id/Makefile
new file mode 12
index ..d0b0e8e0086f
--- /dev/null
+++ b/src/udev/hidraw_id/Makefile
@@ -0,0 +1 @@
+../Makefile
\ No newline at end of file
diff --git a/src/udev/hidraw_id/hidraw_id.c b/src/udev/hidraw_id/hidraw_id.c
new file mode 100644
index ..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
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+#include stdio.h
+#include string.h
+#include sys/types.h
+#include sys/stat.h
+#include fcntl.h
+#include unistd.h
+
+#include libudev.h
+#include libudev-private.h
+
+_printf_(6,0)
+static void log_fn(struct udev *udev, int priority,
+ const char *file, int line, const char *fn,
+ const char *format, va_list args)
+{
+log_metav(priority, file, line, fn, format, args);
+}
+
+int main(int argc, char **argv)
+{
+struct udev *udev;
+struct udev_device *dev, *hiddev;
+char path[4096];
+unsigned char desc[4096];
+int desclen;
+int fd = -1;
+int i;
+int ret = 1;
+unsigned int usage_page = 0;
+int is_u2f_token = 0;
+
+if (argc != 2) {
+fprintf(stderr, Usage: hidraw_id SYSFS_PATH|--udev\n);
+return 1;
+}
+
+log_parse_environment();
+log_open();
+
+udev = udev_new();
+
+udev_set_log_fn(udev, log_fn);
+
+if (!strcmp(argv[1], --udev))
+dev = udev_device_new_from_environment(udev);
+else
+dev = udev_device_new_from_syspath(udev, argv[1]);
+
+if (!dev)
+goto out;
+
+hiddev = udev_device_get_parent(dev);
+if (!hiddev)
+goto out;
+
+if (snprintf(path, sizeof(path), %s/report_descriptor,
+ udev_device_get_syspath(hiddev)) (int)sizeof(path))
+return 1;
+
+fd = open(path, O_RDONLY | O_NOFOLLOW);
+if (fd == -1)
+goto out;
+
+desclen = read(fd, desc, sizeof(desc));
+if (desclen = 0)
+goto out;
+
+/* 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;
