Re: [PATCH] NFC: s3fwrn5: bound the UART receive buffer

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
NFC: s3fwrn5: bound the UART receive buffer

This commit attempts to fix a potential buffer overrun in the UART receive
handler by dropping oversized frames before they exceed NCI_SKB_BUFF_LEN.

> diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
> index ccfcee06ee77..aba688499c05 100644
> --- a/drivers/nfc/s3fwrn5/uart.c
> +++ b/drivers/nfc/s3fwrn5/uart.c
> @@ -63,8 +63,17 @@ static size_t s3fwrn82_uart_read(struct serdev_device *serdev,
[ ... ]
>  		if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
> -				< phy->recv_skb->data[S3FWRN82_NCI_IDX])
> +				< phy->recv_skb->data[S3FWRN82_NCI_IDX]) {
> +			if (phy->recv_skb->len < NCI_SKB_BUFF_LEN)
> +				continue;

Is it mathematically possible for this condition to be false?

Since phy->recv_skb->data[S3FWRN82_NCI_IDX] is an 8-bit unsigned integer,
its maximum value is 255. With S3FWRN82_NCI_HEADER being 3, the check
len - 3 < data[2] ensures the length is strictly less than 258 when
entering this block.

Because NCI_SKB_BUFF_LEN is 258, wouldn't len < NCI_SKB_BUFF_LEN always
evaluate to true, making the new frame-dropping logic unreachable?

> +
> +			dev_dbg(&serdev->dev, "dropping oversized UART frame\n");
> +			kfree_skb(phy->recv_skb);
> +			phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> +			if (!phy->recv_skb)
> +				return 0;

This isn't a new issue introduced by this patch, but does returning 0 here
after a failed allocation cause a NULL pointer dereference?

If alloc_skb() fails, phy->recv_skb becomes NULL and the function returns 0.
When the serdev core retries delivering the buffer later, the loop
immediately calls skb_put_u8(phy->recv_skb, *data++), which will dereference
the NULL pointer.

Also, does returning 0 after already processing some bytes corrupt the
UART state machine by forcing already-consumed bytes to be replayed?

>  			continue;
> +		}
>  
>  		s3fwrn5_recv_frame(phy->common.ndev, phy->recv_skb,
>  				   phy->common.mode);
-- 
pw-bot: cr