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

[Ian Ray <ian.ray@gehealthcare.com>]

On Fri, Feb 20, 2026 at 01:34:19PM -0800, Jakub Kicinski wrote:
> CAUTION: This email originated from outside of GE HealthCare. Only open links or attachments if you trust the sender. Report suspicious emails using Outlook’s “Report” button.
> 
> 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?

Good catch; the proposed change impacts:

drivers/nfc/st-nci/core.c : st_nci_prop_rsp_packet

drivers/nfc/s3fwrn5/nci.c : s3fwrn5_nci_prop_rsp

drivers/nfc/nxp-nci/core.c : n/a

drivers/nfc/fdp/fdp.c : fdp_nci_core_get_config_rsp_packet
drivers/nfc/fdp/fdp.c : fdp_nci_prop_patch_rsp_packet
drivers/nfc/fdp/fdp.c : fdp_nci_prop_patch_ntf_packet
drivers/nfc/fdp/fdp.c : fdp_nci_prop_set_production_data_rsp_packet

Restricting the patch to NTF only would make it safer and less
intrusive; I will do this in V2.

> 
> 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