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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Stack-allocated buffers are used for cryptographic scatterlists, violating DMA constraints and causing kernel panics on architectures with virtually mapped stacks.
- [High] An ABBA deadlock occurs between `attachment->lock` and `cancel_work_sync()` during device teardown or reset.
- [Medium] The driver utilizes CRYPTO_LIB_SHA256 and CRYPTO_RSA but lacks the required Kconfig declarations, which can lead to build and runtime failures.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but incorrect teardown order in `gip_disconnect()` allows the URB completion handler to re-queue a work item after it was cancelled, leading to a Use-After-Free.
--

commit 0028f224002448741826a03077075da976d732d6
Author: Vicki Pfau <vi@endrift.com>
Subject: 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. The implementation hooks into the xbox_gip driver to
handle security commands.

> diff --git a/drivers/input/joystick/gip/Kconfig b/drivers/input/joystick/gip/Kconfig
> index 3422bf84cb895..b933583d325f7 100644
> --- a/drivers/input/joystick/gip/Kconfig
> +++ b/drivers/input/joystick/gip/Kconfig
> @@ -4,7 +4,7 @@
>  #
>  config JOYSTICK_XBOX_GIP
>  	tristate "Xbox One/Series controller support"
> -	depends on USB && USB_ARCH_HAS_HCD && LEDS_CLASS_MULTICOLOR
> +	depends on USB && USB_ARCH_HAS_HCD && LEDS_CLASS_MULTICOLOR && CRYPTO_ECDH

[Severity: Medium]
This patch introduces a dependency on CRYPTO_LIB_SHA256 and CRYPTO_RSA (used
in gip-security.c), but these are missing from the Kconfig depends block.
Can this lead to build and runtime failures when compiling without these
crypto options, or when a device requiring the security handshake connects?

>  	help
>  	  Say Y here if you want to use Xbox One and Series controllers with your
>  	  computer. Make sure to say Y to "Joystick support" (CONFIG_INPUT_JOYDEV)

[ ... ]

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 544becf8badd6..db4d2956e7d9b 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

> @@ -1809,6 +1797,8 @@ static void gip_free_devices(struct gip_attachment *attachment)
>  	struct input_dev *input;
>  	struct hid_device *hdev;
>  
> +	gip_security_release(&attachment->security);
> +

[Severity: High]
This is a pre-existing issue, but the teardown order in the pre-existing
gip_disconnect() function appears to cancel work items before stopping the
USB URB. 

gip_disconnect() calls gip_shutdown() (which eventually calls
gip_security_release() here), and only afterwards calls usb_kill_urb(). If
the incoming URB completes in this window, it can process messages and
re-queue the cancelled work items (e.g., work_exchange_rsa).

After the device is freed by devres, can the re-queued work item execute and
dereference freed pointers, leading to a Use-After-Free?

>  	if (attachment->driver && attachment->driver->remove)
>  		attachment->driver->remove(attachment);

[ ... ]

> diff --git a/drivers/input/joystick/gip/gip-security.c b/drivers/input/joystick/gip/gip-security.c
> new file mode 100644
> index 0000000000000..58de547aa1dd1
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-security.c

[ ... ]

> +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);
> +	sg_init_one(&dest, secret, secret_len);

[Severity: High]
Does this code use a stack-allocated buffer (secret) for a cryptographic
scatterlist? Passing stack memory to sg_init_one() violates DMA constraints
and can cause a kernel panic/Oops (due to BUG_ON(!virt_addr_valid(buf))) on
architectures with virtually mapped stacks.

> +	kpp_request_set_input(req, &src, pubkey_len);
> +	kpp_request_set_output(req, &dest, secret_len);

[ ... ]

> +static int gip_security_compute_ecdh(u8 *pubkey_in, u8 *pubkey_out, int pubkey_len, u8 *secret_hash)
> +{
> +	struct crypto_kpp *tfm_ecdh;
> +	u8 secret[GIP_SECURITY_ECDH_SECRET_LEN] = {0};
> +	int err;
> +
> +	tfm_ecdh = crypto_alloc_kpp("ecdh-nist-p256", 0, 0);
> +	if (IS_ERR(tfm_ecdh)) {
> +		err = PTR_ERR(tfm_ecdh);
> +		goto err_free_secret;
> +	}
> +
> +	err = gip_security_ecdh_get_pubkey(tfm_ecdh, pubkey_out, pubkey_len);
> +	if (err)
> +		goto err_free_ecdh;
> +
> +	err = gip_security_ecdh_get_secret(tfm_ecdh, pubkey_in, pubkey_len,
> +		secret, sizeof(secret));

[ ... ]

> +static void gip_security_exchange_rsa(struct work_struct *work)
> +{
> +	struct gip_security *security = container_of(work, typeof(*security), work_exchange_rsa);
> +	struct gip_attachment *attachment = container_of(security, struct gip_attachment, security);
> +	struct gip_security_host_secret message = {};
> +	u8 random[GIP_SECURITY_RANDOM_LEN * 2];
> +	int err;
> +
> +	guard(mutex)(&attachment->lock);

[ ... ]

> +void gip_security_release(struct gip_security *security)
> +{
> +	if (security->status < GIP_SECURITY_SENT)
> +		return;
> +
> +	cancel_work_sync(&security->work_exchange_rsa);

[Severity: High]
Can this cause an ABBA deadlock during device teardown?

During hot-unplug, gip_shutdown() in gip-core.c acquires attachment->lock
and then calls gip_security_release(), which synchronously waits for the
work item using cancel_work_sync(). 

Simultaneously, the running gip_security_exchange_rsa() work item blocks
trying to acquire the same attachment->lock mutex.

> +	cancel_work_sync(&security->work_exchange_ecdh);
> +	cancel_work_sync(&security->work_complete);
> +
> +	memzero_explicit(&security->transcript, sizeof(security->transcript));
> +
> +	security->status = GIP_SECURITY_PENDING;
> +}

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