Re: [PATCH v5 6/8] HID: apple: Add DockChannel HID transport driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: dockchannel: fix STM device_id race in
  dchid_create_interface_work()
- [High] HID: dockchannel: bound TX size and timeout in dchid_send() and
  dchid_probe()
- [Medium] HID: dockchannel: validate report descriptor and iface in
  dchid_handle_init()
--

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

HID: apple: Add DockChannel HID transport driver

This commit adds a HID transport driver for Apple MTP keyboards and
trackpads connected over the DockChannel mailbox and RTKit.

> diff --git a/drivers/hid/dockchannel/apple-hid.c b/drivers/hid/dockchannel/apple-hid.c
> new file mode 100644
> index 0000000000000..3974b877236b5
> --- /dev/null
> +++ b/drivers/hid/dockchannel/apple-hid.c
[ ... ]
> +static int dchid_send(struct dchid_iface *iface, u32 flags, const void *msg,
> +		      size_t size)
> +{
> +	struct dchid_dev *dchid = iface->dchid;
> +	size_t payload_padded = round_up(size, sizeof(u32));
> +	size_t total_len = sizeof(struct dchid_hdr) + sizeof(struct dchid_subhdr) +
> +			   payload_padded + DCHID_CHECKSUM_LEN;
> +	struct apple_dockchannel_msg dc_msg;
> +	struct dchid_hdr *hdr;
> +	struct dchid_subhdr *sub;
> +	__le32 *checksum_ptr;
> +	u8 *buf;
> +	int ret;
> +
> +	if (total_len > DCHID_RX_BUF_SIZE)
> +		return -EINVAL;
> +
> +	buf = kzalloc(total_len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	hdr = (struct dchid_hdr *)buf;
> +	sub = (struct dchid_subhdr *)(buf + sizeof(*hdr));
> +	checksum_ptr = (__le32 *)(buf + total_len - DCHID_CHECKSUM_LEN);
> +
> +	hdr->hdr_len = sizeof(*hdr);
> +	hdr->channel = DCHID_CHANNEL_CMD;
> +	hdr->length = cpu_to_le16(payload_padded + sizeof(*sub));
> +	hdr->seq = iface->tx_seq;
> +	hdr->iface = iface->index;
> +
> +	sub->flags = (u8)flags;
> +	sub->length = cpu_to_le16(size);
> +
> +	memcpy(buf + sizeof(*hdr) + sizeof(*sub), msg, size);
> +
> +	*checksum_ptr = cpu_to_le32(DCHID_CHECKSUM_SEED -
> +				   dchid_checksum(buf, total_len - DCHID_CHECKSUM_LEN));
> +
> +	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;
> +}

[Severity: High]
Can a SET_REPORT larger than 2048 bytes wedge dchid->dc_mbox for an hour
and leave a dangling stack pointer queued in the mailbox channel?

dchid_send() checks total_len against DCHID_RX_BUF_SIZE (65547), while
dchid_ll leaves max_buffer_size unset (allowing raw output reports up to
HID_MAX_BUFFER_SIZE, 16384 bytes).  However, apple_dockchannel_send_data()
rejects any message larger than APPLE_DOCKCHANNEL_FIFO_SIZE (2048 bytes)
with -EMSGSIZE.

When msg_submit() fails in mbox_send_message(), the stack pointer &dc_msg
has already been added to chan->msg_data[] without setting
chan->active_req.  Because dchid_probe() sets dc_mbox_client.tx_block =
true while leaving tx_tout as 0, mbox_send_message() defaults to a
3600000 ms (1 hour) wait on chan->tx_complete while holding
dchid->cmd_lock.  Once that wait times out, dchid_send() frees buf and
returns while &dc_msg remains in chan->msg_data[], causing a stack and
heap use-after-free on later mailbox submissions.

Bounding total_len to APPLE_DOCKCHANNEL_FIFO_SIZE and setting a reasonable
dc_mbox_client.tx_tout in dchid_probe() would avoid this.

[ ... ]
> +static void dchid_create_interface_work(struct work_struct *ws)
> +{
> +	struct dchid_iface *iface = container_of(ws, struct dchid_iface, create_work);
> +	struct dchid_dev *dchid = iface->dchid;
> +	struct hid_device *hid;
> +	char cap_name[16];
> +	int ret;
> +
> +	if (READ_ONCE(dchid->stopping))
> +		goto done;
> +
> +	if (iface->hid) {
> +		dev_warn(dchid->dev, "interface %s already created\n", iface->name);
> +		goto done;
> +	}
> +
> +	ret = dchid_enable_interface(iface);
> +	if (ret < 0) {
> +		dev_warn(dchid->dev, "failed to enable %s: %d\n", iface->name, ret);
> +		goto done;
> +	}
> +
> +	iface->deferred = false;
> +
> +	hid = hid_allocate_device();
> +	if (IS_ERR(hid))
> +		goto done;
> +
> +	strscpy(cap_name, iface->name, sizeof(cap_name));
> +	if (cap_name[0])
> +		cap_name[0] = toupper(cap_name[0]);
> +	snprintf(hid->name, sizeof(hid->name), "Apple DockChannel %s", cap_name);
> +
> +	snprintf(hid->phys, sizeof(hid->phys), "%s.%d", dev_name(dchid->dev),
> +		 iface->index);
> +	strscpy(hid->uniq, dchid->serial, sizeof(hid->uniq));
> +
> +	hid->ll_driver = &dchid_ll;
> +	hid->bus = BUS_HOST;
> +	hid->vendor = le16_to_cpu(dchid->device_id.vendor_id);
> +	hid->product = le16_to_cpu(dchid->device_id.product_id);
> +	hid->version = le16_to_cpu(dchid->device_id.version_number);
[ ... ]
> +	ret = hid_add_device(hid);

[Severity: High]
On machines with apple,has-stm, doesn't dchid_create_interface_work()
register the stm HID device before or concurrently while
dchid_handle_ready() fetches dchid->device_id and dchid->serial?

dchid_handle_init() queues create_work for stm before dchid->id_ready is
set.  When CMD_ENABLE_INTERFACE completes, dchid_create_interface_work()
does not wait for EVENT_READY on stm, so it races with dchid_handle_ready()
running on dchid->comm->wq:

kworker (dchid->new_iface_wq)     kworker (dchid->comm->wq)
dchid_create_interface_work()
  dchid_enable_interface(stm)
                                  dchid_handle_ready()
                                    dchid_get_report_cmd(...,
                                      &dchid->device_id, ...)
                                    dchid_get_report_cmd(...,
                                      dchid->serial, ...)
  strscpy(hid->uniq,
          dchid->serial, ...)  <-- data race / uninitialized
  hid->vendor = ...            <-- data race / zero IDs
  hid_add_device(hid)
    ...
      dchid_open()
        !completion_done(&stm->ready)
        dchid_start_interface(stm)
          dchid_reset_interface() <-- resets stm mid-init
                                    dchid->serial[ret] = 0;
                                    dchid->id_ready = true;
                                    complete_all(&stm->ready);

As a result, the stm HID device is registered with zero or torn device IDs
and an uninitialized or unterminated serial string, and opening it before
dchid_handle_ready() reaches complete_all(&iface->ready) sends
CMD_RESET_INTERFACE while STM reports are being queried (or triggers a
second EVENT_READY that overwrites dchid->device_id and dchid->serial
while deferred interfaces are reading them).

[ ... ]
> +static void dchid_handle_descriptor(struct dchid_iface *iface, void *hid_desc,
> +				    size_t desc_len)
> +{
> +	u8 *rdesc;
> +	int i;
> +
> +	/* Keep the descriptor stable while creation and HID parsing run. */
> +	if (iface->hid_desc)
> +		return;
> +
> +	rdesc = devm_kmemdup(iface->dchid->dev, hid_desc, desc_len,
> +			     GFP_KERNEL);
> +	if (!rdesc)
> +		return;
[ ... ]
> +	iface->hid_desc = rdesc;
> +	iface->hid_desc_len = desc_len;
> +}
[ ... ]
> +static void dchid_handle_init(struct dchid_dev *dchid, void *data, size_t length)
> +{
> +	struct dchid_init_hdr *hdr = data;
> +	struct dchid_init_block_hdr *blk;
> +	struct dchid_iface *iface;
> +	u8 *p = data;
> +
> +	if (length < sizeof(*hdr))
> +		return;
> +
> +	if (!memchr(hdr->name, 0, sizeof(hdr->name)))
> +		return;
> +
> +	iface = dchid_get_interface(dchid, hdr->iface, hdr->name);
> +	if (!iface)
> +		return;
[ ... ]
> +	if (hdr->more_packets)
> +		return;
> +
> +	/* Wait for the shared device IDs only on machines with an STM. */
> +	if (dchid->id_ready || !strcmp(iface->name, "stm"))
> +		dchid_create_interface(iface);
> +	else
> +		iface->deferred = true;
> +}

[Severity: Medium]
What happens if devm_kmemdup() fails in dchid_handle_descriptor(), or if
an EVENT_INIT packet has no non-empty INIT_HID_DESCRIPTOR block?

dchid_handle_descriptor() returns without recording an error, and
dchid_handle_init() still queues dchid_create_interface(iface).
dchid_parse() then passes a NULL or zero-length iface->hid_desc to
hid_parse_report(), where kmemdup(NULL, 0, GFP_KERNEL) returns
ZERO_SIZE_PTR and registers a broken HID device with a zero-length report
descriptor.

Also, dchid_get_interface() accepts the name "comm" and returns any
existing dchid->ifaces[index] without checking that hdr->iface !=
IFACE_COMM or that the existing interface name matches hdr->name, which
would allow an EVENT_INIT for index 0 to queue HID device creation on
dchid->comm.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-b4-apple-mtp-keyboard-v5-0-e2c97b82669a@gmail.com?part=6