[PATCH] net/nfc: bound SENSF response copy length

[PATCH] net/nfc: bound SENSF response copy length

[Pengpeng Hou]

`digital_in_recv_sensf_res()` only checks `resp->len` against the
minimum frame size before copying the response into
`target.sensf_res`. The destination is `NFC_SENSF_RES_MAXSIZE` bytes
long, so an oversized SENSF response can overwrite adjacent fields in
the stack-local `struct nfc_target` before the result is handed to
`digital_target_found()`.

Reject frames larger than the destination buffer before copying.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 net/nfc/digital_technology.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/nfc/digital_technology.c b/net/nfc/digital_technology.c
index 63f1b721c71d..8147e61c224a 100644
--- a/net/nfc/digital_technology.c
+++ b/net/nfc/digital_technology.c
@@ -768,6 +768,11 @@ static void digital_in_recv_sensf_res(struct nfc_digital_dev *ddev, void *arg,
 
 	skb_pull(resp, 1);
 
+	if (resp->len > NFC_SENSF_RES_MAXSIZE) {
+		rc = -EIO;
+		goto exit;
+	}
+
 	memset(&target, 0, sizeof(struct nfc_target));
 
 	sensf_res = (struct digital_sensf_res *)resp->data;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] net/nfc: bound SENSF response copy length

[Simon Horman]

On Sun, Mar 22, 2026 at 11:19:22AM +0800, Pengpeng Hou wrote:
> `digital_in_recv_sensf_res()` only checks `resp->len` against the
> minimum frame size before copying the response into
> `target.sensf_res`. The destination is `NFC_SENSF_RES_MAXSIZE` bytes
> long, so an oversized SENSF response can overwrite adjacent fields in
> the stack-local `struct nfc_target` before the result is handed to
> `digital_target_found()`.
> 
> Reject frames larger than the destination buffer before copying.
> 

Hi Pengpeng Hou,

A fixes tag needs to go here (without a blank line between it
and your Signed-off-by line).

Possibly the following is appropriate

Fixes: 8c0695e4998d ("NFC Digital: Add NFC-F technology support")

> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

This patch should be targeted at the net tree, like this:

Subject: [PATCH net] ...

And, looking at git history, it seems that an appropriate prefix
is "NFC: digital:"

Subject: [PATCH net] NFC: digital: bound SENSF response copy length

> ---
>  net/nfc/digital_technology.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/nfc/digital_technology.c b/net/nfc/digital_technology.c
> index 63f1b721c71d..8147e61c224a 100644
> --- a/net/nfc/digital_technology.c
> +++ b/net/nfc/digital_technology.c
> @@ -768,6 +768,11 @@ static void digital_in_recv_sensf_res(struct nfc_digital_dev *ddev, void *arg,
>  
>  	skb_pull(resp, 1);
>  
> +	if (resp->len > NFC_SENSF_RES_MAXSIZE) {
> +		rc = -EIO;
> +		goto exit;
> +	}
> +
>  	memset(&target, 0, sizeof(struct nfc_target));
>  
>  	sensf_res = (struct digital_sensf_res *)resp->data;

AI generated review at sashiko.dev says:

    Does this strict bounds check drop valid NFC-F (FeliCa) SENSF_RES
    frames that contain the Request Data (RD) field?

    According to the NFC Forum Digital Protocol, a SENSF_RES frame payload
    can include an optional 2-byte Request Data (RD) field, bringing the
    total length of the response to 19 bytes. This matches the kernel's
    definition of

    struct digital_sensf_res which is exactly 19 bytes long:

    struct digital_sensf_res {
    	u8 cmd;
    	u8 nfcid2[8];
    	u8 pad0[2];
    	u8 pad1[3];
    	u8 mrti_check;
    	u8 mrti_update;
    	u8 pad2;
    	u8 rd[2];
    }

    However, the UAPI macro NFC_SENSF_RES_MAXSIZE is defined as 18.

    If a valid FeliCa SENSF_RES with RD arrives, resp->len will be 19.
    Returning -EIO here completely drops these frames. Could we instead
    clamp the copy length to NFC_SENSF_RES_MAXSIZE (truncating the optional
    RD field if the UAPI buffer cannot be extended) to avoid breaking
    functionality?

Which does seem to be factually correct (please check).
And perhaps the current code buffer overruns if RD is present (in full).
If so it would be interesting to analyse what the effect of that is:
is the neighbouring data unused so this works; or has it always been broken?

Also, I wonder if it is possible to expand NFC_SENSF_RES_MAXSIZE to
accomodate all of a struct nfc_target.

But I am also curious to know how the skb_pull() in the code relates to:

1. Using the packet data as a struct nfc_target after pulling the first byte
2. But checking the length of packet data against
   DIGITAL_SENSF_RES_MIN_LENGTH (17) before the call to skb_pull()

[PATCH net v2] NFC: digital: bound SENSF response copy into nfc_target

[Pengpeng Hou]

digital_in_recv_sensf_res() copies the received SENSF response into
struct nfc_target without bounding the copy to target.sensf_res. A full
on-wire digital_sensf_res is 19 bytes long, while nfc_target stores 18
bytes, so oversized or full-length frames can overwrite adjacent stack
fields before digital_target_found() sees the target.

Reject payloads larger than struct digital_sensf_res and clamp the copy
into target.sensf_res so valid 19-byte responses keep working while the
fixed destination buffer stays bounded.

Fixes: 8c0695e4998dd268ff2a05951961247b7e015651 ("NFC Digital: Add NFC-F technology support")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- target the net tree and use the NFC: digital: prefix
- add the missing Fixes tag
- preserve valid 19-byte SENSF responses by clamping the copy into
  struct nfc_target
- reject only payloads larger than struct digital_sensf_res

net/nfc/digital_technology.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/nfc/digital_technology.c b/net/nfc/digital_technology.c
index 63f1b721c71d..abf544d917f3 100644
--- a/net/nfc/digital_technology.c
+++ b/net/nfc/digital_technology.c
@@ -767,13 +767,18 @@ static void digital_in_recv_sensf_res(struct nfc_digital_dev *ddev, void *arg,
 	}
 
 	skb_pull(resp, 1);
+	if (resp->len > sizeof(struct digital_sensf_res)) {
+		rc = -EIO;
+		goto exit;
+	}
 
 	memset(&target, 0, sizeof(struct nfc_target));
 
 	sensf_res = (struct digital_sensf_res *)resp->data;
 
-	memcpy(target.sensf_res, sensf_res, resp->len);
-	target.sensf_res_len = resp->len;
+	target.sensf_res_len = min_t(unsigned int, resp->len,
+				     sizeof(target.sensf_res));
+	memcpy(target.sensf_res, sensf_res, target.sensf_res_len);
 
 	memcpy(target.nfcid2, sensf_res->nfcid2, NFC_NFCID2_MAXSIZE);
 	target.nfcid2_len = NFC_NFCID2_MAXSIZE;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH net v2] NFC: digital: bound SENSF response copy into nfc_target

[Jakub Kicinski]

On Tue, 7 Apr 2026 09:57:36 +0800 Pengpeng Hou wrote:
> digital_in_recv_sensf_res() copies the received SENSF response into
> struct nfc_target without bounding the copy to target.sensf_res. A full
> on-wire digital_sensf_res is 19 bytes long, while nfc_target stores 18
> bytes, so oversized or full-length frames can overwrite adjacent stack
> fields before digital_target_found() sees the target.
> 
> Reject payloads larger than struct digital_sensf_res and clamp the copy
> into target.sensf_res so valid 19-byte responses keep working while the
> fixed destination buffer stays bounded.

You need to solve the riddle why this driver thinks the response is 19
bytes but the core wants to store only 18...

> Fixes: 8c0695e4998dd268ff2a05951961247b7e015651 ("NFC Digital: Add NFC-F technology support")

nit: the hash in the Fixes tag should be only 12 chars
-- 
pw-bot: cr

Re: [PATCH net v2] NFC: digital: bound SENSF response copy into nfc_target

[Pengpeng Hou]

Hi Jakub,

Thanks, that makes sense.

I won't resend another bounds-only version as-is. I'll first dig into
why the digital path uses a 19-byte `struct digital_sensf_res` while
the core/UAPI path only carries 18 bytes in `sensf_res`, and follow up
once I have a clearer explanation for what the driver/core boundary
should be here. I'll also shorten the `Fixes:` hash in the next
revision.

Thanks,
Pengpeng

Re: [PATCH] net/nfc: bound SENSF response copy length

[Pengpeng Hou]

Hi Simon,

Thanks, you're right about the net targeting, the NFC: digital:
prefix, and the missing Fixes tag.

You are also right that a valid full SENSF_RES can be 19 bytes long.
So instead of rejecting resp->len > NFC_SENSF_RES_MAXSIZE, v2 only
rejects payloads larger than struct digital_sensf_res, then clamps the
copy into the 18-byte sensf_res buffer inside struct nfc_target.

That keeps valid 19-byte responses working while still fixing the stack
overwrite in the target copy path. The lower-bound check remains on the
pre-skb_pull() frame length, and v2 only adds the post-pull upper bound
before treating the payload as struct digital_sensf_res.

Thanks,
Pengpeng