Re: [PATCH net] nfc: llcp: fix tlv offset wrap and missing bounds checks

[Simon Horman]

On Tue, Mar 24, 2026 at 09:25:41PM +0000, Oleh Konko wrote:
> nfc_llcp_parse_gb_tlv() and nfc_llcp_parse_connection_tlv() iterate a
> u16 tlv_array_len with a u8 offset. once cumulative TLV consumption
> crosses 255 bytes, offset wraps and the loop may continue past the
> declared TLV array bounds.
> 
> both parsers also read tlv[1] before checking that a full 2-byte TLV
> header remains, and they advance by length + 2 without validating that
> the declared payload still fits in the remaining array.
> 
> fix this by widening offset to u16 and by rejecting incomplete headers
> or truncated TLVs before dereferencing or advancing the cursor.

This patch seems to fix two problems.  It's usually best to fix one problem
per patch, creating a patch-set with two or more patches and a cover letter
if necessary.

> 
> Fixes: d646960f7986 ("NFC: Initial LLCP support")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Oleh Konko <security@xxxxxxxxx>
> Signed-off-by: Oleh Konko <security@xxxxxxxxx>

This patch seems to have been sent twice in about half an hour.
Please don't do that.

Rather, please observe a pause of 24h between sending updated
revisions of a patch.

Link: https://docs.kernel.org/process/maintainer-netdev.html

> ---
>  net/nfc/llcp_commands.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
> index 291f26fac..157afd62f 100644
> --- a/net/nfc/llcp_commands.c
> +++ b/net/nfc/llcp_commands.c
> @@ -193,7 +193,8 @@ int nfc_llcp_parse_gb_tlv(struct nfc_llcp_local *local,
>  			  const u8 *tlv_array, u16 tlv_array_len)
>  {
>  	const u8 *tlv = tlv_array;
> -	u8 type, length, offset = 0;
> +	u8 type, length;
> +	u16 offset = 0;

It seems to me that the value passed to this function by the caller,
nfc_llcp_set_remote_gb(), is limited to a maximum value of 255 - 3 by
virtue of being a u8 with 3 subtracted. So I'm not sure that offset
can overflow here.

For consistency with other parts of this call-chain
it might be worth changing the type of tlv_array_len to u8.
But that would be a clean-up for net-next rather than a fix for net.

>  
>  	pr_debug("TLV array length %d\n", tlv_array_len);
>  
> @@ -201,6 +202,9 @@ int nfc_llcp_parse_gb_tlv(struct nfc_llcp_local *local,
>  		return -ENODEV;
>  
>  	while (offset < tlv_array_len) {
> +		if (tlv_array_len - offset < 2)
> +			return -EINVAL;
> +
>  		type = tlv[0];
>  		length = tlv[1];
>  
> @@ -227,6 +231,9 @@ int nfc_llcp_parse_gb_tlv(struct nfc_llcp_local *local,
>  			break;
>  		}
>  
> +		if (tlv_array_len - offset < (u16)length + 2)
> +			return -EINVAL;
> +
>  		offset += length + 2;
>  		tlv += length + 2;
>  	}
> @@ -243,7 +250,8 @@ int nfc_llcp_parse_connection_tlv(struct nfc_llcp_sock *sock,
>  				  const u8 *tlv_array, u16 tlv_array_len)
>  {
>  	const u8 *tlv = tlv_array;
> -	u8 type, length, offset = 0;
> +	u8 type, length;
> +	u16 offset = 0;

In this case the callers pass skb->ken as tlv_array_len.
As skb->len is an unsigned int in theory it might overflow
the u16 used for tlv_array_len. But perhaps in practice that
doesn't occur?

In any case, if you are going to update the type of offset, then
I would just change the type of type, length and offset to unsigned int.
I don't see a value in using narrower types here.

>  
>  	pr_debug("TLV array length %d\n", tlv_array_len);
>  
> @@ -251,6 +259,9 @@ int nfc_llcp_parse_connection_tlv(struct nfc_llcp_sock *sock,
>  		return -ENOTCONN;
>  
>  	while (offset < tlv_array_len) {
> +		if (tlv_array_len - offset < 2)
> +			return -EINVAL;
> +
>  		type = tlv[0];
>  		length = tlv[1];
>  
> @@ -270,6 +281,9 @@ int nfc_llcp_parse_connection_tlv(struct nfc_llcp_sock *sock,
>  			break;
>  		}
>  
> +		if (tlv_array_len - offset < (u16)length + 2)
> +			return -EINVAL;
> +
>  		offset += length + 2;
>  		tlv += length + 2;
>  	}
> -- 
> 2.50.0
>