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

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 [off-list ref]

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.

[ ... ]

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.

[ ... ]

+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