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

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

[Pengpeng Hou]

s3fwrn82_uart_read() appends bytes to phy->recv_skb with skb_put_u8().
It only resets the skb after a complete NCI frame is assembled.

The receive skb is allocated with NCI_SKB_BUFF_LEN bytes, but
malformed UART traffic can keep the frame incomplete and continue
growing recv_skb until skb_put_u8() overruns the tailroom.

Drop the accumulated partial frame once the fixed receive buffer is full
and no complete frame has been seen yet so malformed UART traffic cannot
grow the skb past NCI_SKB_BUFF_LEN.

Fixes: 3f52c2cb7e3a ("nfc: s3fwrn5: Support a UART interface")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/nfc/s3fwrn5/uart.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
index 9c09c10c2a46..c664fed78c1f 100644
--- a/drivers/nfc/s3fwrn5/uart.c
+++ b/drivers/nfc/s3fwrn5/uart.c
@@ -64,8 +64,17 @@ static size_t s3fwrn82_uart_read(struct serdev_device *serdev,
 			continue;
 
 		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;
+
+			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;
 			continue;
+		}
 
 		s3fwrn5_recv_frame(phy->common.ndev, phy->recv_skb,
 				   phy->common.mode);
-- 
2.50.1 (Apple Git-155)

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