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

On Fri, Jul 24, 2026 at 06:22:55AM -0500, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <[email protected]>
> 
> 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: [email protected]
> Suggested-by: Simon Horman <[email protected]>
> Signed-off-by: Bryam Vargas <[email protected]>
> ---
> v3:
>  - Rebase on current net; no code change since v2. Doruk Tan Ozturk
>    independently found the same missing bound (0sec,
>    https://lore.kernel.org/all/[email protected]) and
>    noted there that this v2 is the more complete fix -- it also rejects the
>    sub-minimum length Simon flagged and closes the skb leak. Reposting now
>    that the driver has an active maintainer.
>  v2: https://lore.kernel.org/all/[email protected]
> 
> 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/[email protected]
> 
> 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

Reviewed-by: Simon Horman <[email protected]>