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

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

New issues:
- [High] Asynchronous cancellation of security workers leads to Use-After-Free on disconnect. A naive synchronous fix would cause a deadlock.
- [High] The security transcript incorrectly hashes USB transport padding for unfragmented messages, causing handshake failures.
- [Medium] Unselectable Kconfig dependency on hidden symbol CRYPTO_LIB_SHA256.
- [Medium] Out-of-bounds read when processing unexpectedly short security messages.
- [Medium] Sensitive cryptographic secrets are not cleared from stack and heap memory.

Pre-existing issues:
- [High] Preexisting Use-After-Free in out_fragment timeout handlers due to missing synchronous cancellation.
--

commit e5c6e1be1aac0ee61bba5f91bea05c7f0f89796f
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.

[Severity: Medium]
Is depends on CRYPTO_LIB_SHA256 intentional here?

CRYPTO_LIB_SHA256 is a hidden tristate symbol without a prompt, meaning users
won't be able to select JOYSTICK_XBOX_GIP via menuconfig unless another
driver accidentally selects CRYPTO_LIB_SHA256 first.

Should this use select CRYPTO_LIB_SHA256 instead?

[ ... ]

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
involving out_fragments during shutdown.

When the controller is unplugged, the teardown path calls
gip_reset_attachment(), which loops through out_fragments and calls
gip_free_fragment(). However, gip_free_fragment() uses the non-blocking
cancel_delayed_work().

If the out_fragment timeout worker is running, it will block on
guard(mutex)(&attachment->lock). After the disconnect path finishes and
frees the attachment, the worker could unblock and access the freed memory.

[ ... ]

[ ... ]

+static void gip_security2_exchange_ecdh(struct work_struct *work)
+{
+	struct gip_security *security = container_of(work, typeof(*security), work_exchange_ecdh);
+	struct gip_attachment *attachment = container_of(security, struct gip_attachment, security);
+	struct gip_security2_host_pubkey message = {};
+	u8 random[GIP_SECURITY_RANDOM_LEN * 2];
+	u8 secret[GIP_SECURITY2_SECRET_LEN];

[Severity: Medium]
Does this leave the derived hashed ECDH shared secret on the stack?

The 32-byte secret is allocated here but never explicitly cleared with
memzero_explicit() before the function returns.

+	int err;
+
+	guard(mutex)(&attachment->lock);

[Severity: High]
Can this lock acquisition cause a use-after-free if the controller is
disconnected during a handshake?

When gip_disconnect() triggers shutdown, it acquires attachment->lock and
calls gip_security_release(), which uses the non-blocking cancel_work() on
these security workers.

If this worker is running, it blocks here. After the teardown path completes
and frees the attachment via devm_kfree(), this worker would unblock and
access the freed memory. (Note that simply switching to cancel_work_sync() in
gip_security_release() while holding the mutex might deadlock).

[ ... ]

+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 (security->status != GIP_SECURITY_SENT) {
+		gip_err(security, "%s: security message received when none expected: 0x%02x\n",
+			__func__, handshake->command);

[Severity: Medium]
Could this dereference handshake->command out-of-bounds?

We haven't verified that the payload length num_bytes is at least
sizeof(*handshake) before trying to read the command field for this error
message.

A malformed USB message might cause an out-of-bounds read and information
disclosure in the kernel log.

+		return -EPROTO;
+	}
+
+	if (num_bytes < sizeof(*handshake))
+		return -EINVAL;
+

[ ... ]

+	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]
Does the transcript hash include trailing padding bytes?

The core receive path in gip_receive_message() passes num_bytes (the full
USB packet length minus header offset) to gip_handle_message() without
truncating it to header.length.

If the USB host controller pads the interrupt transfer packet, these trailing
padding bytes will be included in the sha256_update() call here, causing the
handshake to fail.

[ ... ]

+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);
+
+	memzero_explicit(&security->transcript, sizeof(security->transcript));

[Severity: Medium]
While the transcript is explicitly cleared here, should we also zero out the
long-term session keys (security->pms and security->master_secret)?

Leaving these on the heap could lead to a leak of cryptographic secrets if
combined with an out-of-bounds read vulnerability.

+
+	security->status = GIP_SECURITY_PENDING;
+}

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