Re: [PATCH v5 06/12] Input: xbox_gip - Allow sending fragmented and ACME messages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unconditional dereference of NULL fragment pointer on error path for non-fragmented messages.
- [High] Missing synchronization when allocating and initializing `out_fragments` slots.
- [High] Unsynchronized read-modify-write of the `intf->has_pending_out` bitmask.
- [High] Dropped ACKs permanently leak `out_fragments` slots.
--

commit 317888ee9cccdc0f09f7f1c8d616d2664382777a
Author: Vicki Pfau <vi@endrift.com>

Input: xbox_gip - Allow sending fragmented and ACME messages

This commit introduces support for sending fragmented messages that exceed the
MTU length and messages requiring ACKs using the ACME flag in the GIP protocol.
It adds queuing, processing, and acknowledgment handling logic to the xbox_gip
driver.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 57d638408daf4..544becf8badd6 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> +static void gip_free_fragment(struct gip_out_fragment *fragment)
> +{
> +	kfree(fragment->data);
> +	memset(fragment, 0, sizeof(*fragment));
> +}
[ ... ]
>  static int gip_send_raw_message(struct gip_attachment *attachment,
>  	uint8_t message_type, uint8_t flags, uint8_t seq, const uint8_t *bytes,
>  	int num_bytes)
> @@ -486,20 +529,25 @@ static int gip_send_raw_message(struct gip_attachment *attachment,
>  	struct gip_interface *intf;
>  	int offset = 3;
>  	struct gip_urb *urb = NULL;
> +	struct gip_out_fragment *fragment = NULL;
>  	int i;
>  	int rc = 0;
[ ... ]
> +	if (num_bytes > reduced_mtu ||
> +		(flags & (GIP_FLAG_ACME | GIP_FLAG_FRAGMENT)) == GIP_FLAG_ACME) {
> +		for (i = 0; i < MAX_OUT_FRAGMENTS; i++) {
> +			if (attachment->out_fragments[i].active)
> +				continue;
> +
> +			fragment = &attachment->out_fragments[i];
> +			break;
> +		}

[Severity: High]
Does this code have a missing synchronization issue?

The fragment slot is claimed and initialized locklessly here. If concurrent
callers (like force feedback workers, sysfs, or incoming message handlers)
pick the same inactive slot, could this lead to double-allocation (a memory
leak) and state corruption?

> +
> +		if (!fragment) {
> +			gip_err(attachment, "Fragmented message queue is full; dropping message\n");
> +			return -EALREADY;
> +		}
> +
> +		fragment->data = kmalloc(num_bytes, GFP_KERNEL);
> +		if (!fragment->data)
> +			return -ENOMEM;
> +		fragment->active = true;

[Severity: High]
Can these out_fragments slots permanently leak if ACKs are dropped?

When an outgoing message requires an ACK, the slot is marked active here and
relies entirely on a device-initiated ACK to be freed. Because there is no
timeout mechanism, if the device drops the message or fails to respond, could
the slot remain locked forever? Since MAX_OUT_FRAGMENTS is only 2, a couple
of dropped packets might permanently exhaust the queue.

[ ... ]
>  	guard(spinlock_irqsave)(&attachment->device->message_lock);
>  	for (i = 0; i < MAX_OUT_MESSAGES && !urb; i++) {
>  		if (!intf->out_queue[i].urb)
[ ... ]
> +	if (fragment) {
> +		/*
> +		 * Sending further fragments is handled in gip_urb_out if this fragment doesn't need
> +		 * ACKing or in gip_handle_command_protocol_control if it does. The flag should also
> +		 * be cleared if we're done sending this packet.
> +		 */
> +		if (fragment->total_length > reduced_mtu && !(flags & GIP_FLAG_ACME))
> +			intf->has_pending_out |= gip_fragment_id(attachment, fragment);
> +		else
> +			intf->has_pending_out &= ~gip_fragment_id(attachment, fragment);
>  	}
>  
>  	return rc;
> +
> +err_free_fragment:
> +	gip_free_fragment(fragment);
> +	return rc;
> +}

[Severity: Critical]
Does this error path unconditionally dereference a NULL pointer?

If gip_send_raw_message() is called for a standard, non-fragmented message,
the fragment pointer remains NULL. If the output queue is full or
usb_submit_urb() fails, the function jumps to err_free_fragment and calls
gip_free_fragment().

Since gip_free_fragment() unconditionally accesses fragment->data and calls
memset(fragment, ...), won't this immediately crash the kernel when fragment
is NULL?

[ ... ]
> +static int gip_send_next_fragment(struct gip_attachment *attachment,
> +	struct gip_out_fragment *fragment)
> +{
> +	struct gip_interface *intf;
[ ... ]
> +	if (fragment->fragment_offset == fragment->total_length) {
> +		if ((fragment->message & GIP_DATA_CLASS_MASK) == GIP_DATA_CLASS_AUDIO)
> +			intf = &attachment->device->audio;
> +		else
> +			intf = &attachment->device->data;
> +
> +		next_bytes = 0;
> +		intf->has_pending_out &= ~gip_fragment_id(attachment, fragment);

[Severity: High]
Is there an unsynchronized read-modify-write data race here?

Here in gip_send_next_fragment(), intf->has_pending_out is updated without
holding message_lock. However, as seen in gip_send_raw_message(), this shared
bitmask is also modified under message_lock.

Could concurrent updates to this shared variable cause lost updates,
permanently stalling fragment transmission for any attachments whose bits get
lost?

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