[PATCH 0/3] xilinx: fru: harden FRU parsing against malformed data

[PATCH 0/3] xilinx: fru: harden FRU parsing against malformed data

[Michal Simek]

Hi,

this series fixes out-of-bounds reads in FRU parsing when handling
malformed or corrupted FRU data.

Thanks,
Michal


Pranav Tilak (3):
  xilinx: fru: Add bounds checks when parsing FRU data
  xilinx: fru: Fix wrong argv index in do_fru_capture() addr check
  xilinx: fru: Use sizeof() for board header size

 board/xilinx/common/board.c   |   7 ++-
 board/xilinx/common/fru.c     |  14 +++--
 board/xilinx/common/fru.h     |   5 +-
 board/xilinx/common/fru_ops.c | 103 +++++++++++++++++++++++++++-------
 4 files changed, 102 insertions(+), 27 deletions(-)

---
base-commit: c859517f0606595da489b5762844292e28c948fa

-- 
2.43.0

[PATCH 1/3] xilinx: fru: Add bounds checks when parsing FRU data

[Michal Simek]

From: Pranav Tilak <pranav.vinaytilak@amd.com>

fru_capture() reads hdr->off_board and hdr->off_multirec and uses
them to locate FRU areas without checking whether the resulting
offsets are within the captured buffer. The board and multirecord
parsers then trust lengths read from the FRU contents, which can
cause reads beyond the end of the buffer when processing a malformed
or corrupted FRU image.

Pass the buffer size through the parsing path and validate that FRU
headers, areas and fields fit within the supplied buffer, and that
the board area meets the FRU spec's minimum size, before accessing
them. board.c passes the size of the probed EEPROM; the fru capture
command now requires an explicit <size> argument.

Fixes: f1b97b5f18a8 ("xilinx: cmd: Add support for FRU commands")
Signed-off-by: Pranav Tilak <pranav.vinaytilak@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 board/xilinx/common/board.c   |  7 ++-
 board/xilinx/common/fru.c     | 12 +++--
 board/xilinx/common/fru.h     |  5 +-
 board/xilinx/common/fru_ops.c | 98 ++++++++++++++++++++++++++++-------
 4 files changed, 98 insertions(+), 24 deletions(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
index ea438472696d..790494f9b388 100644
--- a/board/xilinx/common/board.c
+++ b/board/xilinx/common/board.c
@@ -234,7 +234,12 @@ static int xilinx_read_eeprom_fru(struct udevice *dev, char *name,
 		goto end;
 	}
 
-	fru_capture((unsigned long)fru_content);
+	ret = fru_capture((unsigned long)fru_content, eeprom_size);
+	if (ret) {
+		printf("%s: FRU capture failed\n", __func__);
+		goto end;
+	}
+
 	if (gd->flags & GD_FLG_RELOC || (_DEBUG && IS_ENABLED(CONFIG_DTB_RESELECT))) {
 		printf("Xilinx I2C FRU format at %s:\n", name);
 		ret = fru_display(0);
diff --git a/board/xilinx/common/fru.c b/board/xilinx/common/fru.c
index 8cf307e33f20..de165c548592 100644
--- a/board/xilinx/common/fru.c
+++ b/board/xilinx/common/fru.c
@@ -14,6 +14,7 @@ static int do_fru_capture(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	unsigned long addr;
 	char *endp;
+	u32 size;
 
 	if (argc < cmdtp->maxargs)
 		return CMD_RET_USAGE;
@@ -22,7 +23,11 @@ static int do_fru_capture(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (*argv[1] == 0 || *endp != 0)
 		return -1;
 
-	return fru_capture(addr);
+	size = hextoul(argv[3], &endp);
+	if (*argv[3] == 0 || *endp != 0)
+		return -1;
+
+	return fru_capture(addr, size);
 }
 
 static int do_fru_display(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -46,7 +51,7 @@ static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,
 }
 
 static struct cmd_tbl cmd_fru_sub[] = {
-	U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
+	U_BOOT_CMD_MKENT(capture, 4, 0, do_fru_capture, "", ""),
 	U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
 	U_BOOT_CMD_MKENT(board_gen, 8, 0, do_fru_generate, "", ""),
 };
@@ -72,7 +77,8 @@ static int do_fru(struct cmd_tbl *cmdtp, int flag, int argc,
 
 /***************************************************/
 U_BOOT_LONGHELP(fru,
-	"capture <addr> - Parse and capture FRU table present at address.\n"
+	"capture <addr> <size> - Parse and capture FRU table present at\n"
+	"              <addr>. <size> is the buffer length in bytes.\n"
 	"fru display - Displays content of FRU table that was captured using\n"
 	"              fru capture command\n"
 	"fru board_gen <addr> <manufacturer> <board name> <serial number>\n"
diff --git a/board/xilinx/common/fru.h b/board/xilinx/common/fru.h
index 2b3fa05a6136..f61c3b31df72 100644
--- a/board/xilinx/common/fru.h
+++ b/board/xilinx/common/fru.h
@@ -98,8 +98,11 @@ struct fru_table {
 #define FRU_TYPELEN_TYPE_BINARY		0
 #define FRU_TYPELEN_TYPE_ASCII8		3
 
+/* FRU spec minimum board area size */
+#define FRU_BOARD_AREA_MIN_LEN		16
+
 int fru_display(int verbose);
-int fru_capture(unsigned long addr);
+int fru_capture(unsigned long addr, u32 size);
 int fru_generate(unsigned long addr, char *manufacturer, char *board_name,
 		 char *serial_no, char *part_no, char *revision);
 u8 fru_checksum(u8 *addr, u8 len);
diff --git a/board/xilinx/common/fru_ops.c b/board/xilinx/common/fru_ops.c
index 24b135f7666c..fb88c21cdf6f 100644
--- a/board/xilinx/common/fru_ops.c
+++ b/board/xilinx/common/fru_ops.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * (C) Copyright 2019 - 2022, Xilinx, Inc.
- * (C) Copyright 2022 - 2023, Advanced Micro Devices, Inc.
+ * (C) Copyright 2022 - 2026, Advanced Micro Devices, Inc.
  */
 
 #include <cpu_func.h>
@@ -166,19 +166,37 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name,
 	return 0;
 }
 
-static int fru_parse_board(unsigned long addr)
+static int fru_parse_board(unsigned long addr, u32 size)
 {
+	u8 *data, *term, *limit, *next_addr, *eof, *end;
+	u32 area_len;
 	u8 i, type;
 	int len;
-	u8 *data, *term, *limit, *next_addr, *eof;
+
+	if (size < sizeof(struct fru_board_info_header)) {
+		printf("%s Board area header out of range\n", __func__);
+		return -EINVAL;
+	}
 
 	memcpy(&fru_data.brd.ver, (void *)addr, 6);
 
+	area_len = fru_data.brd.len * FRU_COMMON_HDR_LEN_MULTIPLIER;
+	if (area_len > size) {
+		printf("%s Board area length out of range\n", __func__);
+		return -EINVAL;
+	}
+
+	if (area_len < FRU_BOARD_AREA_MIN_LEN) {
+		printf("%s Board area too small\n", __func__);
+		return -EINVAL;
+	}
+
 	/*
 	 * eof marks the last data byte (without checksum). That's why checksum
 	 * is address length - 1 and last data byte is length - 2.
 	 */
-	eof = (u8 *)(fru_data.brd.len * 8 + addr - 2);
+	eof = (u8 *)(area_len + addr - 2);
+	end = (u8 *)addr + size;
 
 	addr += 6;
 	data = (u8 *)&fru_data.brd.manufacturer_type_len;
@@ -187,25 +205,32 @@ static int fru_parse_board(unsigned long addr)
 	limit = (u8 *)&fru_data.brd + sizeof(struct fru_board_data);
 
 	for (i = 0; ; i++, data += FRU_BOARD_MAX_LEN) {
-		len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code,
-					 &type);
-		next_addr = (u8 *)addr + 1;
-
-		if ((u8 *)addr >= eof) {
+		if ((u8 *)addr >= eof || (u8 *)addr >= end) {
 			debug("Reach EOF record: addr %lx, eof %lx\n", addr,
 			      (unsigned long)eof);
 			break;
 		}
 
+		len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code,
+					 &type);
+		next_addr = (u8 *)addr + 1;
+
 		/*
 		 * Stop capture if the type is ASCII and valid field length
 		 * is 1 (0xc1) and next FRU data is less than 0x20 (space " ")
-		 * or it is 0x7f (delete 'DEL').
+		 * or it is 0x7f (delete 'DEL'). Only dereference next_addr
+		 * if it is still within the buffer.
 		 */
-		if (type == FRU_TYPELEN_TYPE_ASCII8 && len == 1	&&
+		if (type == FRU_TYPELEN_TYPE_ASCII8 && len == 1 &&
+		    next_addr < end &&
 		    (*next_addr < 0x20 || *next_addr == 0x7F))
 			break;
 
+		if (next_addr + len > end) {
+			debug("%s Board area field out of range\n", __func__);
+			break;
+		}
+
 		/* Stop when amount of chars is more then fields to record */
 		if (data + len > limit)
 			break;
@@ -235,7 +260,7 @@ static int fru_parse_board(unsigned long addr)
 	return 0;
 }
 
-static int fru_parse_multirec(unsigned long addr)
+static int fru_parse_multirec(unsigned long addr, u32 size)
 {
 	struct fru_multirec_hdr mrc;
 	u8 checksum = 0;
@@ -245,6 +270,11 @@ static int fru_parse_multirec(unsigned long addr)
 	debug("%s: multirec addr %lx\n", __func__, addr);
 
 	do {
+		if (size < hdr_len) {
+			printf("%s MultiRecord area out of range\n", __func__);
+			return -EINVAL;
+		}
+
 		memcpy(&mrc.rec_type, (void *)addr, hdr_len);
 
 		checksum = fru_checksum((u8 *)addr, hdr_len);
@@ -253,6 +283,12 @@ static int fru_parse_multirec(unsigned long addr)
 			return -EINVAL;
 		}
 
+		if (mrc.len > size - hdr_len) {
+			printf("%s MultiRecord length out of range\n",
+			       __func__);
+			return -EINVAL;
+		}
+
 		if (mrc.rec_type == FRU_MULTIREC_TYPE_OEM) {
 			struct fru_multirec_mac *mac = (void *)addr + hdr_len;
 			u32 type = FRU_DUT_MACID;
@@ -271,16 +307,24 @@ static int fru_parse_multirec(unsigned long addr)
 			}
 		}
 		addr += mrc.len + hdr_len;
+		size -= mrc.len + hdr_len;
 	} while (!(mrc.type & FRU_LAST_REC));
 
 	return 0;
 }
 
-int fru_capture(unsigned long addr)
+int fru_capture(unsigned long addr, u32 size)
 {
 	struct fru_common_hdr *hdr;
 	u8 checksum = 0;
 	unsigned long multirec_addr = addr;
+	u16 offset;
+	int ret;
+
+	if (size < sizeof(struct fru_common_hdr)) {
+		printf("%s Common header out of range\n", __func__);
+		return -EINVAL;
+	}
 
 	checksum = fru_checksum((u8 *)addr, sizeof(struct fru_common_hdr));
 	if (checksum) {
@@ -293,18 +337,34 @@ int fru_capture(unsigned long addr)
 	memcpy((void *)&fru_data, (void *)hdr,
 	       sizeof(struct fru_common_hdr));
 
-	fru_data.captured = true;
-
 	if (hdr->off_board) {
-		addr += fru_cal_area_len(hdr->off_board);
-		fru_parse_board(addr);
+		offset = fru_cal_area_len(hdr->off_board);
+		if (offset >= size) {
+			printf("%s Board area offset out of range\n", __func__);
+			return -EINVAL;
+		}
+
+		addr += offset;
+		ret = fru_parse_board(addr, size - offset);
+		if (ret)
+			return ret;
 	}
 
+	fru_data.captured = true;
 	env_set_hex("fru_addr", addr);
 
 	if (hdr->off_multirec) {
-		multirec_addr += fru_cal_area_len(hdr->off_multirec);
-		fru_parse_multirec(multirec_addr);
+		offset = fru_cal_area_len(hdr->off_multirec);
+		if (offset >= size) {
+			debug("%s MultiRecord area offset out of range\n",
+			      __func__);
+			return 0;
+		}
+
+		multirec_addr += offset;
+		ret = fru_parse_multirec(multirec_addr, size - offset);
+		if (ret)
+			debug("%s MultiRecord parse failed\n", __func__);
 	}
 
 	return 0;
-- 
2.43.0

[PATCH 2/3] xilinx: fru: Fix wrong argv index in do_fru_capture() addr check

[Michal Simek]

From: Pranav Tilak <pranav.vinaytilak@amd.com>

do_fru_capture() parses the address from argv[2] but validates
emptiness against argv[1], which is always the literal "capture"
subcommand token and can never be empty. An empty address string
passed as argv[2] is never rejected.

Check argv[2] instead, matching what was actually parsed.

Fixes: f1b97b5f18a8 ("xilinx: cmd: Add support for FRU commands")
Signed-off-by: Pranav Tilak <pranav.vinaytilak@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 board/xilinx/common/fru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/board/xilinx/common/fru.c b/board/xilinx/common/fru.c
index de165c548592..7566d961af1e 100644
--- a/board/xilinx/common/fru.c
+++ b/board/xilinx/common/fru.c
@@ -20,7 +20,7 @@ static int do_fru_capture(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_USAGE;
 
 	addr = hextoul(argv[2], &endp);
-	if (*argv[1] == 0 || *endp != 0)
+	if (*argv[2] == 0 || *endp != 0)
 		return -1;
 
 	size = hextoul(argv[3], &endp);
-- 
2.43.0

[PATCH 3/3] xilinx: fru: Use sizeof() for board header size

[Michal Simek]

From: Pranav Tilak <pranav.vinaytilak@amd.com>

fru_parse_board() copies the board area header and then skips past
it using the bare literal 6, the header's size.

Use sizeof(struct fru_board_info_header) at both sites instead, so
the two copies of this constant cannot drift apart if the struct
ever changes.

Signed-off-by: Pranav Tilak <pranav.vinaytilak@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 board/xilinx/common/fru_ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/board/xilinx/common/fru_ops.c b/board/xilinx/common/fru_ops.c
index fb88c21cdf6f..0fc95b927f8e 100644
--- a/board/xilinx/common/fru_ops.c
+++ b/board/xilinx/common/fru_ops.c
@@ -178,7 +178,8 @@ static int fru_parse_board(unsigned long addr, u32 size)
 		return -EINVAL;
 	}
 
-	memcpy(&fru_data.brd.ver, (void *)addr, 6);
+	memcpy(&fru_data.brd.ver, (void *)addr,
+	       sizeof(struct fru_board_info_header));
 
 	area_len = fru_data.brd.len * FRU_COMMON_HDR_LEN_MULTIPLIER;
 	if (area_len > size) {
@@ -198,7 +199,7 @@ static int fru_parse_board(unsigned long addr, u32 size)
 	eof = (u8 *)(area_len + addr - 2);
 	end = (u8 *)addr + size;
 
-	addr += 6;
+	addr += sizeof(struct fru_board_info_header);
 	data = (u8 *)&fru_data.brd.manufacturer_type_len;
 
 	/* Record max structure limit not to write data over allocated space */
-- 
2.43.0