Re: [PATCH v2 2/3] can: rockchip_canfd: retry the outstanding TX buffer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] TOCTOU race in `rkcanfd_tx_tail_is_eff()` causes spurious NULL pointer errors in the kernel log and a Use-After-Free vulnerability.
--

--- Patch [2]: [PATCH v2 2/3] can: rockchip_canfd: retry the outstanding TX buffer ---
commit 5e421e7397d8c64fe895d386c02cbed045a1e541
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip_canfd: retry the outstanding TX buffer

rkcanfd_xmit_retry() originally operated with a TX FIFO depth of one. At
that depth, the masked head and tail indices both select buffer 0, so using
tx_head happened to select the correct buffer.

After the FIFO depth was increased to two, tx_head instead identifies the
next free buffer when one frame is outstanding. The erratum 6 workaround
therefore requests transmission from the wrong buffer, leaving the
outstanding echo entry incomplete and the netdev TX queue stopped.

Use tx_tail to select the outstanding buffer for retransmission.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index d2cbe25c46d6c..c4ecdc9411cc6 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> @@ -57,8 +57,8 @@ static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
>  
>  void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
>  {
> -	const unsigned int tx_head = rkcanfd_get_tx_head(priv);
> -	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
> +	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
> +	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
>  
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
>  }

[Severity: High]
This is a pre-existing issue, but does the lockless access to tx_tail and
echo_skb in rkcanfd_tx_tail_is_eff() introduce a race condition?

drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_tx_tail_is_eff() {
...
	tx_tail = rkcanfd_get_tx_tail(priv);
	skb = priv->can.echo_skb[tx_tail];
	if (!skb) {
		netdev_err(priv->ndev,
			   "%s: echo_skb[%u]=NULL tx_head=0x%08x tx_tail=0x%08x\n",
			   __func__, tx_tail,
			   priv->tx_head, priv->tx_tail);

		return false;
	}

	cfd = (struct canfd_frame *)skb->data;

	return cfd->can_id & CAN_EFF_FLAG;
}

The transmit path holds netif_tx_lock, which disables softirqs but leaves
hardirqs enabled.

If a hardware RX interrupt fires concurrently, it can process the loopback
frame, free the echo_skb (setting it to NULL), and increment tx_tail.

If the interrupt fires between reading tx_tail and accessing the array,
could this read NULL and spuriously flood the kernel log with netdev_err?

If the interrupt fires after the skb pointer is read but before skb->data
is accessed, could the SKB be freed before it is used, leading to a
use-after-free when evaluating cfd->can_id & CAN_EFF_FLAG?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-master-v2-0-572974052400@qq.com?part=2