Re: [PATCH v3 08/10] HID: apple: Add DockChannel HID transport driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] hid: use-after-free in dchid_cmd() if unbound with pending commands
- [Medium] hid: out-of-bounds read due to using hid_input_report() instead of hid_safe_input_report()
- [Medium] hid: concurrent command submissions trigger WARN_ON and fail with -EBUSY
- [Medium] hid: race condition between internal STM requests and userspace in dchid_handle_ready()
--

commit 9430d372e505704cf501df7e6ffaa4952a5855aa
Author: Michael Reeves [off-list ref]

HID: apple: Add DockChannel HID transport driver

This commit adds a new HID transport driver for Apple DockChannel. It boots
the MTP RTKit coprocessor, exchanges HID packets through the DockChannel
mailbox, and registers child HID interfaces from devicetree.

[ ... ]

+static int dchid_cmd(struct dchid_iface *iface, u32 type, u32 req,
+		     void *data, size_t size, void *resp_buf, size_t resp_size)
+{
+	unsigned long flags;
+	int ret;
+	int report_id;
+	bool timed_out = false;
+	u32 out_flags;
+
+	if (size < 1)
+		return -EINVAL;
+
+	report_id = *(u8 *)data;
+	out_flags = FIELD_PREP(FLAGS_GROUP, type) | FIELD_PREP(FLAGS_REQ, req);
+
+	spin_lock_irqsave(&iface->out_lock, flags);
+
+	/* Only one command can be in flight per interface */
+	if (WARN_ON(iface->out_report != -1)) {
+		spin_unlock_irqrestore(&iface->out_lock, flags);
+		return -EBUSY;
+	}

[Severity: Medium]
Can this warning be triggered by unprivileged userspace or during normal
initialization?

The dchid_cmd() function rejects concurrent commands with a kernel warning
and returns an error. Since the HID core does not serialize raw_request
ioctls, concurrent ioctls on the hidraw node can trigger this.

Additionally, dchid_open() invokes dchid_start_interface() which calls
dchid_comm_cmd() on the shared "comm" interface. If multiple child devices
are opened concurrently, they might race on the shared interface and hit
this warning. Could the driver wait for the interface to become free or
queue the commands instead of returning an error?

+
+	iface->out_report = report_id;
+	iface->out_flags = out_flags;
+	iface->retcode = 0;
+	iface->resp_buf = resp_buf;
+	iface->resp_size = resp_size;
+	reinit_completion(&iface->out_complete);
+
+	spin_unlock_irqrestore(&iface->out_lock, flags);
+
+	ret = dchid_send(iface, out_flags, data, size);
+	if (ret < 0) {
+		spin_lock_irqsave(&iface->out_lock, flags);
+		iface->out_report = -1;
+		iface->resp_buf = NULL;
+		iface->resp_size = 0;
+		spin_unlock_irqrestore(&iface->out_lock, flags);
+		return ret;
+	}
+
+	if (!wait_for_completion_timeout(&iface->out_complete,
+					 msecs_to_jiffies(COMMAND_TIMEOUT_MS))) {
+		dev_err(iface->dchid->dev, "command 0x%x to iface %d (%s) timed out\n",
+			report_id, iface->index, iface->name);
+		timed_out = true;
+	}
+
+	spin_lock_irqsave(&iface->out_lock, flags);

[Severity: High]
Is there a use-after-free risk here if the driver is unbound while commands
are pending?

A thread blocking in wait_for_completion_timeout() could wake up after
dchid_remove() returns. The dchid_shutdown() function stops the hardware
and destroys the HID devices, but does not abort or wait for pending
commands. After the remove callback returns, devres frees the iface
structure. When the blocked thread times out, it would access freed memory
at spin_lock_irqsave(&iface->out_lock, flags).

[ ... ]

+static void dchid_handle_ready(struct dchid_dev *dchid, void *data, size_t length)
+{

[ ... ]

+	iface = dchid->ifaces[index];
+	if (!iface)
+		return;
+
+	dev_dbg(dchid->dev, "interface %s is now ready\n", iface->name);
+	complete_all(&iface->ready);
+
+	/* When STM is ready, grab global device info */
+	if (!strcmp(iface->name, "stm")) {
+		ret = dchid_get_report_cmd(iface, STM_REPORT_ID, &dchid->device_id,

[Severity: Medium]
Does signaling the completion before internal requests are sent create a
race condition with userspace?

By calling complete_all(&iface->ready) before fetching the STM report ID,
any thread waiting in dchid_open() is woken up. A userspace process could
then immediately issue a synchronous HID request, entering dchid_cmd()
concurrently while dchid_handle_ready() is issuing its own internal STM
request via dchid_get_report_cmd(). This would trigger the lock check
warning in dchid_cmd() and fail with an error.

Should the completion be signaled only after internal requests are
finished?

[ ... ]

+static void dchid_handle_report(struct dchid_iface *iface, void *data, size_t length)
+{
+	if (!iface->hid || !iface->open)
+		return;
+
+	hid_input_report(iface->hid, HID_INPUT_REPORT, data, length, 1);

[Severity: Medium]
Does this allow an out-of-bounds read in the HID core?

The raw data is passed to hid_input_report() without specifying its
allocated bounds. If the device sends a maliciously truncated packet but
declares a large report size in its descriptor, the core could read past
the allocated bounds of the buffer.

Should this use hid_safe_input_report() to enforce explicit boundary checks
based on the allocated buffer size?

+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-apple-mtp-keyboard-final-v3-0-e356464e454b@gmail.com?part=8