[PATCH] nfc: fdp: reject an oversized device-reported packet length

[PATCH] nfc: fdp: reject an oversized device-reported packet length

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

fdp_nci_i2c_read() reads the length of the next packet from the device
into phy->next_read_size and uses it as the i2c_master_recv() byte count
into a fixed on-stack buffer:

	u8 tmp[FDP_NCI_I2C_MAX_PAYLOAD];		/* 261 bytes */
	...
	len = phy->next_read_size;
	r = i2c_master_recv(client, tmp, len);

When a "length packet" arrives (tmp[0] == 0 && tmp[1] == 0), the next
length is taken verbatim from two device-supplied bytes:

	phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;

next_read_size is a u16, so this can be driven as high as 65535 - far
larger than the 261-byte tmp[] buffer - and it is never bounded before
the next iteration's i2c_master_recv(). A malfunctioning, malicious or
counterfeit FDP NFC controller (or an attacker tampering with the I2C
bus) that sends such a length packet makes i2c_master_recv() write up to
about 64 KB into the 261-byte on-stack buffer: a stack out-of-bounds
write that clobbers the stack canary, saved registers and the return
address.

Reject a next_read_size larger than the receive buffer the same way a
corrupted packet is already handled - drop it and force resynchronization
- so a device can never drive an over-length read.

Fixes: a06347c04c13 ("NFC: Add Intel Fields Peak NFC solution driver")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
I reproduced the out-of-bounds write with an in-kernel test that drives
the fdp_nci_i2c_read() buffer geometry verbatim under KASAN
(CONFIG_KASAN_STACK=y), modelling i2c_master_recv() delivering
next_read_size device bytes into the 261-byte tmp[] buffer:

  next_read_size = 281, no bound:
    BUG: KASAN: stack-out-of-bounds in i2c_master_recv...
    Write of size 281 ... [48, 309) 'tmp'   (the 261-byte buffer)
  with the device length bounded to <= FDP_NCI_I2C_MAX_PAYLOAD (what this
    patch enforces): no KASAN report.
  a well-formed packet (length <= 261) is unaffected, no KASAN report.

The full device range - next_read_size = 65535 (tmp[2] = 0xff,
tmp[3] = 0xfc; the u16 field truncates the + 3), a 65535-byte write =
65274 bytes past the buffer, smashing the stack canary and the return
address - reproduces the same way under userspace AddressSanitizer on
both -m32 and -m64.
---
 drivers/nfc/fdp/i2c.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
index c1896a1d978c..0392bb49bb4b 100644
--- a/drivers/nfc/fdp/i2c.c
+++ b/drivers/nfc/fdp/i2c.c
@@ -166,6 +166,20 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
 		/* Packet that contains a length */
 		if (tmp[0] == 0 && tmp[1] == 0) {
 			phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;
+
+			/*
+			 * next_read_size is taken from the device and is used
+			 * as the i2c_master_recv() count on the next iteration.
+			 * A value larger than the receive buffer would overflow
+			 * tmp[]; treat it like a corrupted packet and force
+			 * resynchronization.
+			 */
+			if (phy->next_read_size > FDP_NCI_I2C_MAX_PAYLOAD) {
+				dev_dbg(&client->dev, "%s: corrupted packet\n",
+					__func__);
+				phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
+				goto flush;
+			}
 		} else {
 			phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
 

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260615-b4-disp-f42dce2d-055035ea37ba

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

Re: [PATCH] nfc: fdp: reject an oversized device-reported packet length

[Simon Horman]

On Mon, Jun 15, 2026 at 03:04:02AM -0500, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> fdp_nci_i2c_read() reads the length of the next packet from the device
> into phy->next_read_size and uses it as the i2c_master_recv() byte count
> into a fixed on-stack buffer:
> 
> 	u8 tmp[FDP_NCI_I2C_MAX_PAYLOAD];		/* 261 bytes */
> 	...
> 	len = phy->next_read_size;
> 	r = i2c_master_recv(client, tmp, len);
> 
> When a "length packet" arrives (tmp[0] == 0 && tmp[1] == 0), the next
> length is taken verbatim from two device-supplied bytes:
> 
> 	phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;
> 
> next_read_size is a u16, so this can be driven as high as 65535 - far
> larger than the 261-byte tmp[] buffer - and it is never bounded before
> the next iteration's i2c_master_recv(). A malfunctioning, malicious or
> counterfeit FDP NFC controller (or an attacker tampering with the I2C
> bus) that sends such a length packet makes i2c_master_recv() write up to
> about 64 KB into the 261-byte on-stack buffer: a stack out-of-bounds
> write that clobbers the stack canary, saved registers and the return
> address.
> 
> Reject a next_read_size larger than the receive buffer the same way a
> corrupted packet is already handled - drop it and force resynchronization
> - so a device can never drive an over-length read.
> 
> Fixes: a06347c04c13 ("NFC: Add Intel Fields Peak NFC solution driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
> I reproduced the out-of-bounds write with an in-kernel test that drives
> the fdp_nci_i2c_read() buffer geometry verbatim under KASAN
> (CONFIG_KASAN_STACK=y), modelling i2c_master_recv() delivering
> next_read_size device bytes into the 261-byte tmp[] buffer:
> 
>   next_read_size = 281, no bound:
>     BUG: KASAN: stack-out-of-bounds in i2c_master_recv...
>     Write of size 281 ... [48, 309) 'tmp'   (the 261-byte buffer)
>   with the device length bounded to <= FDP_NCI_I2C_MAX_PAYLOAD (what this
>     patch enforces): no KASAN report.
>   a well-formed packet (length <= 261) is unaffected, no KASAN report.
> 
> The full device range - next_read_size = 65535 (tmp[2] = 0xff,
> tmp[3] = 0xfc; the u16 field truncates the + 3), a 65535-byte write =
> 65274 bytes past the buffer, smashing the stack canary and the return
> address - reproduces the same way under userspace AddressSanitizer on
> both -m32 and -m64.
> ---
>  drivers/nfc/fdp/i2c.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
> index c1896a1d978c..0392bb49bb4b 100644
> --- a/drivers/nfc/fdp/i2c.c
> +++ b/drivers/nfc/fdp/i2c.c
> @@ -166,6 +166,20 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
>  		/* Packet that contains a length */
>  		if (tmp[0] == 0 && tmp[1] == 0) {
>  			phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;


Thanks Bryam,

I agree with your analysis regarding overrunning tmp and that the
fix for that is correct.

But I am concerned that there is also an expectation in the code that
next_read_size is always at least FDP_NCI_I2C_MIN_PAYLOAD (5).
But that smaller values can be achieved if either:

* tmp[2] is 0 and tmp[3] is < 2.
* the addition above overflows 16bits. e.g. both tmp[2] and tmp[3] are 255.

So I wonder if the check you are adding below should also guard
against phy->next_read_size < FDP_NCI_I2C_MIN_PAYLOAD.

> +
> +			/*
> +			 * next_read_size is taken from the device and is used
> +			 * as the i2c_master_recv() count on the next iteration.
> +			 * A value larger than the receive buffer would overflow
> +			 * tmp[]; treat it like a corrupted packet and force
> +			 * resynchronization.
> +			 */
> +			if (phy->next_read_size > FDP_NCI_I2C_MAX_PAYLOAD) {
> +				dev_dbg(&client->dev, "%s: corrupted packet\n",
> +					__func__);
> +				phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
> +				goto flush;
> +			}
>  		} else {
>  			phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
>  
> 
> ---
> base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
> change-id: 20260615-b4-disp-f42dce2d-055035ea37ba
> 
> Best regards,
> -- 
> Bryam Vargas <hexlabsecurity@proton.me>
> 
> 

Re: [PATCH] nfc: fdp: reject an oversized device-reported packet length

[Bryam Vargas]

> So I wonder if the check you are adding below should also guard
> against phy->next_read_size < FDP_NCI_I2C_MIN_PAYLOAD.

Agreed. Both routes are reachable: a length packet with tmp[2] == 0 &&
tmp[3] < 2 gives 3 or 4 directly, and the + 3 can carry the u16 store
past 65535 (tmp[2] == tmp[3] == 0xff -> 65538, truncated to 2).
next_read_size then drops below the 2-byte header + 1-byte LRC that
fdp_nci_i2c_remove_len_lrc() strips, and the short read also leaves
tmp[2]/tmp[3] unrefreshed when the next packet is parsed as a length
packet, so the following length comes from stale buffer bytes. It is not
a second overflow -- skb_pull()/skb_trim() are self-guarding, so the
frame is malformed rather than out of bounds -- but the desync is real
and bounding it is correct.

v2 folds both ends into one range check on the stored value, so the
truncation case is covered too. It also carries a fix for an skb leak in
the same function (a device that sends two data packets in one call
leaks the first skb); I am happy to split that into its own patch if you
would prefer.

I will add your Suggested-by.

Bryam