Re: [PATCH 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] Flawed teardown ordering in dchid_remove leads to Use-After-Free of HID devices and transport channel.
- [High] dchid_raw_request returns an unvalidated length from the device, leading to a heap out-of-bounds read in hidraw.
- [High] Missing null-termination enforcement on hardware-provided interface name leads to an out-of-bounds read.
- [High] Concurrency flaws in dchid_cmd lead to kernel log spam and state corruption.
- [High] Unbounded devres memory allocation in dchid_handle_descriptor allows device to exhaust kernel memory.
- [High] Missing memory barriers when publishing and reading dchid->ifaces array causes a potential crash on weakly ordered architectures.
- [Medium] dchid_raw_request returns incorrect length for HID_REQ_SET_REPORT, causing false short write errors.
- [Medium] Missing cleanup for the ASC coprocessor and RTKit on probe error paths.
- [Low] Native pointer assignment writes the checksum in host byte order, causing failures on Big-Endian systems.
--

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

HID: apple: Add DockChannel HID transport driver

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

> diff --git a/drivers/hid/dockchannel/apple-hid.c b/drivers/hid/dockchannel/apple-hid.c
> index 0000000000000..162fcfb5ab1ce
> --- /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->index = index;
> +	iface->name = devm_kstrdup(dchid->dev, name, GFP_KERNEL);

[Severity: High]
Can this cause an out-of-bounds read if the hardware sends a name that is
exactly 16 characters and not null-terminated? In dchid_handle_init(), the
device provides a fixed 16-byte character array for hdr->name. If it lacks a
null terminator, devm_kstrdup() will read past the end of the array into
adjacent fields. Should this use devm_kstrndup(..., 16, ...) instead?

[ ... ]

> +	dchid->ifaces[index] = iface;
> +	mutex_unlock(&dchid->ifaces_lock);
> +	return iface;
> +}

[Severity: High]
Does the absence of a memory barrier here allow the reader to observe an
uninitialized interface? The interface is published to the array here and
the mutex is unlocked, but dchid_process_packet() reads this array locklessly.
The mutex_unlock() does not prevent the compiler or CPU from reordering the
initialization of the structure (like iface->wq) to after the pointer becomes
visible. Should smp_store_release() be used when publishing to the array?

[ ... ]

> +static int dchid_send(struct dchid_iface *iface, u32 flags, const void *msg,
> +		      size_t size)
> +{

[ ... ]

> +	*checksum_ptr = 0xffffffff - dchid_checksum(buf, total_len - DCHID_CHECKSUM_LEN);

[Severity: Low]
Does this write the checksum in the CPU's native byte order rather than
the expected little-endian format? Since other fields like hdr->length use
__le16, it appears the hardware expects little-endian, which could cause
packets to be dropped on big-endian architectures. Should this use
put_unaligned_le32() instead?

[ ... ]

> +static int dchid_cmd(struct dchid_iface *iface, u32 type, u32 req,
> +		     void *data, size_t size, void *resp_buf, size_t resp_size)
> +{

[ ... ]

> +	/* 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: High]
Can userspace easily trigger this WARN_ON and spam the kernel log by issuing
concurrent hidraw ioctls? Furthermore, is there a race window here? In
dchid_handle_ack(), iface->out_report is cleared before the waiting thread
completes its cleanup. This could allow a second concurrent thread to bypass
this check and set up its own command, only to have its state clobbered when
the first thread finally wakes up.

[ ... ]

> +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 returning the result of dchid_cmd() here cause false short-write errors
for userspace? The HID core expects the raw_request function for a SET_REPORT
call to return the number of bytes written (the requested length). However,
dchid_cmd() returns the length of the device's ACK payload, which is typically
just 1 byte.

[ ... ]

> +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]
Could this return an unvalidated length from the device and lead to an
out-of-bounds heap read? In dchid_handle_ack(), iface->resp_size is
overwritten with the hardware-supplied sub_len. If the device sends a
response larger than the requested length, dchid_cmd() returns that larger
value, and hidraw_ioctl() will use it as the count for copy_to_user(),
reading past the end of the allocated buffer.

[ ... ]

> +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);
> +	if (!rdesc)
> +		return;

[Severity: High]
Can a misbehaving device exhaust kernel memory by flooding the host with
descriptor packets? The devm_kmemdup() allocation is performed without checking
if iface->hid_desc is already allocated or freeing the previous pointer. Since
the check for iface->hid only bypasses this once asynchronous registration
completes, a rapid stream of packets before that point could leak memory.

[ ... ]

> +static void dchid_handle_ack(struct dchid_iface *iface, struct dchid_hdr *hdr,
> +			     void *data)
> +{

[ ... ]

> +	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]
Is it safe to unconditionally overwrite iface->resp_size with the unvalidated
sub_len from the device here? This causes the calling function to believe more
bytes were read than actually requested, setting up the info leak mentioned
earlier in dchid_raw_request().

[ ... ]

> +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;
> +
> +	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;

[Severity: High]
Does reading the dchid->ifaces array locklessly in the RX interrupt context
require an acquire barrier? Without READ_ONCE() and corresponding release
semantics on the writer side, a weakly ordered architecture could see a
non-NULL pointer but read uninitialized memory for fields like the workqueue,
leading to a crash.

[ ... ]

> +static int dchid_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	dchid->new_iface_wq = alloc_ordered_workqueue("dchid-new", 0);
> +	if (!dchid->new_iface_wq)
> +		return dev_err_probe(dev, -ENOMEM, "failed to allocate workqueue\n");

[Severity: Medium]
Will this error path leave the ASC coprocessor running and RTKit awake? If
probe fails here after the hardware is started, it returns without calling
apple_rtkit_quiesce() or writing 0 to the control register, which could leak
resources or drain power.

[ ... ]

> +static void dchid_remove(struct platform_device *pdev)
> +{
> +	struct dchid_dev *dchid = platform_get_drvdata(pdev);
> +	int i;
> +
> +	if (dchid->dc_mbox) {
> +		devm_release_action(&pdev->dev, dchid_free_mbox, dchid->dc_mbox);
> +		dchid->dc_mbox = NULL;
> +	}

[Severity: High]
Does freeing the mailbox channel early here introduce a use-after-free risk?
The HID devices and workqueues are still active at this point. If a userspace
process continues to access the HID device concurrently, or if a pending work
item executes and calls dchid_send(), they will attempt to dereference the
freed mailbox channel. Should the teardown order be reversed to destroy
consumers first?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-apple-mtp-keyboard-final-v1-0-506d936a1707@gmail.com?part=8