Re: [PATCH net v3 3/4] nfc: llcp: fix TLV parsing OOB in nfc_llcp_recv_snl

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
nfc: llcp: fix TLV parsing OOB in nfc_llcp_recv_snl

This commit fixes multiple out-of-bounds read vulnerabilities in the
nfc_llcp_recv_snl() function when handling Service Name Lookup PDUs from
hostile NFC peers.

> nfc: llcp: fix TLV parsing OOB in nfc_llcp_recv_snl
>
> nfc_llcp_recv_snl() has four problems when handling a hostile peer:
>
>  1. nfc_llcp_dsap()/nfc_llcp_ssap() dereference skb->data[0..1] without
>     verifying skb->len; a 0- or 1-byte frame leads to an OOB read.
>     Additionally tlv_len = skb->len - LLCP_HEADER_SIZE wraps when
>     skb->len < 2, causing the following loop to run far past the
>     buffer.
>
>  2. The per-iteration loop guard `offset < tlv_len` only proves one
>     byte is available, but the body reads tlv[0] and tlv[1].
>
>  3. The peer-supplied `length` field is used to advance `tlv` without
>     being checked against the remaining array space.
>
>  4. The SDREQ handler previously only required length >= 1 but reads
>     both tid (tlv[2]) and the first byte of service_name (tlv[3], via
>     the pr_debug("%.16s") print and the service_name_len = length - 1
>     string usage), so length >= 2 is required.
>
> Fix: reject frames smaller than LLCP_HEADER_SIZE up front; add TLV
> header and TLV value guards at the top of each iteration; bump the
> SDREQ minimum length to 2.
>
> Reachable from any NFC peer within ~4 cm once an LLCP link is up.
>
> Fixes: 7a06f0ee2823 ("NFC: llcp: Service Name Lookup implementation")

The Fixes: tag references commit 7a06f0ee2823, but this commit does
not exist in the repository. The vulnerable TLV parsing was actually
introduced in commit 19cfe5843e86 ("NFC: Initial SNL support") from
October 5, 2012, which added nfc_llcp_recv_snl() with the bounds
checking issues.

Should the Fixes: tag be:

    Fixes: 19cfe5843e86 ("NFC: Initial SNL support")

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index db5bc6a878dd..8e7b8d2347d6 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -1284,6 +1284,11 @@ static void nfc_llcp_recv_snl(struct nfc_llcp_local *local,
>  	size_t sdres_tlvs_len;
>  	HLIST_HEAD(nl_sdres_list);
>
> +	if (skb->len < LLCP_HEADER_SIZE) {
> +		pr_err("Malformed SNL PDU\n");
> +		return;
> +	}
> +
>  	dsap = nfc_llcp_dsap(skb);
>  	ssap = nfc_llcp_ssap(skb);
>
> @@ -1300,11 +1305,17 @@ static void nfc_llcp_recv_snl(struct nfc_llcp_local *local,
>  	sdres_tlvs_len = 0;
>
>  	while (offset < tlv_len) {
> +		if (tlv_len - offset < 2)
> +			break;
>  		type = tlv[0];
>  		length = tlv[1];
> +		if (tlv_len - offset - 2 < length)
> +			break;
>
>  		switch (type) {
>  		case LLCP_TLV_SDREQ:
> +			if (length < 2)
> +				break;
>  			tid = tlv[2];
>  			service_name = (char *) &tlv[3];
>  			service_name_len = length - 1;
>
>  			pr_debug("Looking for %.16s\n", service_name);

The new length check ensures length >= 2 to protect tlv[2] and tlv[3]
accesses. However, pr_debug with %.16s can read up to 16 bytes from
service_name (starting at &tlv[3]), not just one byte as stated in the
commit message.

When length==2 (the minimum), only tlv[3] is validated, but pr_debug
may read through tlv[18] while searching for a null terminator. If a
hostile peer sends a SDREQ with length=2 near the end of the buffer,
pr_debug could read beyond the validated region into skb tail room.

This only occurs when DEBUG is enabled (development kernels), so it's
less critical than the production OOB reads being fixed. However, should
the debug code use bounded printing to match the security-focused nature
of this patch?

    pr_debug("Looking for %.*s\n", (int)service_name_len, service_name);

[ ... ]

> @@ -1363,6 +1374,8 @@ static void nfc_llcp_recv_snl(struct nfc_llcp_local *local,
>  		break;
>
>  	case LLCP_TLV_SDRES:

In v2 of this patch, Paolo Abeni noted that SDRES also needs a length
check for accessing tlv[2] and tlv[3]. While v2 included this check,
it was removed in v3 and is missing from the current version.

SDREQ now has the length < 2 protection (added above), but SDRES still
lacks validation. Looking at the SDRES handler later in the function:

net/nfc/llcp_core.c:nfc_llcp_recv_snl() {
    case LLCP_TLV_SDRES:
        dsap = tlv[2];     /* reads tlv[2] without validation */
        ...
        ssap = tlv[3];     /* reads tlv[3] without validation */
}

Should SDRES also have the length >= 2 check before accessing tlv[2]
and tlv[3]?