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

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Receiving a GIP_CMD_SECURITY message before the handshake is explicitly started causes a NULL pointer dereference.
- [High] Concurrent unprotected calls to crypto_shash_update corrupt the cryptographic transcript.
- [High] Missing NULL check for the state buffer allocation in gip_security_get_transcript leads to a NULL pointer dereference.
- [High] Allocating the transcript state buffer using crypto_shash_descsize instead of crypto_shash_statesize causes a heap buffer overflow.
- [High] Unchecked return values in gip_security_compute_prf lead to an information leak of kernel stack memory.
- [Medium] Missing cancellation of pending workers during protocol upgrade.
- [Medium] Unchecked return value of crypto_shash_tfm_digest() in gip_security_compute_ecdh.
- [Medium] The ECDH shared secret is freed without being securely zeroized.
--

--- Patch [7]: [PATCH v4 07/12] Input: xbox_gip - Add security implementation from xone ---
commit 5605e9d7634da03b416418354e348fe0ff787ead
Author: Vicki Pfau <[email protected]>
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. It adds a security message driver for Xbox One and
Series controllers, establishing handshakes needed for wireless dongles.

> diff --git a/drivers/input/joystick/gip/gip-security.c b/drivers/input/joystick/gip/gip-security.c
> new file mode 100644
> index 0000000000000..71da0833980ac
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-security.c
[ ... ]
> +static int gip_security_get_transcript(struct shash_desc *desc, void *transcript)
> +{
> +	void *state = kzalloc(crypto_shash_descsize(desc->tfm), GFP_KERNEL);

[Severity: High]
Does this allocate the wrong size? The crypto_shash_export function expects
a buffer sized by crypto_shash_statesize, not crypto_shash_descsize. Since
the state size can be strictly larger than the descriptor size for certain
algorithms, this could cause a heap buffer overflow.

[Severity: High]
Is it possible for kzalloc to fail here? The return value is passed directly
to crypto_shash_export without a null check, which would cause a null pointer
dereference under memory pressure.

> +	int err;
> +
> +	err = crypto_shash_export(desc, state);
[ ... ]
> +static int gip_security_compute_prf(struct shash_desc *desc, const char *label,
> +	u8 *key, int key_len, u8 *seed, int seed_len, u8 *out, int out_len)
> +{
> +	u8 hash[SHA256_DIGEST_SIZE], hash_out[SHA256_DIGEST_SIZE];
> +	int err;
> +
> +	err = crypto_shash_setkey(desc->tfm, key, key_len);
> +	if (err)
> +		return err;
> +
> +	crypto_shash_init(desc);
> +	crypto_shash_update(desc, label, strlen(label));
> +	crypto_shash_update(desc, seed, seed_len);
> +	crypto_shash_final(desc, hash);
> +
> +	while (out_len > 0) {
> +		crypto_shash_init(desc);
> +		crypto_shash_update(desc, hash, sizeof(hash));
> +		crypto_shash_update(desc, label, strlen(label));
> +		crypto_shash_update(desc, seed, seed_len);
> +		crypto_shash_final(desc, hash_out);
> +
> +		memcpy(out, hash_out, min_t(int, out_len, sizeof(hash)));

[Severity: High]
Should we check the return values of the crypto operations inside this loop?
If crypto_shash_init, crypto_shash_update, or crypto_shash_final fail, the
hash_out buffer will retain its uninitialized kernel stack data. This data is
then copied into the output buffer and eventually sent over USB, leading to an
information leak.

> +		out += sizeof(hash);
> +		out_len -= sizeof(hash);
> +
> +		crypto_shash_digest(desc, hash, sizeof(hash), hash);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int gip_security_compute_ecdh(u8 *pubkey_in, u8 *pubkey_out, int pubkey_len, u8 *secret_hash)
> +{
> +	struct crypto_kpp *tfm_ecdh;
> +	struct crypto_shash *tfm_sha;
> +	u8 *secret;
> +	int err;
[ ... ]
> +	err = gip_security_ecdh_get_pubkey(tfm_ecdh, pubkey_out, pubkey_len);
> +	if (err)
> +		goto err_free_sha;
> +
> +	err = gip_security_ecdh_get_secret(tfm_ecdh, pubkey_in, pubkey_len,
> +		secret, GIP_SECURITY_ECDH_SECRET_LEN);
> +	if (err)
> +		goto err_free_sha;
> +
> +	crypto_shash_tfm_digest(tfm_sha, secret, GIP_SECURITY_ECDH_SECRET_LEN, secret_hash);

[Severity: Medium]
Could this leave secret_hash uninitialized if the digest fails? The return
value is ignored, which means the caller might silently proceed to compute the
master secret using uninitialized stack memory.

> +
> +err_free_sha:
> +	crypto_free_shash(tfm_sha);
> +err_free_ecdh:
> +	crypto_free_kpp(tfm_ecdh);
> +err_free_secret:
> +	kfree(secret);

[Severity: Medium]
Since the secret buffer holds raw cryptographic key material, should this use
kfree_sensitive instead of kfree to prevent sensitive data from lingering in
the slab allocator?

> +
> +	return err;
> +}
> +
> +static int gip_security_send_message(struct gip_security *security,
> +	enum gip_security_command_handshake cmd, void *message, u16 len)
> +{
> +	struct gip_attachment *attachment = container_of(security, struct gip_attachment, security);
> +	struct gip_security_header_full *hdr = message;
> +	u16 data_len = len - sizeof(hdr->handshake) - GIP_SECURITY_TRAILER_LEN;
> +
> +	hdr->handshake.context = GIP_SECURITY_CTX_HANDSHAKE;
> +	hdr->handshake.options = GIP_SECURITY_OPT_ACKNOWLEDGE | GIP_SECURITY_OPT_FROM_HOST;
> +	hdr->handshake.command = cmd;
> +	hdr->handshake.length = cpu_to_be16(data_len);
> +
> +	hdr->data.command = cmd;
> +	hdr->data.version = cmd >= GIP_SECURITY2_CMD_HOST_HELLO ? 0x02 : 0x01;
> +	hdr->data.length = cpu_to_be16(data_len - sizeof(hdr->data));
> +
> +	security->last_sent_command = cmd;
> +	crypto_shash_update(security->shash_transcript, message + sizeof(hdr->handshake), data_len);

[Severity: High]
Can this code execute concurrently with the message handler? The
gip_security_send_message function is called from asynchronous workers, while
gip_security_handle_message can also update the same shash_transcript
descriptor. Since shash_desc is not thread-safe, concurrent modifications
could corrupt the cryptographic transcript.

> +
> +	return gip_send_system_message(attachment, GIP_CMD_SECURITY, GIP_FLAG_ACME, message, len);
> +}
[ ... ]
> +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 (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 (handshake->command != hdr->data.command) {
> +		/* reset transcript hash and restart handshake */
> +		gip_dbg(security, "%s: protocol upgrade\n", __func__);
> +		crypto_shash_init(security->shash_transcript);

[Severity: Critical]
If an attacker-controlled USB device sends a security packet before the
handshake is explicitly started, could security->shash_transcript be a null
pointer here and cause a dereference? The cryptographic descriptors are
initialized lazily in gip_security_start_handshake, so early packets routed
to this handler might trigger a crash.

[Severity: Medium]
If we restart the handshake here, do we need to cancel any previously
scheduled v1 workers? If a v1 worker like work_exchange_rsa is pending, it
could execute concurrently, modify the state, and corrupt the new v2 handshake.

> +		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;
> +
> +	return crypto_shash_update(security->shash_transcript,
> +		bytes + sizeof(hdr->handshake),
> +		num_bytes - sizeof(hdr->handshake));

[Severity: Critical]
Similar to the issue above, could security->shash_transcript be a null pointer
here if a packet is received before the handshake was explicitly started,
resulting in a null pointer dereference in crypto_shash_update?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7