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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] hid: out-of-bounds read due to unsafe hid_input_report()
- [High] soc: apple: leaked running ASC coprocessor on probe failure
- [High] hid: out-of-bounds read in userspace via oversized raw_request return
- [High] hid: UAF due to dynamic HID device registration on probe failure
- [High] hid: data race and potential uninitialized memory access in interface array
- [Medium] hid: memory leak of report descriptor on multiple INIT events
- [Medium] hid: incorrect return value in raw_request for SET_REPORT
- [Low] hid: missing null termination for serial string updates
- [Low] hid: checksum stored in host endianness breaking big-endian systems
--

commit d188bf6bf7d855a9c26413ca6bc664695872df14
Author: Michael Reeves <michael.reeves077@gmail.com>

    HID: apple: Add DockChannel HID transport driver

This commit adds a HID transport layer over the Apple DockChannel mailbox
interface. It enables support for the internal keyboard and trackpad on
Apple M2 and later MacBook models by exchanging HID packets with the
RTKit coprocessor.

> diff --git a/drivers/hid/dockchannel/apple-hid.c b/drivers/hid/dockchannel/apple-hid.c
> --- /dev/null
> +++ b/drivers/hid/dockchannel/apple-hid.c
[ ... ]
> +static struct dchid_iface *
> +dchid_get_interface(struct dchid_dev *dchid, int index, const char *name)
> +{
[ ... ]
> +	iface->fwnode = fwnode;
> +	ret = devm_add_action_or_reset(dchid->dev, dchid_fwnode_release,
> +				       iface->fwnode);
> +	if (ret)
> +		goto err_unlock;
> +
> +	dchid->ifaces[index] = iface;
> +	mutex_unlock(&dchid->ifaces_lock);
> +	return iface;

[Severity: High]
Does this code have a data race?

In dchid_get_interface(), dchid->ifaces[index] is published locklessly with
respect to the interrupt handler using a simple assignment. Since there is no
write memory barrier here, and no read memory barrier in dchid_process_packet()
where the pointer is loaded, could weakly-ordered architectures observe a
non-NULL pointer but read stale or uninitialized values for the struct's
contents (such as work->iface->wq)?

> +
> +err_put_fwnode:
> +	fwnode_handle_put(fwnode);
> +err_unlock:
> +	mutex_unlock(&dchid->ifaces_lock);
> +	return NULL;
> +}
> +
> +static int dchid_send(struct dchid_iface *iface, u32 flags, const void *msg,
> +		      size_t size)
> +{
[ ... ]
> +	memcpy(buf + sizeof(*hdr) + sizeof(*sub), msg, size);
> +
> +	*checksum_ptr = 0xffffffff - dchid_checksum(buf, total_len - DCHID_CHECKSUM_LEN);

[Severity: Low]
Does this write the checksum in host endianness?

The assignment through a u32* pointer writes the computed checksum in host
endianness. Could this break compatibility and cause the hardware to reject
packets on big-endian systems?

> +
> +	dc_msg.data = buf;
> +	dc_msg.len = total_len;
> +	ret = mbox_send_message(dchid->dc_mbox, &dc_msg);
> +	kfree(buf);
> +
> +	return ret < 0 ? ret : 0;
> +}
[ ... ]
> +static int dchid_set_report(struct dchid_iface *iface, void *buf, size_t len)
> +{
> +	return dchid_cmd(iface, HID_OUTPUT_REPORT, REQ_SET_REPORT, buf, len,
> +			 NULL, 0);
> +}

[Severity: Medium]
Does this return an incorrect value for SET_REPORT?

dchid_set_report() returns the value from dchid_cmd(), which is the size
of the acknowledgment packet payload received from the device. However, the
HID raw_request callback contract requires returning the number of bytes
successfully written to the device (i.e., len). Returning the ACK size
could break callers that verify the full payload was transferred.

> +
> +static int dchid_raw_request(struct hid_device *hdev, unsigned char reportnum,
> +			     __u8 *buf, size_t len, unsigned char rtype,
> +			     int reqtype)
> +{
> +	struct dchid_iface *iface = hdev->driver_data;
> +
> +	switch (reqtype) {
> +	case HID_REQ_GET_REPORT:
> +		if (len < 1)
> +			return -EINVAL;
> +
> +		buf[0] = reportnum;
> +		return dchid_cmd(iface, rtype, REQ_GET_REPORT, &reportnum, 1,
> +				 buf + 1, len - 1);

[Severity: High]
Does this return an oversized length to userspace, causing an out-of-bounds read?

dchid_cmd() returns the actual size of the device's response payload. If the
device's response is larger than the requested buffer size, raw_request()
will return a length greater than the buffer allocation. Could this cause
callers to copy uninitialized adjacent kernel heap memory to userspace?

> +	case HID_REQ_SET_REPORT:
> +		return dchid_set_report(iface, buf, len);
> +	default:
> +		return -EIO;
> +	}
> +}
[ ... ]
> +static void dchid_create_interface_work(struct work_struct *ws)
> +{
[ ... ]
> +	hid->dev.parent = iface->dchid->dev;
> +	hid->driver_data = iface;
> +	iface->hid = hid;
> +
> +	ret = hid_add_device(hid);

[Severity: High]
Can this result in a use-after-free on probe failure?

If apple_rtkit_wake() fails in dchid_probe(), it initiates devres cleanup. As
part of this cleanup, the workqueue is flushed. If coprocessor events triggered
this work, dchid_create_interface_work() will run and successfully register
the HID device here, because dchid->stopping is not set during probe failure.
When devres subsequently frees the driver's memory, the active HID device
will be left pointing to freed memory.

> +	if (ret < 0) {
> +		iface->hid = NULL;
> +		hid_destroy_device(hid);
> +		dev_warn(iface->dchid->dev, "failed to register HID device %s\n",
> +			 iface->name);
> +	}
> +
> +done:
> +	iface->creating = false;
> +}
[ ... ]
> +static void dchid_handle_descriptor(struct dchid_iface *iface, void *hid_desc,
> +				    size_t desc_len)
> +{
> +	u8 *rdesc;
> +	int i;
> +
> +	if (iface->hid)
> +		return;
> +
> +	rdesc = devm_kmemdup(iface->dchid->dev, hid_desc, desc_len,
> +			     GFP_KERNEL);

[Severity: Medium]
Does this leak memory if multiple INIT events are received?

There is no check to see if iface->hid_desc has already been allocated.
If a malfunctioning or malicious firmware sends multiple INIT_HID_DESCRIPTOR
events before HID device registration, previous allocations will be
overwritten and leaked until the device is unbound.

> +	if (!rdesc)
> +		return;
[ ... ]
> +static void dchid_handle_ready(struct dchid_dev *dchid, void *data, size_t length)
> +{
[ ... ]
> +	/* 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,
> +					   sizeof(dchid->device_id));
> +		if (ret < (int)sizeof(dchid->device_id)) {
> +			dev_warn(iface->dchid->dev, "failed to get device ID from STM\n");
> +			/* Fake it and keep going. Things might still work. */
> +			memset(&dchid->device_id, 0, sizeof(dchid->device_id));
> +		}
> +
> +		ret = dchid_get_report_cmd(iface, STM_REPORT_SERIAL, dchid->serial,
> +					   sizeof(dchid->serial) - 1);
> +		if (ret < 0) {
> +			dev_warn(iface->dchid->dev, "failed to get serial from STM\n");
> +			dchid->serial[0] = 0;
> +		}

[Severity: Low]
Should this be null-terminated on the success path?

When reading the serial string into dchid->serial, the driver does not append
a null terminator at the returned length. Since the buffer is initially allocated
with zeroed memory, the first read is safe, but if the coprocessor restarts
and resends an EVENT_READY with a shorter serial string, the buffer will
retain trailing characters from the older string.

> +
> +		dchid->id_ready = true;
> +		for (i = 0; i < MAX_INTERFACES; i++) {
> +			if (!dchid->ifaces[i] || !dchid->ifaces[i]->deferred)
> +				continue;
> +			dchid_create_interface(dchid->ifaces[i]);
> +		}
> +	}
> +}
[ ... ]
> +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: High]
Does this cause an out-of-bounds read if the report is smaller than expected?

According to the HID subsystem guidelines, transport drivers must use
hid_safe_input_report() because hid_input_report() assumes the buffer size
matches the descriptor's expected size. If the coprocessor provides a report
that is smaller than expected, hid_input_report() will blindly read past the
end of the dynamically allocated buffer.

> +}
[ ... ]
> +static void dchid_handle_ack(struct dchid_iface *iface, struct dchid_hdr *hdr,
> +			     void *data)
> +{
> +	struct dchid_subhdr *shdr = data;
> +	u8 *payload = data + sizeof(*shdr);
> +	u16 hdr_len = le16_to_cpu(hdr->length);
> +	u16 sub_len = le16_to_cpu(shdr->length);
> +	unsigned long flags;
> +	bool complete_cmd = false;
> +
> +	if (hdr_len < sizeof(*shdr) || sub_len > hdr_len - sizeof(*shdr) ||
> +	    sub_len < 1)
> +		return;
> +
> +	spin_lock_irqsave(&iface->out_lock, flags);
> +
> +	if (shdr->flags == iface->out_flags && iface->tx_seq == hdr->seq &&
> +	    iface->out_report == payload[0]) {
> +		if (iface->resp_buf && iface->resp_size)
> +			memcpy(iface->resp_buf, payload + 1,
> +			       min_t(size_t, sub_len - 1, iface->resp_size));
> +
> +		iface->resp_size = sub_len;

[Severity: High]
Does this update contribute to the oversized length returned by raw_request?

This updates the tracked response size to the received length (sub_len),
which is then returned by dchid_cmd() to dchid_raw_request(). This can exceed
the caller's originally requested buffer size, which dchid_raw_request()
then returns to userspace.

> +		iface->out_report = -1;
> +		iface->retcode = le32_to_cpu(shdr->retcode);
> +		complete_cmd = true;
> +	}
> +
> +	spin_unlock_irqrestore(&iface->out_lock, flags);
> +
> +	if (complete_cmd)
> +		complete(&iface->out_complete);
> +}
[ ... ]
> +static void dchid_process_packet(struct dchid_dev *dchid, struct dchid_hdr *hdr,
> +				 u8 *payload, size_t payload_len, u8 *packet,
> +				 size_t packet_len)
> +{
> +	struct dchid_work *work;
> +	unsigned long flags;
> +
> +	if (dchid_checksum(packet, packet_len) != DCHID_CHECKSUM_SEED) {
> +		dev_err_ratelimited(dchid->dev, "checksum error\n");
> +		return;
> +	}
> +
> +	if (payload_len < sizeof(struct dchid_subhdr))
> +		return;
> +
> +	if (hdr->iface >= MAX_INTERFACES || !dchid->ifaces[hdr->iface])
> +		return;
> +
> +	if (hdr->channel == DCHID_CHANNEL_CMD) {
> +		dchid_handle_ack(dchid->ifaces[hdr->iface], hdr, payload);
> +		return;
> +	}
> +
> +	if (hdr->channel != DCHID_CHANNEL_REPORT)
> +		return;
> +
> +	work = kzalloc(sizeof(*work) + payload_len, GFP_ATOMIC);
> +	if (!work)
> +		return;
> +
> +	work->hdr = *hdr;
> +	work->iface = dchid->ifaces[hdr->iface];

[Severity: High]
Is this missing a read memory barrier?

This pointer is loaded and dereferenced locklessly. As mentioned earlier,
without a read memory barrier, this could observe uninitialized memory if
it races with interface creation in dchid_get_interface().

> +	memcpy(work->data, payload, payload_len);
> +	INIT_WORK(&work->work, dchid_packet_work);
> +
> +	spin_lock_irqsave(&dchid->rx_lock, flags);
> +	if (dchid->stopping)
> +		kfree(work);
> +	else
> +		queue_work(work->iface->wq, &work->work);
> +	spin_unlock_irqrestore(&dchid->rx_lock, flags);
> +}
[ ... ]
> +static int dchid_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	writel_relaxed(APPLE_ASC_CPU_CONTROL_RUN,
> +		       dchid->asc_base + APPLE_ASC_CPU_CONTROL);
> +
> +	ret = apple_rtkit_wake(dchid->rtk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to wake coprocessor\n");

[Severity: High]
Does this leak the running ASC coprocessor on failure?

The driver starts the ASC CPU by writing APPLE_ASC_CPU_CONTROL_RUN to its
control register. If apple_rtkit_wake() fails, the driver returns an error
without clearing the control register. Devres will unmap the memory and
free resources, but the CPU remains physically running and may continue to
access unmapped or freed memory.

> +
> +	return 0;
> +}
[ ... ]

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