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

Paolo Abeni
2026-04-14T07:52:50Z

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