[PATCH] nfc: nci: Fix zero-length proprietary OIDs

[PATCH] nfc: nci: Fix zero-length proprietary OIDs

[Ian Ray]

NCI NFC controllers may have proprietary OIDs with zero-length payload.
One example is [1], [2].

Allow a zero length payload in proprietary OIDs *only* - note that this
change does *not* impact standard OIDs (which must have non-zero length
due to the earlier uninit-value fix).

Before:

-- >8 --
kernel: nci: nci_recv_frame: len 3
-- >8 --

After:

-- >8 --
kernel: nci: nci_recv_frame: len 3
kernel: nci: nci_ntf_packet: NCI RX: MT=ntf, PBF=0, GID=0x1, OID=0x23, plen=0
kernel: nci: nci_ntf_packet: unknown ntf opcode 0x123
kernel: nfc nfc0: NFC: RF transmitter couldn't start. Bad power and/or configuration?
-- >8 --

[1] drivers/nfc/nxp-nci/core.c
[2] NXP_NCI_RF_TXLDO_ERROR_NTF

Fixes: d24b03535e5e ("nfc: nci: Fix uninit-value in nci_dev_up and nci_ntf_packet")
Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
 net/nfc/nci/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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;
+
 	return true;
 }
 
-- 
2.49.0

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

[Jakub Kicinski]

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

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

[Ian Ray]

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

Re: [PATCH] nfc: nci: Fix zero-length proprietary OIDs

[Jakub Kicinski]

On Fri, 20 Feb 2026 13:25:35 +0200 Ian Ray wrote:
> Before:
> 
> -- >8 --  
> kernel: nci: nci_recv_frame: len 3
> -- >8 --  
> 
> After:
> 
> -- >8 --  
> kernel: nci: nci_recv_frame: len 3
> kernel: nci: nci_ntf_packet: NCI RX: MT=ntf, PBF=0, GID=0x1, OID=0x23, plen=0
> kernel: nci: nci_ntf_packet: unknown ntf opcode 0x123
> kernel: nfc nfc0: NFC: RF transmitter couldn't start. Bad power and/or configuration?
> -- >8 --  

FWIW the last message in "After" still doesn't sound particularly happy
so it may be worth explaining why this makes the device usable ;)
If there's more that needs to be fixed for the device in question to
work it'd be best to have all the necessary fixes in one series.

> [1] drivers/nfc/nxp-nci/core.c
> [2] NXP_NCI_RF_TXLDO_ERROR_NTF

Re: [PATCH] nfc: nci: Fix zero-length proprietary OIDs

[Ian Ray]

On Fri, Feb 20, 2026 at 01:36:01PM -0800, Jakub Kicinski wrote:
> On Fri, 20 Feb 2026 13:25:35 +0200 Ian Ray wrote:
> > Before:
> >
> > -- >8 --
> > kernel: nci: nci_recv_frame: len 3
> > -- >8 --
> >
> > After:
> >
> > -- >8 --
> > kernel: nci: nci_recv_frame: len 3
> > kernel: nci: nci_ntf_packet: NCI RX: MT=ntf, PBF=0, GID=0x1, OID=0x23, plen=0
> > kernel: nci: nci_ntf_packet: unknown ntf opcode 0x123
> > kernel: nfc nfc0: NFC: RF transmitter couldn't start. Bad power and/or configuration?
> > -- >8 --
> 
> FWIW the last message in "After" still doesn't sound particularly happy
> so it may be worth explaining why this makes the device usable ;)

The theory here is that we use +3.3V to power pn7160, but maybe the
evaluation kit requires a higher supply.  We will test and confirm this
before sending V2.

> If there's more that needs to be fixed for the device in question to
> work it'd be best to have all the necessary fixes in one series.
> 
> > [1] drivers/nfc/nxp-nci/core.c
> > [2] NXP_NCI_RF_TXLDO_ERROR_NTF
> 

Thanks,
Ian