[PATCH 0/2] nfc: pn533: bound device-supplied lengths in the receive path

[PATCH 0/2] nfc: pn533: bound device-supplied lengths in the receive path

[Michael Bommarito]

The PN533 receive path trusts several device-supplied length fields and
reads past the received buffer when they exceed it. The receive skb is
sized to the device's USB transfer length, so a malicious or counterfeit
PN53x reader triggers out-of-bounds reads in the standard/ACR122 frame
validators, in Type A target parsing (nfcid_len), and in the autopoll
record walk.

Michael Bommarito (2):
  nfc: pn533: bound device-supplied lengths in the receive path
  nfc: pn533: add KUnit tests for receive-path bounds

 drivers/nfc/pn533/Kconfig |   9 +++
 drivers/nfc/pn533/pn533.c | 125 +++++++++++++++++++++++++++++++++++++-
 drivers/nfc/pn533/pn533.h |   3 +-
 drivers/nfc/pn533/usb.c   |  12 +++-
 4 files changed, 144 insertions(+), 5 deletions(-)

-- 
2.53.0

[PATCH 1/2] nfc: pn533: bound device-supplied lengths in the receive path

[Michael Bommarito]

The PN533 receive path trusts device-supplied length fields and reads
past the received buffer when they exceed it. The rx skb is sized to the
device's USB transfer length, so a malicious PN53x reader triggers
out-of-bounds reads in the standard/ACR122 frame validators, in Type A
target parsing (nfcid_len), and in the autopoll record walk.

Thread the received length into the rx_is_frame_valid callbacks, reject
ACR122 frames with datalen < 2, and add the missing nfcid and per-record
bounds.

This was found with static analysis and confirmed with the KUnit tests
added in the following patch.

Fixes: c46ee38620a2 ("NFC: pn533: add NXP pn533 nfc device driver")
Fixes: 9815c7cf22da ("NFC: pn533: Separate physical layer from the core implementation")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/nfc/pn533/pn533.c | 32 ++++++++++++++++++++++++++++++--
 drivers/nfc/pn533/pn533.h |  3 ++-
 drivers/nfc/pn533/usb.c   | 12 ++++++++++--
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d7bdbc82e2ba2..984a61a83feaf 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -274,11 +274,15 @@ static void pn533_std_tx_update_payload_len(void *_frame, int len)
 	frame->datalen += len;
 }
 
-static bool pn533_std_rx_frame_is_valid(void *_frame, struct pn533 *dev)
+static bool pn533_std_rx_frame_is_valid(void *_frame, size_t frame_len,
+					struct pn533 *dev)
 {
 	u8 checksum;
 	struct pn533_std_frame *stdf = _frame;
 
+	if (frame_len < sizeof(struct pn533_std_frame))
+		return false;
+
 	if (stdf->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF))
 		return false;
 
@@ -286,6 +290,10 @@ static bool pn533_std_rx_frame_is_valid(void *_frame, struct pn533 *dev)
 		/* Standard frame code */
 		dev->ops->rx_header_len = PN533_STD_FRAME_HEADER_LEN;
 
+		if (frame_len < sizeof(struct pn533_std_frame) +
+				stdf->datalen + PN533_STD_FRAME_TAIL_LEN)
+			return false;
+
 		checksum = pn533_std_checksum(stdf->datalen);
 		if (checksum != stdf->datalen_checksum)
 			return false;
@@ -299,6 +307,11 @@ static bool pn533_std_rx_frame_is_valid(void *_frame, struct pn533 *dev)
 
 		dev->ops->rx_header_len = PN533_EXT_FRAME_HEADER_LEN;
 
+		if (frame_len < sizeof(struct pn533_ext_frame) ||
+		    frame_len < sizeof(struct pn533_ext_frame) +
+				be16_to_cpu(eif->datalen) + PN533_STD_FRAME_TAIL_LEN)
+			return false;
+
 		checksum = pn533_ext_checksum(be16_to_cpu(eif->datalen));
 		if (checksum != eif->datalen_checksum)
 			return false;
@@ -701,6 +714,10 @@ static bool pn533_target_type_a_is_valid(struct pn533_target_type_a *type_a,
 	if (type_a->nfcid_len > NFC_NFCID1_MAXSIZE)
 		return false;
 
+	if (sizeof(struct pn533_target_type_a) + type_a->nfcid_len >
+	    (size_t)target_data_len)
+		return false;
+
 	return true;
 }
 
@@ -1399,6 +1416,7 @@ static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
 {
 	struct pn532_autopoll_resp *apr;
 	struct nfc_target nfc_tgt;
+	size_t remaining;
 	u8 nbtg;
 	int rc;
 
@@ -1417,12 +1435,21 @@ static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
 		goto stop_poll;
 	}
 
+	if (resp->len < 1)
+		return -EAGAIN;
+
 	nbtg = resp->data[0];
 	if ((nbtg > 2) || (nbtg <= 0))
 		return -EAGAIN;
 
+	remaining = resp->len - 1;
 	apr = (struct pn532_autopoll_resp *)&resp->data[1];
 	while (nbtg--) {
+		if (remaining < sizeof(struct pn532_autopoll_resp) ||
+		    apr->ln < 1 ||
+		    remaining < (size_t)apr->ln + 2)
+			return -EAGAIN;
+
 		memset(&nfc_tgt, 0, sizeof(struct nfc_target));
 		switch (apr->type) {
 		case PN532_AUTOPOLL_TYPE_ISOA:
@@ -1468,6 +1495,7 @@ static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
 		}
 
 		dev->tgt_available_prots = nfc_tgt.supported_protocols;
+		remaining -= (size_t)apr->ln + 2;
 		apr = (struct pn532_autopoll_resp *)
 			(apr->tgdata + (apr->ln - 1));
 	}
@@ -2189,7 +2217,7 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
 	print_hex_dump_debug("PN533 RX: ", DUMP_PREFIX_NONE, 16, 1, skb->data,
 			     dev->ops->rx_frame_size(skb->data), false);
 
-	if (!dev->ops->rx_is_frame_valid(skb->data, dev)) {
+	if (!dev->ops->rx_is_frame_valid(skb->data, skb->len, dev)) {
 		nfc_err(dev->dev, "Received an invalid frame\n");
 		dev->cmd->status = -EIO;
 	} else if (!pn533_rx_frame_is_cmd_response(dev, skb->data)) {
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 09e35b8693f5a..08804f88b38a3 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -201,7 +201,8 @@ struct pn533_frame_ops {
 	int tx_header_len;
 	int tx_tail_len;
 
-	bool (*rx_is_frame_valid)(void *frame, struct pn533 *dev);
+	bool (*rx_is_frame_valid)(void *frame, size_t frame_len,
+				  struct pn533 *dev);
 	bool (*rx_frame_is_ack)(void *frame);
 	int (*rx_frame_size)(void *frame);
 	int rx_header_len;
diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index efb07f944fce2..1d50600609dd4 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -312,14 +312,22 @@ static void pn533_acr122_tx_update_payload_len(void *_frame, int len)
 	frame->datalen += len;
 }
 
-static bool pn533_acr122_is_rx_frame_valid(void *_frame, struct pn533 *dev)
+static bool pn533_acr122_is_rx_frame_valid(void *_frame, size_t frame_len,
+					   struct pn533 *dev)
 {
 	struct pn533_acr122_rx_frame *frame = _frame;
 
+	if (frame_len < sizeof(struct pn533_acr122_rx_frame))
+		return false;
+
 	if (frame->ccid.type != 0x83)
 		return false;
 
-	if (!frame->ccid.datalen)
+	if (frame->ccid.datalen < 2)
+		return false;
+
+	if (frame_len < sizeof(struct pn533_acr122_rx_frame) +
+			frame->ccid.datalen)
 		return false;
 
 	if (frame->data[frame->ccid.datalen - 2] == 0x63)
-- 
2.53.0

[PATCH 2/2] nfc: pn533: add KUnit tests for receive-path bounds

[Michael Bommarito]

Two regression tests: a header-only standard frame declaring datalen = 8
(KASAN slab-out-of-bounds in pn533_std_rx_frame_is_valid on an unpatched
tree), and a header-only Type A target with nfcid_len = 10
(pn533_target_found_type_a returns -EPROTO once bounded). Benign controls
exercise the valid path.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/nfc/pn533/Kconfig |  9 ++++
 drivers/nfc/pn533/pn533.c | 93 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
index 31822b432d51c..a0ebbb75419b1 100644
--- a/drivers/nfc/pn533/Kconfig
+++ b/drivers/nfc/pn533/Kconfig
@@ -37,3 +37,12 @@ config NFC_PN532_UART
 
 	  If you choose to build a module, it'll be called pn532_uart.
 	  Say N if unsure.
+
+config NFC_PN533_KUNIT_TEST
+	bool "KUnit tests for the NFC PN533 driver" if !KUNIT_ALL_TESTS
+	depends on NFC_PN533=y && KUNIT=y
+	default KUNIT_ALL_TESTS
+	help
+	  Enable the KUnit regression tests for the NFC PN533 driver,
+	  covering bounds checking of device-supplied frame lengths.
+	  If unsure, say N.
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 984a61a83feaf..47d2b4b56bc2e 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2883,3 +2883,96 @@ MODULE_AUTHOR("Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>");
 MODULE_DESCRIPTION("PN533 driver ver " VERSION);
 MODULE_VERSION(VERSION);
 MODULE_LICENSE("GPL");
+
+#if IS_ENABLED(CONFIG_NFC_PN533_KUNIT_TEST)
+#include <kunit/test.h>
+
+/*
+ * Regression test for the out-of-bounds read in the PN533 standard-frame
+ * validator: pn533_std_rx_frame_is_valid() trusts the device-supplied
+ * datalen field and reads data[0 .. datalen-1] (the data checksum) without
+ * checking that the received buffer actually contains that many bytes. A
+ * malicious USB PN53x device returning a short transfer therefore makes the
+ * driver read past the receive buffer.
+ */
+static void pn533_std_rx_frame_short_oob_test(struct kunit *test)
+{
+	struct pn533 *dev;
+	struct pn533_frame_ops *ops;
+	struct pn533_std_frame *f;
+	const size_t hdr = sizeof(struct pn533_std_frame); /* header, no data[] */
+
+	dev = kunit_kzalloc(test, sizeof(*dev), GFP_KERNEL);
+	ops = kunit_kzalloc(test, sizeof(*ops), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, dev);
+	KUNIT_ASSERT_NOT_NULL(test, ops);
+	dev->ops = ops;
+
+	/* benign control: a correctly sized, well-formed frame validates and
+	 * does not read out of bounds (proves the trigger reaches the real
+	 * data-checksum path). data is all zero: data checksum = 0, and the
+	 * trailing frame-checksum byte data[datalen] is already 0.
+	 */
+	{
+		const u8 dl = 3;
+		const size_t full = hdr + dl + PN533_STD_FRAME_TAIL_LEN;
+		struct pn533_std_frame *gf = kunit_kzalloc(test, full, GFP_KERNEL);
+
+		KUNIT_ASSERT_NOT_NULL(test, gf);
+		gf->start_frame = cpu_to_be16(PN533_STD_FRAME_SOF);
+		gf->datalen = dl;
+		gf->datalen_checksum = pn533_std_checksum(dl);
+		KUNIT_EXPECT_TRUE(test,
+				  pn533_std_rx_frame_is_valid(gf, full, dev));
+	}
+
+	/* attack: a header-only buffer that declares datalen = 8. The data
+	 * checksum loop reads data[0 .. 7] starting one byte past the
+	 * allocation -> KASAN slab-out-of-bounds read on an unpatched tree.
+	 */
+	f = kunit_kzalloc(test, hdr, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, f);
+	f->start_frame = cpu_to_be16(PN533_STD_FRAME_SOF);
+	f->datalen = 8;
+	f->datalen_checksum = pn533_std_checksum(8);
+	pn533_std_rx_frame_is_valid(f, hdr, dev);
+}
+
+static void pn533_target_type_a_nfcid_oob_test(struct kunit *test)
+{
+	struct nfc_target nfc_tgt;
+	struct pn533_target_type_a *ta;
+	const size_t hdr = sizeof(struct pn533_target_type_a); /* 4, no nfcid */
+
+	/* header-only target data, but nfcid_len claims the full 10 bytes ->
+	 * pn533_target_found_type_a()'s memcpy reads nfcid_data[0..9], past the
+	 * allocation -> KASAN slab-out-of-bounds on an unpatched tree.
+	 */
+	ta = kunit_kzalloc(test, hdr, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ta);
+	ta->sens_res = cpu_to_be16(0x0001);
+	ta->sel_res = 0;
+	ta->nfcid_len = NFC_NFCID1_MAXSIZE; /* 10 */
+
+	/* unpatched: is_valid accepts (nfcid_len only checked vs the
+	 * destination) and the memcpy reads nfcid_data[0..9] past the 4-byte
+	 * buffer. The fix bounds nfcid_len against the response buffer and
+	 * rejects the frame, returning -EPROTO.
+	 */
+	KUNIT_EXPECT_EQ(test,
+			pn533_target_found_type_a(&nfc_tgt, (u8 *)ta, hdr),
+			-EPROTO);
+}
+
+static struct kunit_case pn533_test_cases[] = {
+	KUNIT_CASE(pn533_std_rx_frame_short_oob_test),
+	KUNIT_CASE(pn533_target_type_a_nfcid_oob_test),
+	{}
+};
+
+static struct kunit_suite pn533_test_suite = {
+	.name = "pn533",
+	.test_cases = pn533_test_cases,
+};
+kunit_test_suite(pn533_test_suite);
+#endif /* CONFIG_NFC_PN533_KUNIT_TEST */
-- 
2.53.0