[PATCH v2] nfc: fdp: bound the device-reported read length and fix an skb leak

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@xxxxxxxxx>

fdp_nci_i2c_read() takes the next packet length from two device-supplied
bytes and never validates it. The value is a u16 used as the
i2c_master_recv() count into a 261-byte on-stack buffer: a malicious,
counterfeit or malfunctioning controller (or an i2c bus interposer) can
drive it far past the buffer for a stack out-of-bounds write that
clobbers the canary and return address, or below the minimum frame size
(directly, or by truncating the computed sum) so the header/LRC strip
and the next length read run past a short receive. Reject a length
outside [FDP_NCI_I2C_MIN_PAYLOAD, FDP_NCI_I2C_MAX_PAYLOAD], as a
corrupted packet already is, and force resynchronization.

The same loop allocates one data skb per iteration and assumes a length
packet followed by a data packet; a device that sends two data packets
in one call leaks the first skb when the second allocation overwrites
it. Free a previously allocated skb before allocating the next.

Fixes: a06347c04c13 ("NFC: Add Intel Fields Peak NFC solution driver")
Cc: stable@xxxxxxxxxxxxxxx
Suggested-by: Simon Horman <horms@xxxxxxxxxx>
Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
---
v2:
 - Also reject next_read_size < FDP_NCI_I2C_MIN_PAYLOAD, not just
   > FDP_NCI_I2C_MAX_PAYLOAD (Simon Horman). The small value is reachable
   both directly (tmp[2] == 0 && tmp[3] < 2) and through the u16
   truncation of the computed sum (e.g. 0xff,0xff -> 65538 -> 2); a single
   range check on the stored value covers both, and also keeps the next
   length-field read from running on stale buffer bytes.
 - Fold in a fix for an skb leak in the same function (two data packets
   in one call overwrite and leak the first skb).
 v1: https://lore.kernel.org/all/20260615-b4-disp-f42dce2d-v1-1-186ff3dcbf37@xxxxxxxxx

Reproduced in-kernel on x86-64 (Linux 7.1.0-rc5, CONFIG_KASAN_STACK=y)
with a faithful port of the read loop, and at full device magnitude with
a userspace AddressSanitizer model:

  Stack OOB write
    A  no bound, next_read_size 281 -> 20 B past tmp[261]:
         BUG: KASAN: stack-out-of-bounds in i2c_master_recv...
         Write of size 281 ... This frame has 1 object: [48, 309) 'tmp'
    B  bounded to <= FDP_NCI_I2C_MAX_PAYLOAD:   no KASAN report
    C  well-formed (len 5):                     no KASAN report
    ASan model, full u16 next_read_size = 65535 -> 65274-byte
    stack-buffer-overflow WRITE on both -m32 and -m64; bounded build clean.

skb leak (slabinfo active-object delta over 20000 reads, two data
  packets each, measured after a slab shrink)
    without the fix:  skbuff_head_cache +20047, skbuff_small_head +20057
                      (one orphaned skb per call, unreclaimable)
    with the fix:     ~0
---
 drivers/nfc/fdp/i2c.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
index c1896a1d978c..f292e7f37456 100644
--- a/drivers/nfc/fdp/i2c.c
+++ b/drivers/nfc/fdp/i2c.c
@@ -166,9 +166,36 @@ 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 for the next packet
+			 * and as the data skb size. A value above the receive
+			 * buffer overflows tmp[]; one below the minimum frame
+			 * size runs the header/LRC strip and the length-field
+			 * read past a short receive. Either way the packet is
+			 * corrupt: drop it and force resynchronization.
+			 */
+			if (phy->next_read_size < FDP_NCI_I2C_MIN_PAYLOAD ||
+			    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;
 
+			/*
+			 * Only one data packet is delivered per call; if the
+			 * device sends another, do not overwrite and leak the
+			 * skb allocated for the previous one.
+			 */
+			if (*skb) {
+				kfree_skb(*skb);
+				*skb = NULL;
+			}
+
 			*skb = alloc_skb(len, GFP_KERNEL);
 			if (*skb == NULL) {
 				r = -ENOMEM;

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260616-b4-disp-b1f8ab4c-b06ad43c9be5

Best regards,
-- 
Bryam Vargas <hexlabsecurity@xxxxxxxxx>