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

[sashiko-bot@kernel.org]

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 <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/Kconfig b/drivers/input/joystick/gip/Kconfig
> --- a/drivers/input/joystick/gip/Kconfig
> +++ b/drivers/input/joystick/gip/Kconfig
> @@ -5,6 +5,7 @@
>  config JOYSTICK_XBOX_GIP
>  	tristate "Xbox One/Series controller support"
>  	depends on USB && USB_ARCH_HAS_HCD && LEDS_CLASS_MULTICOLOR
> +	depends on CRYPTO_ECDH && CRYPTO_LIB_SHA256 && CRYPTO_RSA

[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?

[ ... ]

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -1932,7 +1922,6 @@ static void gip_reset_attachment(struct gip_attachment *attachment)
>  	}
>  
>  	gip_free_devices(attachment);
> -
>  }

[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.

[ ... ]

> diff --git a/drivers/input/joystick/gip/gip-security.c b/drivers/input/joystick/gip/gip-security.c
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-security.c
[ ... ]
> +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