* Re: [PATCH] nfc: digital: check length before reading SENS_REQ
2026-11-08 22:30 [PATCH] nfc: digital: check length before reading SENS_REQ Noah Weinberger
@ 2026-11-09 23:18 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 23:18 UTC (permalink / raw)
To: NoahWeinberger2003
Cc: david, davem, edumazet, kuba, pabeni, netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The commit message of "nfc: digital: check length before
reading SENS_REQ" has no Signed-off-by: trailer.
- [Low] The patch is presented as a bug fix ("an empty frame reads one
byte past skb->len"), but it changes nothing observable.
- [Low] The patch is framed as a fix for an overread in
digital_tg_recv_sens_req() (net/nfc/digital_technology.c) but has no…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a sibling
target-mode receive callback has the same read-before-length-check…
- [Low] This problem wasn't introduced by this patch, but
digital_in_recv_dep_res() (net/nfc/digital_dep.c) reads the RTOX byte…
--
nfc: digital: check length before reading SENS_REQ
This patch reorders the check in digital_tg_recv_sens_req() so that
resp->len is tested before resp->data[0] is read. It also drops the
now-unused sens_req local.
> Hello,
>
[ ... ]
> Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07), net/nfc/digital_technology.c:1088-1112 (digital_tg_recv_sens_req).
> Longstanding code.
>
> This was found during an assisted (AI-supported) static source review of
> mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
> reporter against that tree.
[ ... ]
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
[Severity: Medium]
Is a Signed-off-by: missing here? The only trailer is Reported-by:, and it
names the author, who is already credited in From:.
Documentation/process/submitting-patches.rst requires a DCO sign-off, so the
patch can't be applied as submitted.
The body also has text that probably doesn't belong in git history:
- the "Hello," greeting
- the "Affected version/commit:" line with file:line ranges
- "every code path cited below was re-verified", although nothing is
cited below
- the note that the patch has not been compiled or runtime tested
- the request to treat this as a report for verification
Could these move below the --- line or into a cover letter?
> diff --git a/net/nfc/digital_technology.c b/net/nfc/digital_technology.c
> index ae6487c10a254..d9a8e9fc3d846 100644
> --- a/net/nfc/digital_technology.c
> +++ b/net/nfc/digital_technology.c
> @@ -1088,7 +1088,6 @@ static int digital_tg_send_sens_res(struct nfc_digital_dev *ddev)
> void digital_tg_recv_sens_req(struct nfc_digital_dev *ddev, void *arg,
> struct sk_buff *resp)
[Severity: Low]
If this stays framed as a fix, should it have a Fixes: tag? git blame shows
that digital_tg_recv_sens_req(), including the read before the length
check, came from:
Fixes: 1c7a4c24fbfd ("NFC Digital: Add target NFC-DEP support")
If it is reworded as a cleanup instead, no Fixes: tag is needed.
> {
> - u8 sens_req;
> int rc;
[ ... ]
> @@ -1097,10 +1096,8 @@ void digital_tg_recv_sens_req(struct nfc_digital_dev *ddev, void *arg,
> goto exit;
> }
>
> - sens_req = resp->data[0];
> -
> - if (!resp->len || (sens_req != DIGITAL_CMD_SENS_REQ &&
> - sens_req != DIGITAL_CMD_ALL_REQ)) {
> + if (!resp->len || (resp->data[0] != DIGITAL_CMD_SENS_REQ &&
> + resp->data[0] != DIGITAL_CMD_ALL_REQ)) {
[Severity: Low]
Does this change any behaviour? In the old code, when resp->len is 0 the ||
short-circuits on !resp->len. So the byte loaded into sens_req never
affects control flow, and both versions return -EINVAL and call
digital_poll_next_tech().
For a linear skb with len == 0, data == tail <= end. The old load stayed
inside the head allocation and could not fault.
The commit message says "so an empty frame reads one byte past skb->len".
Together with the "Affected version/commit:" line, this reads like a
memory-safety fix. Could the changelog say plainly that this is a cleanup
with no functional change?
> rc = -EINVAL;
> goto exit;
> }
[Severity: Medium]
This isn't a bug introduced by this patch, but a sibling target-mode
callback in net/nfc/digital_dep.c has the same read-before-length-check
pattern. There, the byte it reads is actually used:
net/nfc/digital_dep.c:digital_tg_send_atr_res_complete() {
...
offset = 2;
if (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB)
offset++;
ddev->atn_count = 0;
if (resp->data[offset] == DIGITAL_CMD_PSL_REQ)
digital_tg_recv_psl_req(ddev, arg, resp);
else
digital_tg_recv_dep_req(ddev, arg, resp);
}
Only IS_ERR(resp) is checked before these reads. A remote initiator can
send an empty or 1-3 byte frame after ATR_RES. That reads up to 4 bytes
past skb->len, and the result decides which handler runs.
The reads stay inside the skb head allocation. Both handlers later reject
short frames through skb_check_crc() and digital_skb_pull_dep_sod(). So the
visible effect seems to be a branch on uninitialized tailroom, which KMSAN
could report as uninit-value.
Should this function get a length check too, like the !resp->len check in
digital_tg_recv_atr_req()?
[Severity: Low]
This is a pre-existing issue, but digital_in_recv_dep_res() in
net/nfc/digital_dep.c reads the RTOX byte without checking that a byte is
left after the header pull:
net/nfc/digital_dep.c:digital_in_recv_dep_res() {
...
if (size > resp->len) {
rc = -EIO;
goto error;
}
skb_pull(resp, size);
...
rtox = DIGITAL_NFC_DEP_RTOX_VALUE(resp->data[0]);
if (!rtox || rtox > DIGITAL_NFC_DEP_RTOX_MAX) {
...
}
Here size is sizeof(struct digital_dep_req_res), which is 3. If a remote
target sends a header-only RTOX DEP_RES with the timeout supervisor bit
set, resp->len is 0 at the read.
With software CRC, data[0] is the first CRC byte that was just trimmed.
With driver-side CRC (NFC_DIGITAL_DRV_CAPS_IN_CRC), it may be
uninitialized tailroom.
If that byte & 0x3F falls in 1..DIGITAL_NFC_DEP_RTOX_MAX, can the malformed
frame be accepted as a valid RTOX request and lead to a call to
digital_in_send_rtox()?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/i69izu1c4hLS_ZLvaL0RVyF5pJt0gkMCEY-87g-j6iGMyCi9WKPGInJF53uwP9q9k4IPeEoBS6RKRCNaavGJlY7G-ifRKqdeMSC0rzYf9Tg%3D%40proton.me
^ permalink raw reply [flat|nested] 2+ messages in thread