Re: [PATCH v7 07/12] Input: xbox_gip - Add security implementation from xone

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Use-After-Free during device disconnect due to asynchronous cancellation of security workqueues.
- [High] Dynamic `INIT_WORK()` calls in `gip_security_start_handshake()` can corrupt workqueue state if re-entered.
- [High] Data race on shared security state (`transcript`, `random_client`, `pubkey_client`) due to lack of synchronization.
- [High] Heap variables embedded directly in a dynamically allocated struct are passed to scatterlists, risking DMA memory corruption.
- [Medium] Transcript hash incorrectly includes the message trailer, causing a protocol mismatch.
- [Medium] Sensitive session key is left on the kernel stack without explicit zeroization.

Pre-existing issues:
- [High] Index typo in `gip_disconnect()` causes out_fragments delayed works to not be cancelled, leading to a Use-After-Free.
--

commit 90b08fdfc4b71ac21c6c33cfb71aa69cf4b03d4c
Author: Vicki Pfau <vi@endrift.com>

Input: xbox_gip - Add security implementation from xone

This patch imports and adapts the security message implementation from the
out-of-tree xone driver. This message is necessary for most controllers
that use wireless dongles.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 1bfb559f5f7a2..ec21740bba50c 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

This is a pre-existing issue, but I noticed a potential use-after-free bug
in the disconnect flow.

Looking at gip_disconnect() in drivers/input/joystick/gip/gip-core.c:

	mutex_lock(&gip->attachment_lock);
	for (i = 0; i < MAX_ATTACHMENTS; i++) {
		struct gip_attachment *attachment = gip->attachments[i];

		if (!attachment)
			continue;

		for (j = 0; j < MAX_OUT_FRAGMENTS; j++)
			cancel_delayed_work_sync(&attachment->out_fragments[i].timeout);
		devm_kfree(to_gip_device(attachment), attachment->uniq);
		devm_kfree(to_gip_device(attachment), attachment);
		gip->attachments[i] = NULL;
	}
	mutex_unlock(&gip->attachment_lock);

[Severity: High]
Does this code use the outer loop index `i` (attachment index) instead of the
inner loop index `j` (fragment index)?

If `i` is used, timeouts for fragments other than the one at index `i` are
never cancelled. The function then proceeds to free the `attachment` structure.
If any of the un-cancelled delayed works execute later, they will access the
freed `attachment`, leading to a use-after-free.

> diff --git a/drivers/input/joystick/gip/gip-security.c b/drivers/input/joystick/gip/gip-security.c
> new file mode 100644
> index 0000000000000..1850850b60e67
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-security.c
> @@ -0,0 +1,810 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

[ ... ]

> +static int gip_security_ecdh_get_secret(struct crypto_kpp *tfm, u8 *pubkey,
> +	int pubkey_len, u8 *secret, int secret_len)
> +{
> +	struct kpp_request *req;
> +	struct scatterlist src, dest;
> +	DECLARE_CRYPTO_WAIT(wait);
> +	int err;
> +
> +	req = kpp_request_alloc(tfm, GFP_KERNEL);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	sg_init_one(&src, pubkey, pubkey_len);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can passing these embedded, unaligned struct fields directly to
`sg_init_one()` cause DMA memory corruption?

The fields `security->pubkey_client2` and `security->pms` are embedded directly
inside `struct gip_security`, which itself is embedded in
`struct gip_attachment`. If the underlying crypto backend uses hardware DMA,
the DMA transfer will share a cacheline with adjacent fields in the
`gip_attachment` structure. A simultaneous CPU access to these adjacent fields
while the DMA is ongoing can corrupt either the DMA payload or the adjacent
struct fields.

[ ... ]

> +static void gip_security_complete_handshake(struct work_struct *work)
> +{
> +	struct gip_security *security = container_of(work, typeof(*security), work_complete);
> +	struct gip_attachment *attachment = container_of(security, struct gip_attachment, security);
> +	struct gip_security_header_control hdr = {
> +		.context = GIP_SECURITY_CTX_CONTROL,
> +		.control = GIP_SECURITY_CTRL_COMPLETE,
> +	};
> +	u8 random[GIP_SECURITY_RANDOM_LEN * 2];
> +	u8 key[GIP_SECURITY_SESSION_KEY_LEN];
> +	int err;
> +
> +	guard(mutex)(&attachment->lock);
> +	if (security->status != GIP_SECURITY_SENT)
> +		return;
> +
> +	memcpy(random, security->random_host, sizeof(security->random_host));
> +	memcpy(random + sizeof(security->random_host), security->random_client,
> +		sizeof(security->random_client));
> +
> +	gip_security_compute_prf(
> +		"EXPORTER DAWN data channel session key for controller",
> +		security->master_secret, sizeof(security->master_secret),
> +		random, sizeof(random), key, sizeof(key));
> +
> +	gip_dbg(security, "%s: key=%*phD\n", __func__, (int)sizeof(key), key);
> +
> +	err = gip_send_system_message(attachment, GIP_CMD_SECURITY,
> +		0, &hdr, sizeof(hdr));
> +	if (err) {
> +		gip_err(security, "%s: send complete failed: %d\n", __func__, err);
> +		return;
> +	}
> +
> +	security->status = GIP_SECURITY_DONE;
> +}
    ^^^

[Severity: Medium]
Should `memzero_explicit()` be used to wipe the sensitive session key
material from the stack?

The symmetric session key `key` and the pre-master secret/random data `random`
are generated into local stack arrays. After sending the completion message,
the function returns without zeroing out this sensitive cryptographic material,
which could be exposed if a subsequent kernel vulnerability allows reading
uninitialized stack memory.

[ ... ]

> +int gip_security_handle_message(struct gip_security *security, const void *bytes, int num_bytes)
> +{
> +	const struct gip_security_header_handshake *handshake = bytes;
> +	const struct gip_security_header_full *hdr;
> +	int err;
> +
> +	if (num_bytes < sizeof(*handshake))
> +		return -EINVAL;
> +
> +	if (security->status != GIP_SECURITY_SENT) {
> +		gip_err(security, "%s: security message received when none expected: 0x%02x\n",
> +			__func__, handshake->command);
> +		return -EPROTO;
> +	}
> +
> +	if (handshake->error)
> +		return -EPROTO;
> +
> +	if (handshake->options & GIP_SECURITY_OPT_ACKNOWLEDGE) {
> +		if (handshake->command == 0x01)
> +			return gip_security_handle_acknowledge(security);
> +
> +		gip_err(security, "%s: handshake failed: 0x%02x\n",
> +			__func__, handshake->command);
> +		return -EPROTO;
> +	}
> +
> +	if (num_bytes < sizeof(*hdr))
> +		return -EINVAL;
> +
> +	hdr = bytes;
> +	/* client uses v2 */
> +	if (num_bytes < sizeof(*hdr) || handshake->command != hdr->data.command) {
> +		/* reset transcript hash and restart handshake */
> +		gip_dbg(security, "%s: protocol upgrade\n", __func__);
> +		sha256_init(&security->transcript);
> +		return gip_security2_send_hello(security);
> +	}
> +
> +	err = gip_security_dispatch_message(security, hdr->data.command,
> +		bytes + sizeof(*hdr), num_bytes - sizeof(*hdr));
> +	if (err)
> +		return err;
> +
> +	sha256_update(&security->transcript, bytes + sizeof(hdr->handshake),
> +		num_bytes - sizeof(hdr->handshake));
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Is there a data race on the shared security state (`transcript`,
`random_client`, `pubkey_client`) due to lack of synchronization?

Incoming messages are processed by `gip_receive_work`, which calls
`gip_security_handle_message()` without acquiring `attachment->lock`.
This function modifies shared state such as `security->transcript` and
`security->pubkey_client`.

Concurrently, asynchronous work items (like `gip_security_exchange_rsa`)
acquire `attachment->lock` and call `gip_security_send_message()`, which also
updates the `transcript` hash. Since one side does not hold the lock, these
threads can race and corrupt the internal state of the SHA256 context or
copy torn data.

[Severity: Medium]
Does this transcript hash incorrectly include the message trailer?

The transcript hash is updated using `num_bytes - sizeof(hdr->handshake)`.
If the received packet includes the 8-byte trailer, `num_bytes` encompasses it,
and the trailer gets hashed. However, the sender (`gip_security_send_message()`)
specifically excludes the trailer from the hash. This discrepancy causes a
transcript mismatch during the `Finished` step.

> +
> +	return 0;
> +}
> +
> +void gip_security_release(struct gip_security *security)
> +{
> +	if (security->status < GIP_SECURITY_SENT)
> +		return;
> +
> +	cancel_work(&security->work_exchange_rsa);
> +	cancel_work(&security->work_exchange_ecdh);
> +	cancel_work(&security->work_complete);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Is it safe to use the non-blocking `cancel_work()` instead of
`cancel_work_sync()` here?

During device disconnect, `gip_disconnect()` calls `gip_shutdown()` which calls
`gip_security_release()`. This uses the non-blocking `cancel_work()` to cancel
the security work items.

`gip_disconnect()` then explicitly frees the `attachment` structure using
`devm_kfree()`. If a security work item is actively executing on another CPU,
it will continue running and attempt to acquire the freed `attachment->lock` or
access other freed fields (such as inside `gip_security_exchange_rsa()`),
resulting in a use-after-free.

> +
> +	memzero_explicit(&security->transcript, sizeof(security->transcript));
> +	memzero_explicit(&security->pms, sizeof(security->pms));
> +	memzero_explicit(&security->master_secret, sizeof(security->master_secret));
> +
> +	security->status = GIP_SECURITY_PENDING;
> +}
> +
> +int gip_security_start_handshake(struct gip_security *security)
> +{
> +	int err;
> +
> +	sha256_init(&security->transcript);
> +
> +	INIT_WORK(&security->work_exchange_rsa, gip_security_exchange_rsa);
> +	INIT_WORK(&security->work_exchange_ecdh, gip_security2_exchange_ecdh);
> +	INIT_WORK(&security->work_complete, gip_security_complete_handshake);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can dynamically re-initializing these work items corrupt the workqueue state
if they are re-entered?

`INIT_WORK()` is dynamically invoked every time `gip_security_start_handshake()`
runs. If the handshake fails or is reset (e.g., via `HELLO_DEVICE`),
`gip_security_release()` cancels the work asynchronously.

A subsequent `STATUS_DEVICE` message will call `gip_security_start_handshake()`
again. If the previous work item is still executing (because `cancel_work()` is
asynchronous), calling `INIT_WORK()` will overwrite its internal
`work_data_bits` and list pointers, leading to workqueue corruption.

> +
> +	err = gip_security_send_hello(security);
> +	if (err < 0)
> +		return err;
> +
> +	security->status = GIP_SECURITY_SENT;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912030426.2997003-1-vi@endrift.com?part=7