* [PATCH v3 1/3] selftests/hid: hidraw: add more coverage for hidraw ioctls
2025-09-12 16:58 [PATCH v3 0/3] HID: hidraw: rework ioctls Benjamin Tissoires
@ 2025-09-12 16:58 ` Benjamin Tissoires
2025-09-12 16:58 ` [PATCH v3 2/3] selftests/hid: hidraw: forge wrong ioctls and tests them Benjamin Tissoires
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Benjamin Tissoires @ 2025-09-12 16:58 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Arnd Bergmann
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
Try to ensure all ioctls are having at least one test.
Most of the scaffholding has been generated by claude-4-sonnet and then
carefully reviewed.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/hid_common.h | 6 +
tools/testing/selftests/hid/hidraw.c | 346 +++++++++++++++++++++++++++++++
2 files changed, 352 insertions(+)
diff --git a/tools/testing/selftests/hid/hid_common.h b/tools/testing/selftests/hid/hid_common.h
index f77f69c6657d0f0f66beb3b50bf4b126f6f63348..8085519c47cb505b901ac80f2087dc9a1aa2b9c0 100644
--- a/tools/testing/selftests/hid/hid_common.h
+++ b/tools/testing/selftests/hid/hid_common.h
@@ -230,6 +230,12 @@ static int uhid_event(struct __test_metadata *_metadata, int fd)
break;
case UHID_SET_REPORT:
UHID_LOG("UHID_SET_REPORT from uhid-dev");
+
+ answer.type = UHID_SET_REPORT_REPLY;
+ answer.u.set_report_reply.id = ev.u.set_report.id;
+ answer.u.set_report_reply.err = 0; /* success */
+
+ uhid_write(_metadata, fd, &answer);
break;
default:
TH_LOG("Invalid event from uhid-dev: %u", ev.type);
diff --git a/tools/testing/selftests/hid/hidraw.c b/tools/testing/selftests/hid/hidraw.c
index 821db37ba4bbef82e5cf4b44b6675666f87a12ad..6d61d03e2ef05e1900fe5a3938d93421717b2621 100644
--- a/tools/testing/selftests/hid/hidraw.c
+++ b/tools/testing/selftests/hid/hidraw.c
@@ -2,6 +2,9 @@
/* Copyright (c) 2022-2024 Red Hat */
#include "hid_common.h"
+#include <linux/input.h>
+#include <string.h>
+#include <sys/ioctl.h>
/* for older kernels */
#ifndef HIDIOCREVOKE
@@ -215,6 +218,349 @@ TEST_F(hidraw, write_event_revoked)
pthread_mutex_unlock(&uhid_output_mtx);
}
+/*
+ * Test HIDIOCGRDESCSIZE ioctl to get report descriptor size
+ */
+TEST_F(hidraw, ioctl_rdescsize)
+{
+ int desc_size = 0;
+ int err;
+
+ /* call HIDIOCGRDESCSIZE ioctl */
+ err = ioctl(self->hidraw_fd, HIDIOCGRDESCSIZE, &desc_size);
+ ASSERT_EQ(err, 0) TH_LOG("HIDIOCGRDESCSIZE ioctl failed");
+
+ /* verify the size matches our test report descriptor */
+ ASSERT_EQ(desc_size, sizeof(rdesc))
+ TH_LOG("expected size %zu, got %d", sizeof(rdesc), desc_size);
+}
+
+/*
+ * Test HIDIOCGRDESC ioctl to get report descriptor data
+ */
+TEST_F(hidraw, ioctl_rdesc)
+{
+ struct hidraw_report_descriptor desc;
+ int err;
+
+ /* get the full report descriptor */
+ desc.size = sizeof(rdesc);
+ err = ioctl(self->hidraw_fd, HIDIOCGRDESC, &desc);
+ ASSERT_EQ(err, 0) TH_LOG("HIDIOCGRDESC ioctl failed");
+
+ /* verify the descriptor data matches our test descriptor */
+ ASSERT_EQ(memcmp(desc.value, rdesc, sizeof(rdesc)), 0)
+ TH_LOG("report descriptor data mismatch");
+}
+
+/*
+ * Test HIDIOCGRDESC ioctl with smaller buffer size
+ */
+TEST_F(hidraw, ioctl_rdesc_small_buffer)
+{
+ struct hidraw_report_descriptor desc;
+ int err;
+ size_t small_size = sizeof(rdesc) / 2; /* request half the descriptor size */
+
+ /* get partial report descriptor */
+ desc.size = small_size;
+ err = ioctl(self->hidraw_fd, HIDIOCGRDESC, &desc);
+ ASSERT_EQ(err, 0) TH_LOG("HIDIOCGRDESC ioctl failed with small buffer");
+
+ /* verify we got the first part of the descriptor */
+ ASSERT_EQ(memcmp(desc.value, rdesc, small_size), 0)
+ TH_LOG("partial report descriptor data mismatch");
+}
+
+/*
+ * Test HIDIOCGRAWINFO ioctl to get device information
+ */
+TEST_F(hidraw, ioctl_rawinfo)
+{
+ struct hidraw_devinfo devinfo;
+ int err;
+
+ /* get device info */
+ err = ioctl(self->hidraw_fd, HIDIOCGRAWINFO, &devinfo);
+ ASSERT_EQ(err, 0) TH_LOG("HIDIOCGRAWINFO ioctl failed");
+
+ /* verify device info matches our test setup */
+ ASSERT_EQ(devinfo.bustype, BUS_USB)
+ TH_LOG("expected bustype 0x03, got 0x%x", devinfo.bustype);
+ ASSERT_EQ(devinfo.vendor, 0x0001)
+ TH_LOG("expected vendor 0x0001, got 0x%x", devinfo.vendor);
+ ASSERT_EQ(devinfo.product, 0x0a37)
+ TH_LOG("expected product 0x0a37, got 0x%x", devinfo.product);
+}
+
+/*
+ * Test HIDIOCGFEATURE ioctl to get feature report
+ */
+TEST_F(hidraw, ioctl_gfeature)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* set report ID 1 in first byte */
+ buf[0] = 1;
+
+ /* get feature report */
+ err = ioctl(self->hidraw_fd, HIDIOCGFEATURE(sizeof(buf)), buf);
+ ASSERT_EQ(err, sizeof(feature_data)) TH_LOG("HIDIOCGFEATURE ioctl failed, got %d", err);
+
+ /* verify we got the expected feature data */
+ ASSERT_EQ(buf[0], feature_data[0])
+ TH_LOG("expected feature_data[0] = %d, got %d", feature_data[0], buf[0]);
+ ASSERT_EQ(buf[1], feature_data[1])
+ TH_LOG("expected feature_data[1] = %d, got %d", feature_data[1], buf[1]);
+}
+
+/*
+ * Test HIDIOCGFEATURE ioctl with invalid report ID
+ */
+TEST_F(hidraw, ioctl_gfeature_invalid)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* set invalid report ID (not 1) */
+ buf[0] = 2;
+
+ /* try to get feature report */
+ err = ioctl(self->hidraw_fd, HIDIOCGFEATURE(sizeof(buf)), buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGFEATURE should have failed with invalid report ID");
+ ASSERT_EQ(errno, EIO) TH_LOG("expected EIO, got errno %d", errno);
+}
+
+/*
+ * Test HIDIOCSFEATURE ioctl to set feature report
+ */
+TEST_F(hidraw, ioctl_sfeature)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* prepare feature report data */
+ buf[0] = 1; /* report ID */
+ buf[1] = 0x42;
+ buf[2] = 0x24;
+
+ /* set feature report */
+ err = ioctl(self->hidraw_fd, HIDIOCSFEATURE(3), buf);
+ ASSERT_EQ(err, 3) TH_LOG("HIDIOCSFEATURE ioctl failed, got %d", err);
+
+ /*
+ * Note: The uhid mock doesn't validate the set report data,
+ * so we just verify the ioctl succeeds
+ */
+}
+
+/*
+ * Test HIDIOCGINPUT ioctl to get input report
+ */
+TEST_F(hidraw, ioctl_ginput)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* set report ID 1 in first byte */
+ buf[0] = 1;
+
+ /* get input report */
+ err = ioctl(self->hidraw_fd, HIDIOCGINPUT(sizeof(buf)), buf);
+ ASSERT_EQ(err, sizeof(feature_data)) TH_LOG("HIDIOCGINPUT ioctl failed, got %d", err);
+
+ /* verify we got the expected input data */
+ ASSERT_EQ(buf[0], feature_data[0])
+ TH_LOG("expected feature_data[0] = %d, got %d", feature_data[0], buf[0]);
+ ASSERT_EQ(buf[1], feature_data[1])
+ TH_LOG("expected feature_data[1] = %d, got %d", feature_data[1], buf[1]);
+}
+
+/*
+ * Test HIDIOCGINPUT ioctl with invalid report ID
+ */
+TEST_F(hidraw, ioctl_ginput_invalid)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* set invalid report ID (not 1) */
+ buf[0] = 2;
+
+ /* try to get input report */
+ err = ioctl(self->hidraw_fd, HIDIOCGINPUT(sizeof(buf)), buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGINPUT should have failed with invalid report ID");
+ ASSERT_EQ(errno, EIO) TH_LOG("expected EIO, got errno %d", errno);
+}
+
+/*
+ * Test HIDIOCSINPUT ioctl to set input report
+ */
+TEST_F(hidraw, ioctl_sinput)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* prepare input report data */
+ buf[0] = 1; /* report ID */
+ buf[1] = 0x55;
+ buf[2] = 0xAA;
+
+ /* set input report */
+ err = ioctl(self->hidraw_fd, HIDIOCSINPUT(3), buf);
+ ASSERT_EQ(err, 3) TH_LOG("HIDIOCSINPUT ioctl failed, got %d", err);
+
+ /*
+ * Note: The uhid mock doesn't validate the set report data,
+ * so we just verify the ioctl succeeds
+ */
+}
+
+/*
+ * Test HIDIOCGOUTPUT ioctl to get output report
+ */
+TEST_F(hidraw, ioctl_goutput)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* set report ID 1 in first byte */
+ buf[0] = 1;
+
+ /* get output report */
+ err = ioctl(self->hidraw_fd, HIDIOCGOUTPUT(sizeof(buf)), buf);
+ ASSERT_EQ(err, sizeof(feature_data)) TH_LOG("HIDIOCGOUTPUT ioctl failed, got %d", err);
+
+ /* verify we got the expected output data */
+ ASSERT_EQ(buf[0], feature_data[0])
+ TH_LOG("expected feature_data[0] = %d, got %d", feature_data[0], buf[0]);
+ ASSERT_EQ(buf[1], feature_data[1])
+ TH_LOG("expected feature_data[1] = %d, got %d", feature_data[1], buf[1]);
+}
+
+/*
+ * Test HIDIOCGOUTPUT ioctl with invalid report ID
+ */
+TEST_F(hidraw, ioctl_goutput_invalid)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* set invalid report ID (not 1) */
+ buf[0] = 2;
+
+ /* try to get output report */
+ err = ioctl(self->hidraw_fd, HIDIOCGOUTPUT(sizeof(buf)), buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGOUTPUT should have failed with invalid report ID");
+ ASSERT_EQ(errno, EIO) TH_LOG("expected EIO, got errno %d", errno);
+}
+
+/*
+ * Test HIDIOCSOUTPUT ioctl to set output report
+ */
+TEST_F(hidraw, ioctl_soutput)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* prepare output report data */
+ buf[0] = 1; /* report ID */
+ buf[1] = 0x33;
+ buf[2] = 0xCC;
+
+ /* set output report */
+ err = ioctl(self->hidraw_fd, HIDIOCSOUTPUT(3), buf);
+ ASSERT_EQ(err, 3) TH_LOG("HIDIOCSOUTPUT ioctl failed, got %d", err);
+
+ /*
+ * Note: The uhid mock doesn't validate the set report data,
+ * so we just verify the ioctl succeeds
+ */
+}
+
+/*
+ * Test HIDIOCGRAWNAME ioctl to get device name string
+ */
+TEST_F(hidraw, ioctl_rawname)
+{
+ char name[256] = {0};
+ char expected_name[64];
+ int err;
+
+ /* get device name */
+ err = ioctl(self->hidraw_fd, HIDIOCGRAWNAME(sizeof(name)), name);
+ ASSERT_GT(err, 0) TH_LOG("HIDIOCGRAWNAME ioctl failed, got %d", err);
+
+ /* construct expected name based on device id */
+ snprintf(expected_name, sizeof(expected_name), "test-uhid-device-%d", self->hid.dev_id);
+
+ /* verify the name matches expected pattern */
+ ASSERT_EQ(strcmp(name, expected_name), 0)
+ TH_LOG("expected name '%s', got '%s'", expected_name, name);
+}
+
+/*
+ * Test HIDIOCGRAWPHYS ioctl to get device physical address string
+ */
+TEST_F(hidraw, ioctl_rawphys)
+{
+ char phys[256] = {0};
+ char expected_phys[64];
+ int err;
+
+ /* get device physical address */
+ err = ioctl(self->hidraw_fd, HIDIOCGRAWPHYS(sizeof(phys)), phys);
+ ASSERT_GT(err, 0) TH_LOG("HIDIOCGRAWPHYS ioctl failed, got %d", err);
+
+ /* construct expected phys based on device id */
+ snprintf(expected_phys, sizeof(expected_phys), "%d", self->hid.dev_id);
+
+ /* verify the phys matches expected value */
+ ASSERT_EQ(strcmp(phys, expected_phys), 0)
+ TH_LOG("expected phys '%s', got '%s'", expected_phys, phys);
+}
+
+/*
+ * Test HIDIOCGRAWUNIQ ioctl to get device unique identifier string
+ */
+TEST_F(hidraw, ioctl_rawuniq)
+{
+ char uniq[256] = {0};
+ int err;
+
+ /* get device unique identifier */
+ err = ioctl(self->hidraw_fd, HIDIOCGRAWUNIQ(sizeof(uniq)), uniq);
+ ASSERT_GE(err, 0) TH_LOG("HIDIOCGRAWUNIQ ioctl failed, got %d", err);
+
+ /* uniq is typically empty in our test setup */
+ ASSERT_EQ(strlen(uniq), 0) TH_LOG("expected empty uniq, got '%s'", uniq);
+}
+
+/*
+ * Test device string ioctls with small buffer sizes
+ */
+TEST_F(hidraw, ioctl_strings_small_buffer)
+{
+ char small_buf[8] = {0};
+ char expected_name[64];
+ int err;
+
+ /* test HIDIOCGRAWNAME with small buffer */
+ err = ioctl(self->hidraw_fd, HIDIOCGRAWNAME(sizeof(small_buf)), small_buf);
+ ASSERT_EQ(err, sizeof(small_buf))
+ TH_LOG("HIDIOCGRAWNAME with small buffer failed, got %d", err);
+
+ /* construct expected truncated name */
+ snprintf(expected_name, sizeof(expected_name), "test-uhid-device-%d", self->hid.dev_id);
+
+ /* verify we got truncated name (first 8 chars, no null terminator guaranteed) */
+ ASSERT_EQ(strncmp(small_buf, expected_name, sizeof(small_buf)), 0)
+ TH_LOG("expected truncated name to match first %zu chars", sizeof(small_buf));
+
+ /* Note: hidraw driver doesn't guarantee null termination when buffer is too small */
+}
+
int main(int argc, char **argv)
{
return test_harness_run(argc, argv);
--
2.51.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v3 2/3] selftests/hid: hidraw: forge wrong ioctls and tests them
2025-09-12 16:58 [PATCH v3 0/3] HID: hidraw: rework ioctls Benjamin Tissoires
2025-09-12 16:58 ` [PATCH v3 1/3] selftests/hid: hidraw: add more coverage for hidraw ioctls Benjamin Tissoires
@ 2025-09-12 16:58 ` Benjamin Tissoires
2025-09-12 16:58 ` [PATCH v3 3/3] HID: hidraw: tighten ioctl command parsing Benjamin Tissoires
2025-09-17 9:38 ` [PATCH v3 0/3] HID: hidraw: rework ioctls Jiri Kosina
3 siblings, 0 replies; 5+ messages in thread
From: Benjamin Tissoires @ 2025-09-12 16:58 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Arnd Bergmann
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
We also need coverage for when the malicious user is not using the
proper ioctls definitions and tries to work around the driver.
Most of the scaffholding has been generated by claude-4-sonnet and then
carefully reviewed.
Suggested-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/hidraw.c | 127 +++++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/tools/testing/selftests/hid/hidraw.c b/tools/testing/selftests/hid/hidraw.c
index 6d61d03e2ef05e1900fe5a3938d93421717b2621..d625772f8b7cf71fd94956d3a49d54ff44e2b34d 100644
--- a/tools/testing/selftests/hid/hidraw.c
+++ b/tools/testing/selftests/hid/hidraw.c
@@ -332,6 +332,133 @@ TEST_F(hidraw, ioctl_gfeature_invalid)
ASSERT_EQ(errno, EIO) TH_LOG("expected EIO, got errno %d", errno);
}
+/*
+ * Test ioctl with incorrect nr bits
+ */
+TEST_F(hidraw, ioctl_invalid_nr)
+{
+ char buf[256] = {0};
+ int err;
+ unsigned int bad_cmd;
+
+ /*
+ * craft an ioctl command with wrong _IOC_NR bits
+ */
+ bad_cmd = _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x00, sizeof(buf)); /* 0 is not valid */
+
+ /* test the ioctl */
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("ioctl read-write with wrong _IOC_NR (0) should have failed");
+ ASSERT_EQ(errno, ENOTTY)
+ TH_LOG("expected ENOTTY for wrong read-write _IOC_NR (0), got errno %d", errno);
+
+ /*
+ * craft an ioctl command with wrong _IOC_NR bits
+ */
+ bad_cmd = _IOC(_IOC_READ, 'H', 0x00, sizeof(buf)); /* 0 is not valid */
+
+ /* test the ioctl */
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("ioctl read-only with wrong _IOC_NR (0) should have failed");
+ ASSERT_EQ(errno, ENOTTY)
+ TH_LOG("expected ENOTTY for wrong read-only _IOC_NR (0), got errno %d", errno);
+
+ /* also test with bigger number */
+ bad_cmd = _IOC(_IOC_READ, 'H', 0x42, sizeof(buf)); /* 0x42 is not valid as well */
+
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("ioctl read-only with wrong _IOC_NR (0x42) should have failed");
+ ASSERT_EQ(errno, ENOTTY)
+ TH_LOG("expected ENOTTY for wrong read-only _IOC_NR (0x42), got errno %d", errno);
+
+ /* also test with bigger number: 0x42 is not valid as well */
+ bad_cmd = _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x42, sizeof(buf));
+
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("ioctl read-write with wrong _IOC_NR (0x42) should have failed");
+ ASSERT_EQ(errno, ENOTTY)
+ TH_LOG("expected ENOTTY for wrong read-write _IOC_NR (0x42), got errno %d", errno);
+}
+
+/*
+ * Test ioctl with incorrect type bits
+ */
+TEST_F(hidraw, ioctl_invalid_type)
+{
+ char buf[256] = {0};
+ int err;
+ unsigned int bad_cmd;
+
+ /*
+ * craft an ioctl command with wrong _IOC_TYPE bits
+ */
+ bad_cmd = _IOC(_IOC_WRITE|_IOC_READ, 'I', 0x01, sizeof(buf)); /* 'I' should be 'H' */
+
+ /* test the ioctl */
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("ioctl with wrong _IOC_TYPE (I) should have failed");
+ ASSERT_EQ(errno, EINVAL) TH_LOG("expected EINVAL for wrong _IOC_NR, got errno %d", errno);
+}
+
+/*
+ * Test HIDIOCGFEATURE ioctl with incorrect _IOC_DIR bits
+ */
+TEST_F(hidraw, ioctl_gfeature_invalid_dir)
+{
+ __u8 buf[10] = {0};
+ int err;
+ unsigned int bad_cmd;
+
+ /* set report ID 1 in first byte */
+ buf[0] = 1;
+
+ /*
+ * craft an ioctl command with wrong _IOC_DIR bits
+ * HIDIOCGFEATURE should have _IOC_WRITE|_IOC_READ, let's use only _IOC_WRITE
+ */
+ bad_cmd = _IOC(_IOC_WRITE, 'H', 0x07, sizeof(buf)); /* should be _IOC_WRITE|_IOC_READ */
+
+ /* try to get feature report with wrong direction bits */
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGFEATURE with wrong _IOC_DIR should have failed");
+ ASSERT_EQ(errno, EINVAL) TH_LOG("expected EINVAL for wrong _IOC_DIR, got errno %d", errno);
+
+ /* also test with only _IOC_READ */
+ bad_cmd = _IOC(_IOC_READ, 'H', 0x07, sizeof(buf)); /* should be _IOC_WRITE|_IOC_READ */
+
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGFEATURE with wrong _IOC_DIR should have failed");
+ ASSERT_EQ(errno, EINVAL) TH_LOG("expected EINVAL for wrong _IOC_DIR, got errno %d", errno);
+}
+
+/*
+ * Test read-only ioctl with incorrect _IOC_DIR bits
+ */
+TEST_F(hidraw, ioctl_readonly_invalid_dir)
+{
+ char buf[256] = {0};
+ int err;
+ unsigned int bad_cmd;
+
+ /*
+ * craft an ioctl command with wrong _IOC_DIR bits
+ * HIDIOCGRAWNAME should have _IOC_READ, let's use _IOC_WRITE
+ */
+ bad_cmd = _IOC(_IOC_WRITE, 'H', 0x04, sizeof(buf)); /* should be _IOC_READ */
+
+ /* try to get device name with wrong direction bits */
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGRAWNAME with wrong _IOC_DIR should have failed");
+ ASSERT_EQ(errno, EINVAL) TH_LOG("expected EINVAL for wrong _IOC_DIR, got errno %d", errno);
+
+ /* also test with _IOC_WRITE|_IOC_READ */
+ bad_cmd = _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x04, sizeof(buf)); /* should be only _IOC_READ */
+
+ err = ioctl(self->hidraw_fd, bad_cmd, buf);
+ ASSERT_LT(err, 0) TH_LOG("HIDIOCGRAWNAME with wrong _IOC_DIR should have failed");
+ ASSERT_EQ(errno, EINVAL) TH_LOG("expected EINVAL for wrong _IOC_DIR, got errno %d", errno);
+}
+
/*
* Test HIDIOCSFEATURE ioctl to set feature report
*/
--
2.51.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v3 3/3] HID: hidraw: tighten ioctl command parsing
2025-09-12 16:58 [PATCH v3 0/3] HID: hidraw: rework ioctls Benjamin Tissoires
2025-09-12 16:58 ` [PATCH v3 1/3] selftests/hid: hidraw: add more coverage for hidraw ioctls Benjamin Tissoires
2025-09-12 16:58 ` [PATCH v3 2/3] selftests/hid: hidraw: forge wrong ioctls and tests them Benjamin Tissoires
@ 2025-09-12 16:58 ` Benjamin Tissoires
2025-09-17 9:38 ` [PATCH v3 0/3] HID: hidraw: rework ioctls Jiri Kosina
3 siblings, 0 replies; 5+ messages in thread
From: Benjamin Tissoires @ 2025-09-12 16:58 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Arnd Bergmann
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires,
Arnd Bergmann
The handling for variable-length ioctl commands in hidraw_ioctl() is
rather complex and the check for the data direction is incomplete.
Simplify this code by factoring out the various ioctls grouped by dir
and size, and using a switch() statement with the size masked out, to
ensure the rest of the command is correctly matched.
Fixes: 9188e79ec3fd ("HID: add phys and name ioctls to hidraw")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
drivers/hid/hidraw.c | 224 ++++++++++++++++++++++++--------------------
include/uapi/linux/hidraw.h | 2 +
2 files changed, 124 insertions(+), 102 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index c887f48756f4be2a4bac03128f2885bde96c1e39..bbd6f23bce78951c7d667ff5c1c923cee3509e3f 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -394,27 +394,15 @@ static int hidraw_revoke(struct hidraw_list *list)
return 0;
}
-static long hidraw_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long hidraw_fixed_size_ioctl(struct file *file, struct hidraw *dev, unsigned int cmd,
+ void __user *arg)
{
- struct inode *inode = file_inode(file);
- unsigned int minor = iminor(inode);
- long ret = 0;
- struct hidraw *dev;
- struct hidraw_list *list = file->private_data;
- void __user *user_arg = (void __user*) arg;
-
- down_read(&minors_rwsem);
- dev = hidraw_table[minor];
- if (!dev || !dev->exist || hidraw_is_revoked(list)) {
- ret = -ENODEV;
- goto out;
- }
+ struct hid_device *hid = dev->hid;
switch (cmd) {
case HIDIOCGRDESCSIZE:
- if (put_user(dev->hid->rsize, (int __user *)arg))
- ret = -EFAULT;
+ if (put_user(hid->rsize, (int __user *)arg))
+ return -EFAULT;
break;
case HIDIOCGRDESC:
@@ -422,113 +410,145 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
__u32 len;
if (get_user(len, (int __user *)arg))
- ret = -EFAULT;
- else if (len > HID_MAX_DESCRIPTOR_SIZE - 1)
- ret = -EINVAL;
- else if (copy_to_user(user_arg + offsetof(
- struct hidraw_report_descriptor,
- value[0]),
- dev->hid->rdesc,
- min(dev->hid->rsize, len)))
- ret = -EFAULT;
+ return -EFAULT;
+
+ if (len > HID_MAX_DESCRIPTOR_SIZE - 1)
+ return -EINVAL;
+
+ if (copy_to_user(arg + offsetof(
+ struct hidraw_report_descriptor,
+ value[0]),
+ hid->rdesc,
+ min(hid->rsize, len)))
+ return -EFAULT;
+
break;
}
case HIDIOCGRAWINFO:
{
struct hidraw_devinfo dinfo;
- dinfo.bustype = dev->hid->bus;
- dinfo.vendor = dev->hid->vendor;
- dinfo.product = dev->hid->product;
- if (copy_to_user(user_arg, &dinfo, sizeof(dinfo)))
- ret = -EFAULT;
+ dinfo.bustype = hid->bus;
+ dinfo.vendor = hid->vendor;
+ dinfo.product = hid->product;
+ if (copy_to_user(arg, &dinfo, sizeof(dinfo)))
+ return -EFAULT;
break;
}
case HIDIOCREVOKE:
{
- if (user_arg)
- ret = -EINVAL;
- else
- ret = hidraw_revoke(list);
- break;
+ struct hidraw_list *list = file->private_data;
+
+ if (arg)
+ return -EINVAL;
+
+ return hidraw_revoke(list);
}
default:
- {
- struct hid_device *hid = dev->hid;
- if (_IOC_TYPE(cmd) != 'H') {
- ret = -EINVAL;
- break;
- }
+ /*
+ * None of the above ioctls can return -EAGAIN, so
+ * use it as a marker that we need to check variable
+ * length ioctls.
+ */
+ return -EAGAIN;
+ }
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSFEATURE(0))) {
- int len = _IOC_SIZE(cmd);
- ret = hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
- break;
- }
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGFEATURE(0))) {
- int len = _IOC_SIZE(cmd);
- ret = hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
- break;
- }
+ return 0;
+}
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSINPUT(0))) {
- int len = _IOC_SIZE(cmd);
- ret = hidraw_send_report(file, user_arg, len, HID_INPUT_REPORT);
- break;
- }
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGINPUT(0))) {
- int len = _IOC_SIZE(cmd);
- ret = hidraw_get_report(file, user_arg, len, HID_INPUT_REPORT);
- break;
- }
+static long hidraw_rw_variable_size_ioctl(struct file *file, struct hidraw *dev, unsigned int cmd,
+ void __user *user_arg)
+{
+ int len = _IOC_SIZE(cmd);
+
+ switch (cmd & ~IOCSIZE_MASK) {
+ case HIDIOCSFEATURE(0):
+ return hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
+ case HIDIOCGFEATURE(0):
+ return hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
+ case HIDIOCSINPUT(0):
+ return hidraw_send_report(file, user_arg, len, HID_INPUT_REPORT);
+ case HIDIOCGINPUT(0):
+ return hidraw_get_report(file, user_arg, len, HID_INPUT_REPORT);
+ case HIDIOCSOUTPUT(0):
+ return hidraw_send_report(file, user_arg, len, HID_OUTPUT_REPORT);
+ case HIDIOCGOUTPUT(0):
+ return hidraw_get_report(file, user_arg, len, HID_OUTPUT_REPORT);
+ }
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSOUTPUT(0))) {
- int len = _IOC_SIZE(cmd);
- ret = hidraw_send_report(file, user_arg, len, HID_OUTPUT_REPORT);
- break;
- }
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGOUTPUT(0))) {
- int len = _IOC_SIZE(cmd);
- ret = hidraw_get_report(file, user_arg, len, HID_OUTPUT_REPORT);
- break;
- }
+ return -EINVAL;
+}
- /* Begin Read-only ioctls. */
- if (_IOC_DIR(cmd) != _IOC_READ) {
- ret = -EINVAL;
- break;
- }
+static long hidraw_ro_variable_size_ioctl(struct file *file, struct hidraw *dev, unsigned int cmd,
+ void __user *user_arg)
+{
+ struct hid_device *hid = dev->hid;
+ int len = _IOC_SIZE(cmd);
+ int field_len;
+
+ switch (cmd & ~IOCSIZE_MASK) {
+ case HIDIOCGRAWNAME(0):
+ field_len = strlen(hid->name) + 1;
+ if (len > field_len)
+ len = field_len;
+ return copy_to_user(user_arg, hid->name, len) ? -EFAULT : len;
+ case HIDIOCGRAWPHYS(0):
+ field_len = strlen(hid->phys) + 1;
+ if (len > field_len)
+ len = field_len;
+ return copy_to_user(user_arg, hid->phys, len) ? -EFAULT : len;
+ case HIDIOCGRAWUNIQ(0):
+ field_len = strlen(hid->uniq) + 1;
+ if (len > field_len)
+ len = field_len;
+ return copy_to_user(user_arg, hid->uniq, len) ? -EFAULT : len;
+ }
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWNAME(0))) {
- int len = strlen(hid->name) + 1;
- if (len > _IOC_SIZE(cmd))
- len = _IOC_SIZE(cmd);
- ret = copy_to_user(user_arg, hid->name, len) ?
- -EFAULT : len;
- break;
- }
+ return -EINVAL;
+}
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWPHYS(0))) {
- int len = strlen(hid->phys) + 1;
- if (len > _IOC_SIZE(cmd))
- len = _IOC_SIZE(cmd);
- ret = copy_to_user(user_arg, hid->phys, len) ?
- -EFAULT : len;
- break;
- }
+static long hidraw_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct inode *inode = file_inode(file);
+ unsigned int minor = iminor(inode);
+ struct hidraw *dev;
+ struct hidraw_list *list = file->private_data;
+ void __user *user_arg = (void __user *)arg;
+ int ret;
- if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWUNIQ(0))) {
- int len = strlen(hid->uniq) + 1;
- if (len > _IOC_SIZE(cmd))
- len = _IOC_SIZE(cmd);
- ret = copy_to_user(user_arg, hid->uniq, len) ?
- -EFAULT : len;
- break;
- }
- }
+ down_read(&minors_rwsem);
+ dev = hidraw_table[minor];
+ if (!dev || !dev->exist || hidraw_is_revoked(list)) {
+ ret = -ENODEV;
+ goto out;
+ }
+
+ if (_IOC_TYPE(cmd) != 'H') {
+ ret = -EINVAL;
+ goto out;
+ }
+ if (_IOC_NR(cmd) > HIDIOCTL_LAST || _IOC_NR(cmd) == 0) {
ret = -ENOTTY;
+ goto out;
}
+
+ ret = hidraw_fixed_size_ioctl(file, dev, cmd, user_arg);
+ if (ret != -EAGAIN)
+ goto out;
+
+ switch (_IOC_DIR(cmd)) {
+ case (_IOC_READ | _IOC_WRITE):
+ ret = hidraw_rw_variable_size_ioctl(file, dev, cmd, user_arg);
+ break;
+ case _IOC_READ:
+ ret = hidraw_ro_variable_size_ioctl(file, dev, cmd, user_arg);
+ break;
+ default:
+ /* Any other IOC_DIR is wrong */
+ ret = -EINVAL;
+ }
+
out:
up_read(&minors_rwsem);
return ret;
diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
index d5ee269864e07fcaba481fa285bacbd98739e44f..ebd701b3c18d9d7465880199091933f13f2e1128 100644
--- a/include/uapi/linux/hidraw.h
+++ b/include/uapi/linux/hidraw.h
@@ -48,6 +48,8 @@ struct hidraw_devinfo {
#define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
#define HIDIOCREVOKE _IOW('H', 0x0D, int) /* Revoke device access */
+#define HIDIOCTL_LAST _IOC_NR(HIDIOCREVOKE)
+
#define HIDRAW_FIRST_MINOR 0
#define HIDRAW_MAX_DEVICES 64
/* number of reports to buffer */
--
2.51.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3 0/3] HID: hidraw: rework ioctls
2025-09-12 16:58 [PATCH v3 0/3] HID: hidraw: rework ioctls Benjamin Tissoires
` (2 preceding siblings ...)
2025-09-12 16:58 ` [PATCH v3 3/3] HID: hidraw: tighten ioctl command parsing Benjamin Tissoires
@ 2025-09-17 9:38 ` Jiri Kosina
3 siblings, 0 replies; 5+ messages in thread
From: Jiri Kosina @ 2025-09-17 9:38 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Shuah Khan, Arnd Bergmann, linux-input, linux-kselftest,
linux-kernel, Arnd Bergmann
On Fri, 12 Sep 2025, Benjamin Tissoires wrote:
> Arnd sent the v1 of the series in July, and it was bogus. So with a
> little help from claude-sonnet I built up the missing ioctls tests and
> tried to figure out a way to apply Arnd's logic without breaking the
> existing ioctls.
>
> The end result is in patch 3/3, which makes use of subfunctions to keep
> the main ioctl code path clean.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> Changes in v3:
> - dropped the co-developed-by tag and put a blurb instead
> - change the attribution of patch 3/3 to me as requested by Arnd.
> - Link to v2: https://lore.kernel.org/r/20250826-b4-hidraw-ioctls-v2-0-c7726b236719@kernel.org
>
> changes in v2:
> - add new hidraw ioctls tests
> - refactor Arnd's patch to keep the existing error path logic
> - link to v1: https://lore.kernel.org/linux-input/20250711072847.2836962-1-arnd@kernel.org/
Now queued in hid.git#for-6.18/hidraw, thanks a lot Arnd and Benjamin!
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread