Re: [PATCH net v2 2/3] nfc: llcp: add TLV length bounds checks in parse_gb_tlv and parse_connection_tlv

[Paolo Abeni <pabeni@redhat.com>]

On 4/9/26 8:59 PM, Lekë Hapçiu wrote:
> From: Lekë Hapçiu <framemain@outlook.com>
> 
> v1 of this fix promoted `offset` from u8 to u16 in both TLV parsers,
> preventing the infinite loop when a connection TLV array exceeds 255 bytes.
> During review, Simon Horman identified two additional issues that the u16
> promotion alone does not address.
> 
> Issue 1 - truncated TLV header:
> 
>   The loop guard `offset < tlv_array_len` is not sufficient to guarantee
>   that reading tlv[0] (type) and tlv[1] (length) is safe.  When exactly
>   one byte remains (offset == tlv_array_len - 1) the loop body reads
>   tlv[1] one byte past the end of the array.
> 
> Issue 2 - peer-controlled `length` field:
> 
>   `length` is read from peer-supplied frame data and is not checked against
>   the remaining array space before advancing `tlv` and `offset`:
> 
>     offset += length + 2;   /* always */
>     tlv    += length + 2;   /* may now point past buffer end */
> 
>   A crafted `length` advances `tlv` past the array boundary; the following
>   iteration reads tlv[0]/tlv[1] from adjacent kernel memory.
> 
>   For nfc_llcp_parse_gb_tlv() this is particularly impactful: its input is
>   &local->remote_gb[3], a field within nfc_llcp_local.  A large `length`
>   can walk `tlv` into adjacent struct fields including sdreq_timer and
>   sdreq_timeout_work which contain kernel function pointers at approximately
>   +176 and +216 bytes past remote_gb[].  The parsed `type` byte at those
>   positions may match a recognized TLV type causing the parser to store
>   bytes from the function pointer into local->remote_miu, which is
>   subsequently readable via getsockopt().
> 
> Issue 3 - zero-length TLV value:
> 
>   The llcp_tlv8() and llcp_tlv16() accessor helpers read tlv[2] and
>   tlv[2..3] respectively.  The outer guard guarantees `length` bytes of
>   value are available past the two-byte header, but when length == 0 it
>   only guarantees offset+2 <= tlv_array_len (non-strict), leaving tlv[2]
>   out of bounds.  Per-type minimum-length checks are required before each
>   accessor call.  Note: llcp_tlv8/16 additionally validate against the
>   llcp_tlv_length[] table, providing a second safety layer; the per-type
>   checks here make the rejection explicit and avoid silent zero-defaults.
> 
> Fix: add two loop-level guards inside each parsing loop:
> 
>   if (tlv_array_len - offset < 2)            /* need type + length */
>       break;
>   [read type, length]
>   if (tlv_array_len - offset - 2 < length)   /* need length value bytes */
>       break;
> 
> Both subtractions are safe: the loop condition guarantees offset <
> tlv_array_len; the first guard then guarantees the difference is >= 2,
> making the second subtraction non-negative.
> 
> Add per-type minimum-length checks before each accessor call:
>   - tlv8-based (VERSION, LTO, OPT, RW): require length >= 1
>   - tlv16-based (MIUX, WKS):            require length >= 2
> 
> Reachability: nfc_llcp_parse_connection_tlv() is reached on receipt of a
> CONNECT or CC PDU before any connection is established.
> nfc_llcp_parse_gb_tlv() is reached during ATR_RES processing.  Both are
> triggerable from any NFC peer within ~4 cm with no authentication.

It would be helpful if you could condense the above text in a
significantly shorter form. Also it looks like the issue addressed by v1
is not addressed anymore here.

> 
> Reported-by: Simon Horman <horms@kernel.org>
> Fixes: 7a06e586b9bf ("NFC: Move LLCP receiver window value to socket structure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lekë Hapçiu <framemain@outlook.com>
> ---
>  net/nfc/llcp_commands.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
> index 6937dcb3b..7cc237a6d 100644
> --- a/net/nfc/llcp_commands.c
> +++ b/net/nfc/llcp_commands.c
> @@ -202,25 +202,39 @@ int nfc_llcp_parse_gb_tlv(struct nfc_llcp_local *local,
>  		return -ENODEV;
>  
>  	while (offset < tlv_array_len) {
> +		if (tlv_array_len - offset < 2)
> +			break;
>  		type = tlv[0];
>  		length = tlv[1];
> +		if (tlv_array_len - offset - 2 < length)
> +			break;

I *think* it would be better to bail out with an error, instead of
silently returning success. A similar consideration apply to the other
checks below.

>  
>  		pr_debug("type 0x%x length %d\n", type, length);
>  
>  		switch (type) {
>  		case LLCP_TLV_VERSION:
> +			if (length < 1)
> +				break;
>  			local->remote_version = llcp_tlv_version(tlv);
>  			break;
>  		case LLCP_TLV_MIUX:
> +			if (length < 2)
> +				break;

You can probably consolidate all the `length < 1` checks in the previous
one (before the switch statement and add here only `length < 2` check.

/P