Re: [PATCH net] nfc: pn533: fix use-after-free in pn533_recv_frame

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
nfc: pn533: fix use-after-free in pn533_recv_frame

This commit adds a spinlock cmd_state_lock to serialize access to dev->cmd 
in pn533_recv_frame() and the command completion worker. It aims to prevent 
a use-after-free when a command is detached and freed while being 
concurrently received.

> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index d7bdbc82e2ba2..921e93a5f16f5 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c

[ ... ]

> @@ -458,10 +477,10 @@ static int __pn533_send_async(struct pn533 *dev, u8 cmd_code,
>  	mutex_lock(&dev->cmd_lock);
>  
>  	if (!dev->cmd_pending) {
> -		dev->cmd = cmd;
> +		pn533_set_current_cmd(dev, cmd);
>  		rc = dev->phy_ops->send_frame(dev, req);
>  		if (rc) {
> -			dev->cmd = NULL;
> +			pn533_set_current_cmd(dev, NULL);

[Severity: High]
This is a pre-existing issue, but if dev->phy_ops->send_frame() fails, it
jumps to the error label and frees cmd. If a concurrent receive scheduled 
the complete work, could that work also free cmd, causing a double free?

>  			goto error;
>  		}

[ ... ]

> @@ -569,10 +588,10 @@ static void pn533_wq_cmd(struct work_struct *work)
>  
>  	mutex_unlock(&dev->cmd_lock);
>  
> -	dev->cmd = cmd;
> +	pn533_set_current_cmd(dev, cmd);
>  	rc = dev->phy_ops->send_frame(dev, cmd->req);
>  	if (rc < 0) {
> -		dev->cmd = NULL;
> +		pn533_set_current_cmd(dev, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but if dev->phy_ops->send_frame() 
fails in pn533_wq_cmd(), cmd is freed. Does a race condition exist here 
similar to __pn533_send_async(), where concurrent complete work could also 
free cmd?

[Severity: High]
Additionally, this is also a pre-existing issue, but does this error path 
drop the command without calling complete_cb and without clearing 
dev->cmd_pending? Could this leave the command queue permanently hung 
upon transient bus errors?

>  		dev_kfree_skb(cmd->req);
>  		kfree(cmd);
>  		return;
>  	}

[ ... ]

> @@ -2165,6 +2184,15 @@ static int pn533_data_exchange_complete(struct pn533 *dev, void *_arg,
>   */
>  void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
>  {
> +	unsigned long flags;
> +
> +	/*
> +	 * Hold cmd_state_lock across the whole receive path so the current
> +	 * command cannot be freed by pn533_send_async_complete() between the
> +	 * dev->cmd check and the stores into it.
> +	 */
> +	spin_lock_irqsave(&dev->cmd_state_lock, flags);
> +
>  	if (!dev->cmd)
>  		goto sched_wq;

[Severity: Medium]
This isn't a bug introduced by this patch, but if there is no active command 
(!dev->cmd), this jumps to sched_wq without freeing the skb. Does this leak 
the skb if an unexpected frame arrives?

>  
>  	dev->cmd->status = status;

[ ... ]

> @@ -2182,6 +2210,7 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
>  
>  	if (pn533_rx_frame_is_ack(skb->data)) {
>  		dev_dbg(dev->dev, "%s: Received ACK frame\n", __func__);
> +		spin_unlock_irqrestore(&dev->cmd_state_lock, flags);
>  		dev_kfree_skb(skb);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does pn533_rx_frame_is_ack() safely read 
from skb->data here? Since there doesn't appear to be a check for skb->len 
before calling it, could this result in an out-of-bounds read if a malformed 
or short frame is received?