Re: [PATCH net 2/2] nfc: digital: check resp length in digital_tg_send_atr_res_complete()

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0

- [Medium] The commit message's justification ("The downstream handlers
  (digital_tg_recv_psl_req, digital_tg_recv_dep_req) each have their own…

Pre-existing issues:
- [Medium] Sibling instance of the same unchecked-length read remains in
  the same file: digital_in_recv_dep_res() reads resp->data[0] as the…
- [Medium] Frame-format disagreement kept (and re-written) by the patch:
  digital_tg_send_atr_res_complete() derives the SoD presence from the…

--

nfc: digital: check resp length in digital_tg_send_atr_res_complete()

This adds a resp->len check to digital_tg_send_atr_res_complete() before
reading resp->data[0] and resp->data[offset], and frees the skb and
restarts polling when the frame is too short.

A few questions below.

nfc: digital: check resp length in digital_tg_send_atr_res_complete()

    The downstream handlers (digital_tg_recv_psl_req, digital_tg_recv_dep_req)
    each have their own length checks, so the consequence is a misdirected
    dispatch on stale data rather than memory corruption.

[Severity: Medium]
Is this statement accurate for digital_tg_recv_dep_req()?  That handler
validates only resp->len >= sizeof(struct digital_dep_req_res), i.e. 3
bytes (dir, cmd, pfb), and then reads byte 3 before any further length
test:

net/nfc/digital_dep.c:digital_tg_recv_dep_req() {
	...
	size = sizeof(struct digital_dep_req_res);
	dep_req = (struct digital_dep_req_res *)resp->data;

	if (resp->len < size || dep_req->dir != DIGITAL_NFC_DEP_FRAME_DIR_OUT ||
	    dep_req->cmd != DIGITAL_CMD_DEP_REQ) {
		rc = -EIO;
		goto exit;
	}

	pfb = dep_req->pfb;

	if (DIGITAL_NFC_DEP_DID_BIT_SET(pfb)) {
		if (ddev->did && (ddev->did == resp->data[3])) {
	...
}

The "if (size > resp->len)" test that would cover byte 3 runs only after
that comparison, so it does not guard the read.

The precondition is a nonzero ddev->did, which the remote peer sets in
digital_tg_recv_atr_req():

	ddev->did = atr_req->did;

so a peer that negotiates a DID and then sends a 3-byte DEP_REQ payload
with the DID bit set in PFB makes digital_tg_recv_dep_req() read
resp->data[3] one byte past resp->len.  Should the check also be extended
there, or should the changelog drop the claim that the downstream handlers
cover this?

[Severity: Medium]
This isn't a bug introduced by this patch, since the byte-value heuristic
is only reformatted into a ternary here, but while this offset computation
is being rewritten, would it be better to derive the SoD presence from
ddev->curr_rf_tech the way the encoder/decoder pair does?

net/nfc/digital_dep.c:digital_skb_push_dep_sod() {
	if (ddev->curr_rf_tech == NFC_DIGITAL_RF_TECH_106A)
		*(u8 *)skb_push(skb, sizeof(u8)) = DIGITAL_NFC_DEP_NFCA_SOD_SB;
}

net/nfc/digital_dep.c:digital_skb_pull_dep_sod() {
	if (ddev->curr_rf_tech == NFC_DIGITAL_RF_TECH_106A)
		skb_pull(skb, sizeof(u8));
}

In 212F/424F target mode there is no SB byte, so data[0] is the LEN byte.
Can a LEN of 0xF0 (240, legal since local_payload_max is
DIGITAL_PAYLOAD_SIZE_MAX) make this pick offset 3 and read the PFB byte as
the command byte?

DIGITAL_CMD_PSL_REQ is 0x04 and DIGITAL_NFC_DEP_PFB_DID_BIT is also 0x04,
so a first DEP_REQ (I-PDU, PNI 0, DID in use) of exactly that size would
have PFB == 0x04 and be dispatched to digital_tg_recv_psl_req(), which
then drops it:

net/nfc/digital_dep.c:digital_tg_recv_psl_req() {
	if (resp->len != sizeof(struct digital_psl_req) ||
	...
		rc = -EIO;
		goto exit;
	}
}

Does that stall the DEP session, since the frame is freed with no reply?
The new bounds check is computed from the same offset, so it inherits the
disagreement.

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but there
is a sibling unchecked read of the same kind in the same file, in
digital_in_recv_dep_res().  Would it make sense to cover it in the same
series?

net/nfc/digital_dep.c:digital_in_recv_dep_res() {
	...
	if (size > resp->len) {
		rc = -EIO;
		goto error;
	}

	skb_pull(resp, size);
	...
	case DIGITAL_NFC_DEP_PFB_SUPERVISOR_PDU:
	...
		rtox = DIGITAL_NFC_DEP_RTOX_VALUE(resp->data[0]);
}

With a 3-byte DEP_RES payload (dir, cmd, pfb and no DID/NAD bit), size is
3, so both "resp->len < size" and "size > resp->len" pass, and the
skb_pull() leaves resp->len == 0.  The SUPERVISOR_PDU plus timeout-bit
branch then reads resp->data[0] one byte past resp->len and feeds the
masked value to digital_in_send_rtox() as a timeout.  Both ddev->atn_count
and ddev->nack_count are zeroed just above, so the branch is reachable.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912131852.1651462-1-liuc63%40xiaopeng.com