Re: [PATCH v5 3/9] greybus: operation: arm request timeout before sending

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] greybus: operation: UAF and ref underflow in gb_operation_request_send()
- [Critical] greybus: operation: UAF from early timeout in gb_operation_request_send()
- [Critical] greybus: operation: timer leak in gb_operation_request_send() on cancel
--

commit 7b73a81d11f2d699f2c8ebeafd18ae6352191433
Author: Ayush Singh <ayush@beagleboard.org>

greybus: operation: arm request timeout before sending

This commit arms the operation timeout timer before calling
gb_message_send() in gb_operation_request_send() so that a fast response
completing before gb_message_send() returns does not leave an armed timer
behind, and calls timer_delete_sync() in the err_put_active error path.

> diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
> index a63bc2eb164e9..2d37acedf36f8 100644
> --- a/drivers/greybus/operation.c
> +++ b/drivers/greybus/operation.c
> @@ -782,20 +782,25 @@ int gb_operation_request_send(struct gb_operation *operation,
>  		return 0;
>  	}
>  
> +	/*
> +	 * Arm the timeout before sending: the response can arrive, and the
> +	 * operation complete, before the host driver returns.
> +	 */
> +	if (timeout) {
> +		operation->timer.expires = jiffies + msecs_to_jiffies(timeout);
> +		add_timer(&operation->timer);
> +	}

[Severity: Critical]
Could an operation still be cancelled between gb_operation_get_active() and
add_timer(&operation->timer) in gb_operation_request_send(), leaking an
armed timer in operation->timer?

Earlier in gb_operation_request_send(), operation->errno is initialized to
-EINPROGRESS and the first gb_operation_get_active() adds operation->links
to connection->operations.

If a concurrent thread disables the connection via gb_connection_disable()
or gb_connection_disable_forced() after the second
gb_operation_get_active() succeeds and before add_timer(&operation->timer)
runs:

CPU0                                    CPU1
gb_operation_request_send()
  gb_operation_result_set(-EINPROGRESS)
  gb_operation_get_active()
  gb_operation_get_active()
                                        gb_connection_cancel_operations()
                                          gb_operation_cancel(-ESHUTDOWN)
                                            gb_operation_result_set(-ESHUTDOWN)
                                            queue_work(&operation->work)

                                        gb_operation_work()
                                          timer_delete_sync(&operation->timer)
                                          operation->callback(operation)
                                          gb_operation_put_active(operation)
                                          gb_operation_put(operation)
  add_timer(&operation->timer)
  gb_message_send() -> returns 0

Because gb_operation_work() runs timer_delete_sync(&operation->timer) before
CPU0 calls add_timer(&operation->timer), nothing deletes operation->timer
afterward. When the caller later drops its reference via gb_operation_put(),
_gb_operation_destroy() frees struct gb_operation while operation->timer is
still queued in the timer wheel.

> +
>  	ret = gb_message_send(operation->request, gfp);

[Severity: Critical]
What happens if operation->timer expires after add_timer(&operation->timer)
is called, but before the host driver in gb_message_send() has submitted its
transport request?

For an asynchronous host driver such as drivers/greybus/es2.c, if the
sending thread is delayed or preempted before message_send() assigns
message->hcpriv and calls usb_submit_urb():

drivers/greybus/es2.c:message_send() {
	...
	spin_lock_irqsave(&es2->cport_out_urb_lock, flags);
	message->hcpriv = urb;
	spin_unlock_irqrestore(&es2->cport_out_urb_lock, flags);
	...
	retval = usb_submit_urb(urb, gfp_mask);
}

If operation->timer fires first, gb_operation_timeout() sets
operation->errno to -ETIMEDOUT and schedules gb_operation_work():

gb_operation_timeout()
  gb_operation_result_set(operation, -ETIMEDOUT)
  queue_work(gb_operation_completion_wq, &operation->work)

gb_operation_work()
  timer_delete_sync(&operation->timer)
  gb_message_cancel(operation->request)
    es2.c:message_cancel() -> no-op because message->hcpriv is not yet active
  operation->callback(operation)
  gb_operation_put_active(operation)
  gb_operation_put(operation)

When the sending thread resumes in es2.c:message_send(), it submits the URB
and returns 0 to gb_operation_request_send(). Because operation->callback()
has already completed the operation, the caller can drop the final reference
to operation via gb_operation_put(), freeing operation->request and
operation->request->buffer in _gb_operation_destroy() while the URB is still
in flight.

>  	gb_operation_put_active(operation);
>  	gb_operation_put(operation);
>  	if (ret)
>  		goto err_put_active;
>  
> -	if (timeout) {
> -		operation->timer.expires = jiffies + msecs_to_jiffies(timeout);
> -		add_timer(&operation->timer);
> -	}
> -
>  	return 0;
>  
>  err_put_active:
> +	timer_delete_sync(&operation->timer);
>  	gb_operation_put_active(operation);
>  err_put:
>  	gb_operation_put(operation);

[Severity: Critical]
Can this error path underflow operation->active and operation->kref and
trigger a use-after-free on struct gb_operation when gb_message_send() fails
after operation->timer has already expired (or after gb_operation_cancel()
has been called)?

In gb_operation_request_send(), two pairs of kref and active references are
taken before gb_message_send(): one pair for operation completion, and one
pair held across gb_message_send().

With operation->timer armed before gb_message_send(), if gb_message_send()
sleeps or takes longer than timeout before returning an error (for example,
hdlc_tx_frames() in gb-beagleplay.c waiting on a full TX circular buffer
before returning -EAGAIN, or write_all() in gb_uart_node.rs returning
-EINTR), operation->timer can fire while gb_message_send() is still running:

gb_operation_timeout()
  if (gb_operation_result_set(operation, -ETIMEDOUT))
      queue_work(gb_operation_completion_wq, &operation->work);

gb_operation_work() then runs on gb_operation_completion_wq, invokes
operation->callback(operation), and drops the completion reference pair:

gb_operation_work()
  ...
  operation->callback(operation);
  gb_operation_put_active(operation);
  gb_operation_put(operation);

When gb_message_send() returns a non-zero ret, gb_operation_request_send()
drops the send-guard reference pair and jumps to err_put_active:

  ret = gb_message_send(operation->request, gfp);
  gb_operation_put_active(operation);
  gb_operation_put(operation);
  if (ret)
      goto err_put_active;

In err_put_active, timer_delete_sync(&operation->timer) only waits for the
timer callback to finish; it does not cancel the already-queued
operation->work, nor does err_put_active check gb_operation_result_set() to
see whether completion was already claimed.

Consequently, err_put_active drops gb_operation_put_active(operation) and
gb_operation_put(operation) a third time when only two pairs were acquired,
underflowing operation->active to -1 (which also causes wait_event() in
gb_operation_cancel() to hang) and dropping the caller's own kref on
operation (or accessing operation after it was already freed if an
asynchronous callback such as gb_loopback_async_operation_callback() dropped
the caller's reference before gb_operation_put() prior to err_put_active).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-gb-uart-transport-v5-0-f8e326d6561f@beagleboard.org?part=3