Re: nfc: nci: Fix zero-length proprietary OIDs

[Jakub Kicinski <kuba@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: nci: Fix zero-length proprietary OIDs

This patch allows zero-length payloads for proprietary OIDs (0x20-0x3F)
to support the NXP NCI_RF_TXLDO_ERROR_NTF notification case.

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index e419e020a70a..78da0fb9ef3f 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -1482,10 +1482,16 @@ static bool nci_valid_size(struct sk_buff *skb)
>  	unsigned int hdr_size = NCI_CTRL_HDR_SIZE;
>
>  	if (skb->len < hdr_size ||
> -	    !nci_plen(skb->data) ||
>  	    skb->len < hdr_size + nci_plen(skb->data)) {
>  		return false;
>  	}
> +
> +	/* Require non-zero length for standard OIDs (0x00 - 0x1F).
> +	 * But allow zero length in the proprietary range (0x20 - 0x3F). */
> +	if (!nci_plen(skb->data))
> +		if (nci_opcode_oid(nci_opcode(skb->data)) <= 0x1F)
> +			return false;

Does this validation logic create a buffer over-read vulnerability in the
Samsung s3fwrn5 driver?

The patch allows zero-length payloads for any proprietary OID (0x20-0x3F)
without distinguishing between notification (NTF) and response (RSP)
message types. The s3fwrn5 driver registers RSP handlers for proprietary
OIDs 0x22, 0x26, 0x27, and 0x28 that all unconditionally read skb->data[0]:

drivers/nfc/s3fwrn5/nci.c:s3fwrn5_nci_prop_rsp() {
    __u8 status = skb->data[0];  // No length check
    nci_req_complete(ndev, status);
    return 0;
}

If hardware sends a malformed zero-length RSP packet with one of these
proprietary OIDs, the validation would pass (since OID > 0x1F), then
nci_rsp_packet() calls skb_pull(skb, NCI_CTRL_HDR_SIZE) leaving
skb->len = 0, and s3fwrn5_nci_prop_rsp() accesses skb->data[0] on a
zero-length buffer.

The commit message justifies allowing zero-length for the NXP NTF case
(OID 0x23), which has a handler that doesn't access skb data. But the
implementation applies to ALL proprietary OIDs including RSP messages.

Would it be safer to either:
1) Restrict the zero-length exception to notification packets only
2) Check that all proprietary OID handlers can safely handle zero-length
3) Add a length check before skb->data[0] access in s3fwrn5_nci_prop_rsp()
-- 
pw-bot: cr