Re: [PATCH 10/11] tty: Add NVIDIA LSTP UART driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: silent data loss and false success on persistent firmware NACK
- [High] tty: Use-After-Free of ctx due to missing cancel_work_sync in destruct
--

commit dd0d55ce97ca80c844bb25609ee855296bb6dd01
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

tty: Add NVIDIA LSTP UART driver

This commit adds a tty driver for the UART channels of NVIDIA LSTP USB
adapters. The driver handles firmware configurations, modem control lines,
and coalesced transmit buffers sent over the USB channel.

> diff --git a/drivers/tty/lstp_tty.c b/drivers/tty/lstp_tty.c
> index 0000000000000..64894eca16ee1
> --- /dev/null
> +++ b/drivers/tty/lstp_tty.c
> @@ -187,27 +187,27 @@ static int lstp_uart_send_with_retry(struct lstp_uart_ctx *ctx, u8 cmd, const vo
>  	int attempt;
>  
>  	for (attempt = 0; lstp_status == LSTP_NACK && attempt < LSTP_UART_NAK_MAX_RETRIES;
>  	     attempt++) {
>  		if (attempt)
>  			fsleep(LSTP_UART_NAK_RETRY_DELAY_MS * USEC_PER_MSEC);
>  
>  		ret = lstp_request(ch, cmd, payload, len, NULL, 0, NULL, &lstp_status);
>  	}
>  
>  	if (ret && ret != -ENODEV)
>  		dev_warn_ratelimited(ch->dev,
>  				     "%s: ch_%d: Failed to write after %d attempt(s) (%pe)\n",
>  				     __func__, ch->ch_id, attempt, ERR_PTR(ret));
>  
>  	return ret;
>  }

[Severity: High]
If the firmware persistently responds with LSTP_NACK and the loop exhausts
all LSTP_UART_NAK_MAX_RETRIES, does this exit with ret equal to 0 instead of
an error code? When lstp_request transmits successfully over USB but the
firmware NACKs, it populates lstp_status with LSTP_NACK and returns 0. If
ret remains 0, the error warning is bypassed.

>  
>  /* @tx_snapshot is written only here, so it stays stable across the retry loop. */
>  static void lstp_uart_write_work(struct work_struct *work)
>  {
>  	struct lstp_uart_ctx *ctx = container_of(work, struct lstp_uart_ctx, write_work);
>  	size_t len;
[ ... ]
>  	int ret = lstp_uart_send_with_retry(ctx, LSTP_UART_CMD_WRITE, ctx->tx_snapshot, len);
>  
>  	scoped_guard(spinlock_irqsave, &ctx->write_lock) {
>  		ctx->tx_in_flight = false;
>  		if (!ret)
>  			WRITE_ONCE(ctx->iocount.tx, ctx->iocount.tx + len);

[Severity: High]
Does this treat the NACK timeout from lstp_uart_send_with_retry as a
success? Because ret is 0, it appears the driver will increment iocount.tx,
clear tx_in_flight, and permanently discard the tx_snapshot buffer. Could
this cause silent data loss under backpressure, falsely informing the TTY
layer that bytes were successfully sent?

[ ... ]
> @@ -389,17 +389,17 @@ static void lstp_port_shutdown(struct tty_port *port)
>  {
>  	struct lstp_uart_ctx *ctx = container_of(port, struct lstp_uart_ctx, port);
>  
>  	cancel_work_sync(&ctx->write_work);
>  
>  	/*
>  	 * Normally already empty. What survives the close drain and the hangup
>  	 * flush is a drain still stalled when closing_wait expires: bytes queued
>  	 * with no worker after the cancel above, so write_room() would stay
>  	 * short by that much for the life of the port.
>  	 */
>  	scoped_guard(spinlock_irqsave, &ctx->write_lock) {
>  		ctx->coal_len = 0;
>  		ctx->tx_inflight_len = 0;
>  	}
>  }

[Severity: High]
Since cancel_work_sync is called outside the write_lock critical section,
is it possible for a concurrent lstp_tty_write to schedule new work
immediately after the cancellation completes?

[ ... ]
> @@ -490,20 +490,20 @@ static ssize_t lstp_tty_write(struct tty_struct *tty, const unsigned char *buf, size_t count)
>  {
>  	struct lstp_uart_ctx *ctx = tty->driver_data;
>  	struct lstp_channel *ch = ctx->ch;
[ ... ]
>  	count = min(count, room);
>  	memcpy(ctx->coal_buf + ctx->coal_len, buf, count);
>  	ctx->coal_len += count;
>  
>  	schedule_work(&ctx->write_work);
>  
>  	return count;
>  }
[ ... ]
> @@ -407,9 +407,9 @@ static void lstp_port_destruct(struct tty_port *port)
>  {
>  	struct lstp_uart_ctx *ctx = container_of(port, struct lstp_uart_ctx, port);
>  	struct lstp_channel *ch = ctx->ch;
>  
>  	lstp_free_minor(ctx);
>  	kfree(ctx->tx_snapshot);
>  	kfree(ctx->coal_buf);
>  	kfree(ctx);

[Severity: High]
Is a final cancel_work_sync needed here before freeing ctx?

If lstp_port_shutdown cancels the work locklessly, a concurrent
lstp_tty_write could acquire write_lock, see the channel as still connected,
and call schedule_work on ctx->write_work.

If lstp_port_shutdown then acquires write_lock, clears the lengths, and
returns, the TTY port could eventually be put and destructed. Without a
final cancel_work_sync in lstp_port_destruct, would the queued work execute
on the freed ctx memory, causing a use-after-free crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-lstp-usb-adapter-v1-0-aambrozewicz@nvidia.com?part=10