[PATCH v2 00/12] block, disk: Check block sizes and partition tables of devices

[PATCH v2 00/12] block, disk: Check block sizes and partition tables of devices

[Ana Bobillo]

U-Boot scans the partitions of a block device as soon as it probes it,
and takes several sizes from the device and from its partition tables
as they are. A USB stick or SD card that U-Boot merely probes can
therefore make it read or write outside its buffers. This series checks
those sizes where they are read.

Block size. A block driver sets the block size of a device from what
the device reports, and only the virtio block driver and the host
command check it. The partition and filesystem code sizes buffers by the
block size, expects a block to hold a 512-byte boot sector and converts
between bytes and blocks with log2(blksz). So a USB stick, SCSI disk,
ATAPI drive or NVMe namespace reporting a block size below 512 bytes,
one that is not a power of two or a huge one makes U-Boot read or write
outside its buffers, in the partition scan run when the device is probed
and in the filesystem code after it.

Patch 1 drops two local copies of is_power_of_2(), so that blk.h can
use the one in linux/log2.h, and patch 2 adds blk_valid_blksz() there.
Patch 3 fixes the reply the USB storage driver makes up when READ
CAPACITY fails, which gave a 128KiB block size on a little-endian host.
Patches 4-7 check the block size where the USB storage, SCSI, IDE
(ATAPI) and NVMe drivers read it, so that each refuses such a device
cleanly: the USB LUN is skipped instead of failing the whole stick, no
SCSI block device is created, the ATAPI drive is treated as one without
a medium and the NVMe namespace is skipped like an inactive one. Patch 8
then has blk_post_probe() refuse a block device whose block size
blk_valid_blksz() rejects, which covers every block driver when its
device is probed, outside SPL and TPL (their size limits leave no room
for it). The MMC code sets the block size of a card again when it
initialises the card anew, as "mmc rescan" does; a follow-up will check
it there.

The upper bound is 64KiB, the largest logical block size Linux accepts;
the GPT code is already tested with block sizes up to 32KiB.

Partition tables. Patch 9 checks the header size and the size of the
partition entry array of a GPT, as Linux does, before they size the CRC
of the header and the entry buffer. It leaves alloc_read_gpt_entries()
alone, so it applies with or without Benoît Monin's "disk: efi: Reuse
the GPT entries buffer with simple malloc". Patch 10 walks the chain of
DOS extended partition tables in a loop: a table linking back to itself
adds no partition, so the limit added for CVE-2019-13103 did not stop
the recursion. As Linux does, the walk now gives up after a few tables
in a row that hold no partition. Patch 11 makes room for partition
number 256 in the UUID made up for a DOS partition. Patch 12 has the
TCG2 measurement of the GPT measure the GPT the partition driver uses,
read with the driver's own code. It read the primary GPT itself,
unchecked, while the driver falls back to the backup GPT when the
primary one is not valid, so PCR 5 could hold a GPT the partitions were
not found in, and the sizes in an unchecked header could make the
measurement read past its buffer.

Patches 4, 5 and 8-12 add tests: DM tests on sandbox, for the GPT a
test in test/lib on a blkmap device, and for the TCG2 measurement a test
in test/py that starts an EFI application from a GPT disk on sandbox and
checks PCR 5. With the series they pass, as do the rest of "ut dm" and
"ut lib" on sandbox_defconfig with CONFIG_ASAN (but for lib_crypt, which
ASan flags without the series too), and each patch builds and passes the
tests present so far on its own. The test/py test is
skipped with CONFIG_ASAN, under which sandbox cannot run an EFI
application; on sandbox_defconfig it passes, as do the other EFI, TPM
and GPT tests in test/py. The IDE and NVMe changes are build tested
only, as sandbox has no emulator for them.

CI: stacked with my other three series under review, the series passes
all 91 jobs of U-Boot's Azure pipeline, in build 13698 of my CI-only
pull request. Its first run, build 13695, showed the SPL cost of the
blk_post_probe() check on the boards named in the changes below.
PR:    https://github.com/u-boot/u-boot/pull/1064
Build: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=13698

Changes in v2:
- Reorder the block size patches, so that each step is bisectable: the
  helper first, then the driver patches, and the blk_post_probe() check
  last (Simon)
- Use is_power_of_2() in blk_valid_blksz() (Simon), after a new patch 1
  that drops the local copies in the btrfs code and bcm2835_sdhost, which
  clash with it
- Split the USB READ CAPACITY fallback fix into a patch of its own, with
  a Fixes: tag, and fake 2880 blocks rather than 2881 (Simon)
- Name the LUN that the USB storage driver skips, and check the message
  in the test (Simon)
- Take the ATAPI block sizes 0, 2340 and 2352 as 2048, as Linux does,
  and say what is skipped otherwise (Simon)
- Leave the blk_post_probe() check out of SPL and TPL, where it took the
  SPL of imx28_btt3 and popmetal-rk3288 over their size limits and broke
  the link of evb-rk3036's, and say why it is outside the PARTITIONS
  condition (Simon)
- Include linux/sizes.h in part_efi.c, and word the GPT commit message
  so that the fastboot path is not taken as fully covered (Simon)
- Write the DOS walk as a for loop with a helper that follows the links,
  and stop it after 16 tables in a row without a partition (Simon)
- Measure the GPT the partition driver uses, which checks the CRC of
  the entries too (Simon), and when the primary GPT is not valid the
  backup one, with the primary header made from it, as EDK2 does
  (Ilias); say what the bug does to PCR 5 (Ilias)
- Avoid ut_assertok() of a known -ENOMEM in the GPT test (Coverity)

Ana Bobillo (12):
  treewide: Use is_power_of_2() from linux/log2.h
  block: Add blk_valid_blksz() to check a block size
  usb: storage: Fix the capacity faked when READ CAPACITY fails
  usb: storage: Refuse a block size the block layer cannot use
  scsi: Refuse a block size the block layer cannot use
  ide: Refuse an ATAPI block size atapi_read() cannot read
  nvme: Skip a namespace with an unsupported LBA data size
  block: Refuse a device with an unsupported block size
  disk: part_efi: Check the header and entry sizes of a GPT
  disk: part_dos: Walk the extended partition tables in a bounded loop
  disk: part_dos: Make room for a three-digit partition number in the
    UUID
  efi_loader: Measure the GPT the partition driver uses

 arch/sandbox/include/asm/test.h  |  20 +++
 cmd/host.c                       |   5 +-
 common/usb_storage.c             |  10 +-
 disk/part_dos.c                  | 291 ++++++++++++++++++-------------
 disk/part_efi.c                  |  68 +++++++-
 drivers/block/blk-uclass.c       |  15 ++
 drivers/block/ide.c              |  23 ++-
 drivers/mmc/bcm2835_sdhost.c     |   6 +-
 drivers/nvme/nvme.c              |  33 +++-
 drivers/scsi/sandbox_scsi.c      |   8 +
 drivers/scsi/scsi.c              |   4 +
 drivers/usb/emul/sandbox_flash.c |   8 +
 drivers/virtio/virtio_blk.c      |   2 +-
 fs/btrfs/compat.h                |   6 +-
 include/blk.h                    |  21 +++
 include/part.h                   |  26 +++
 lib/efi_loader/efi_tcg2.c        |  51 +++---
 test/dm/blk.c                    |  41 +++++
 test/dm/part.c                   | 221 +++++++++++++++++++++++
 test/dm/scsi.c                   |  37 ++++
 test/dm/usb.c                    |  51 ++++++
 test/lib/part_efi.c              | 203 +++++++++++++++++++++
 test/py/tests/test_efi_tcg2.py   | 222 +++++++++++++++++++++++
 23 files changed, 1198 insertions(+), 174 deletions(-)
 create mode 100644 test/py/tests/test_efi_tcg2.py


base-commit: dfcfc4c9f78d1743f490a497082a4ec8e058aae8

[PATCH v2 01/12] treewide: Use is_power_of_2() from linux/log2.h

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

The btrfs code and the bcm2835_sdhost driver each define their own
is_power_of_2(), which clashes with the one in linux/log2.h once a
header they include brings that in, as include/blk.h does after the next
patch. Drop both and use the common one.

The btrfs copy is the same as the common one. The bcm2835_sdhost copy
takes a u64 and also takes 0 as a power of two; its only caller checks
the block size of a data transfer, which the MMC core never sets to 0,
and the Linux driver it comes from makes the same check with the common
is_power_of_2().

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- New patch, so that blk.h can use is_power_of_2() from linux/log2.h

 drivers/mmc/bcm2835_sdhost.c | 6 +-----
 fs/btrfs/compat.h            | 6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/bcm2835_sdhost.c b/drivers/mmc/bcm2835_sdhost.c
index 720127468d3..eee7e2d43a2 100644
--- a/drivers/mmc/bcm2835_sdhost.c
+++ b/drivers/mmc/bcm2835_sdhost.c
@@ -42,6 +42,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/log2.h>
 #include <linux/sizes.h>
 #include <mach/gpio.h>
 #include <power/regulator.h>
@@ -660,11 +661,6 @@ static void bcm2835_set_clock(struct bcm2835_host *host, unsigned int clock)
 	writel(host->mmc->clock / 2, host->ioaddr + SDTOUT);
 }
 
-static inline int is_power_of_2(u64 x)
-{
-	return !(x & (x - 1));
-}
-
 static int bcm2835_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
 			    struct mmc_data *data)
 {
diff --git a/fs/btrfs/compat.h b/fs/btrfs/compat.h
index 4596b9d1dd3..ffeba740eff 100644
--- a/fs/btrfs/compat.h
+++ b/fs/btrfs/compat.h
@@ -4,6 +4,7 @@
 #define __BTRFS_COMPAT_H__
 
 #include <linux/errno.h>
+#include <linux/log2.h>
 #include <fs_internal.h>
 #include <u-boot/uuid.h>
 
@@ -74,9 +75,4 @@ static inline void uuid_unparse(const u8 *uuid, char *out)
 	return uuid_bin_to_str((unsigned char *)uuid, out, 0);
 }
 
-static inline int is_power_of_2(unsigned long n)
-{
-	return (n != 0 && ((n & (n - 1)) == 0));
-}
-
 #endif

[PATCH v2 02/12] block: Add blk_valid_blksz() to check a block size

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

A block driver sets blksz and log2blksz from what the device reports: the
READ CAPACITY reply of a USB stick or a SCSI disk, the LBA format of an
NVMe namespace, the sector size of a Xen backend. Apart from the virtio
block driver and the host command, none of these drivers checks the
value, yet the partition and filesystem code relies on it:

- part_get_info_extended() and print_partition_extended() read one block
  into a buffer of blksz bytes and parse the partition table at offset
  0x1be, past the end of a block smaller than 512 bytes
- fs_devread() converts bytes to blocks with shifts by log2blksz, masks
  by blksz - 1 and divisions by blksz, which only agree when blksz is a
  power of two; with any other value it reads more bytes into the
  caller's buffer than the caller asked for
- buffers of blksz bytes go on the stack, so a block size in the
  gigabytes overruns it

Add blk_valid_blksz(), which accepts a power of two from 512 bytes up to
BLK_MAX_BLKSZ (64KiB, the largest logical block size Linux accepts), so
that the block drivers can check what their devices report. Use it in
the virtio block driver and the host command, which then also refuse a
block size above 64KiB.

The next patches check the block size in the USB storage, SCSI, IDE and
NVMe drivers, and the last one in blk_post_probe() for every block
device.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Split out of v1 patch 1: the helper comes first, and blk_post_probe()
  checks the block size only at the end of the series (Simon)
- Use is_power_of_2() and leave the linux/log2.h includes alone (Simon)

 cmd/host.c                  |  5 +++--
 drivers/virtio/virtio_blk.c |  2 +-
 include/blk.h               | 21 +++++++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/cmd/host.c b/cmd/host.c
index 51f2b1e3f6e..7a94ea1fa27 100644
--- a/cmd/host.c
+++ b/cmd/host.c
@@ -67,8 +67,9 @@ static int do_host_bind(struct cmd_tbl *cmdtp, int flag, int argc,
 	file = argv[1];
 	if (argc > 2) {
 		blksz = dectoul(argv[2], NULL);
-		if (blksz < DEFAULT_BLKSZ || !is_power_of_2(blksz)) {
-			printf("blksz must be >= 512 and power of 2\n");
+		if (!blk_valid_blksz(blksz)) {
+			printf("blksz must be a power of 2 from %d to %d\n",
+			       DEFAULT_BLKSZ, BLK_MAX_BLKSZ);
 			return CMD_RET_FAILURE;
 		}
 	}
diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
index 404d9140cb2..ddc42a8543b 100644
--- a/drivers/virtio/virtio_blk.c
+++ b/drivers/virtio/virtio_blk.c
@@ -263,7 +263,7 @@ static int virtio_blk_probe(struct udevice *dev)
 	if (virtio_has_feature(dev, VIRTIO_BLK_F_BLK_SIZE)) {
 		virtio_cread(dev, struct virtio_blk_config, blk_size, &blk_size);
 		desc->blksz = blk_size;
-		if (!is_power_of_2(blk_size) || desc->blksz < 512)
+		if (!blk_valid_blksz(blk_size))
 			return -EIO;
 	} else {
 		desc->blksz = 512;
diff --git a/include/blk.h b/include/blk.h
index 5ef9129b3e6..07e0448f65e 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -10,6 +10,8 @@
 #include <bouncebuf.h>
 #include <dm/uclass-id.h>
 #include <efi.h>
+#include <linux/log2.h>
+#include <linux/sizes.h>
 
 #ifdef CONFIG_SYS_64BIT_LBA
 typedef uint64_t lbaint_t;
@@ -22,9 +24,28 @@ typedef ulong lbaint_t;
 #define LBAFU "%" LBAFlength "u"
 
 #define DEFAULT_BLKSZ		512
+/* Largest block size supported, as in Linux */
+#define BLK_MAX_BLKSZ		SZ_64K
 
 struct udevice;
 
+/**
+ * blk_valid_blksz() - Check that a block size is supported
+ *
+ * The block, partition and filesystem code size buffers by the block size,
+ * expect a block to hold at least a 512-byte boot sector and convert between
+ * bytes and blocks with log2(block size), so a device is only usable with a
+ * block size that is a power of two from DEFAULT_BLKSZ to BLK_MAX_BLKSZ.
+ *
+ * @blksz: Block size in bytes
+ * Return: true if @blksz is supported, false if not
+ */
+static inline bool blk_valid_blksz(unsigned long blksz)
+{
+	return blksz >= DEFAULT_BLKSZ && blksz <= BLK_MAX_BLKSZ &&
+	       is_power_of_2(blksz);
+}
+
 static inline bool blk_enabled(void)
 {
 	return CONFIG_IS_ENABLED(BLK) || IS_ENABLED(CONFIG_SPL_LEGACY_BLOCK);

[PATCH v2 03/12] usb: storage: Fix the capacity faked when READ CAPACITY fails

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

When READ CAPACITY fails, usb_stor_get_info() makes up a reply of its own
for a 1.44MB floppy, 2880 blocks of 512 bytes. But it sets the values in
host byte order, while the reply is read back as big endian, so on a
little-endian host the device gets 0x400b0001 blocks (about 1G) of 128KiB
rather than 2880 of 512 bytes. It also puts 2880 where READ CAPACITY
returns the last LBA, so even on a big-endian host the device gets 2881
blocks.

Set the reply in big endian like the one it stands in for, with 2879 as
the last LBA, so that such a device has 2880 blocks of 512 bytes on any
host.

Fixes: 149dded2b178 ("* Add support for USB Mass Storage Devices (BBB)   (tested with USB memory sticks only)")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- New patch, split out of v1 patch 2, with a Fixes: tag (Simon)
- Give 2879 as the last LBA, so that the device has 2880 blocks (Simon)

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

diff --git a/common/usb_storage.c b/common/usb_storage.c
index ac331f1c1b0..f7c96195b59 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1496,8 +1496,9 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	if (usb_read_capacity(pccb, ss) != 0) {
 		printf("READ_CAP ERROR\n");
 		ss->flags &= ~USB_READY;
-		cap[0] = 2880;
-		cap[1] = 0x200;
+		/* a 1.44MB floppy, as READ CAPACITY gives it: last LBA, size */
+		cap[0] = cpu_to_be32(2880 - 1);
+		cap[1] = cpu_to_be32(0x200);
 	}
 	debug("Read Capacity returns: 0x%08x, 0x%08x\n", cap[0], cap[1]);
 #if 0

[PATCH v2 04/12] usb: storage: Refuse a block size the block layer cannot use

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

usb_stor_get_info() takes the block size from the READ CAPACITY reply of
the device as it is. Once blk_post_probe() refuses a block size that
blk_valid_blksz() rejects, at the end of this series, such a device would
fail to probe there, and usb_stor_probe_device() would stop, skipping the
other LUNs of the device.

Check the block size where it is read, and treat a LUN reporting one
that blk_valid_blksz() rejects as unusable, as for a LUN that is not
ready, saying which LUN is skipped. This also covers the legacy (non-BLK)
path, which calls part_init() itself.

Add a test where the sandbox flash stick reports an unsupported block
size.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Move the READ CAPACITY fallback fix to a patch of its own (Simon)
- Name the LUN in the message (Simon)
- Check for that message in the test, with UTF_CONSOLE (Simon)

 arch/sandbox/include/asm/test.h  | 10 +++++++
 common/usb_storage.c             |  5 ++++
 drivers/usb/emul/sandbox_flash.c |  8 +++++
 test/dm/usb.c                    | 51 ++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+)

diff --git a/arch/sandbox/include/asm/test.h b/arch/sandbox/include/asm/test.h
index 0438790028b..e5590d01a92 100644
--- a/arch/sandbox/include/asm/test.h
+++ b/arch/sandbox/include/asm/test.h
@@ -123,6 +123,16 @@ long sandbox_i2c_rtc_get_set_base_time(struct udevice *dev, long base_time);
 
 int sandbox_usb_keyb_add_string(struct udevice *dev, const char *str);
 
+/**
+ * sandbox_flash_set_block_size() - Set the block size a flash stick reports
+ *
+ * @dev:	USB flash-stick emulator
+ * @block_size:	Block size in bytes
+ *
+ * The host reads it when it next probes the stick's mass-storage device.
+ */
+void sandbox_flash_set_block_size(struct udevice *dev, int block_size);
+
 /**
  * sandbox_osd_get_mem() - get the internal memory of a sandbox OSD
  *
diff --git a/common/usb_storage.c b/common/usb_storage.c
index f7c96195b59..6bada2b309b 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1513,6 +1513,11 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	blksz = be32_to_cpu(cap[1]);
 
 	debug("Capacity = 0x%08x, blocksz = 0x%08x\n", capacity, blksz);
+	if (!blk_valid_blksz(blksz)) {
+		printf("LUN %d: unsupported block size %u, skipping\n",
+		       dev_desc->lun, blksz);
+		return 0;
+	}
 	dev_desc->lba = capacity;
 	dev_desc->blksz = blksz;
 	dev_desc->log2blksz = LOG2(dev_desc->blksz);
diff --git a/drivers/usb/emul/sandbox_flash.c b/drivers/usb/emul/sandbox_flash.c
index b5176bb30ce..02e0901050f 100644
--- a/drivers/usb/emul/sandbox_flash.c
+++ b/drivers/usb/emul/sandbox_flash.c
@@ -13,6 +13,7 @@
 #include <scsi.h>
 #include <scsi_emul.h>
 #include <usb.h>
+#include <asm/test.h>
 
 /*
  * This driver emulates a flash stick using the UFI command specification and
@@ -357,6 +358,13 @@ static int sandbox_flash_probe(struct udevice *dev)
 	return 0;
 }
 
+void sandbox_flash_set_block_size(struct udevice *dev, int block_size)
+{
+	struct sandbox_flash_priv *priv = dev_get_priv(dev);
+
+	priv->eminfo.block_size = block_size;
+}
+
 static int sandbox_flash_remove(struct udevice *dev)
 {
 	struct sandbox_flash_priv *priv = dev_get_priv(dev);
diff --git a/test/dm/usb.c b/test/dm/usb.c
index fa894c1096e..79a85b11910 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -84,6 +84,57 @@ static int dm_test_usb_flash(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_usb_flash, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 
+/* Test that a flash stick reporting an unsupported block size is refused */
+static int dm_test_usb_flash_blksz(struct unit_test_state *uts)
+{
+	static const struct {
+		int blksz;
+		bool ok;
+	} cases[] = {
+		{ 256, false },
+		{ 520, false },
+		{ 2 * BLK_MAX_BLKSZ, false },
+		{ SZ_4K, true },
+	};
+	struct udevice *dev, *emul, *blk;
+	struct blk_desc *desc;
+	int i, ret;
+
+	state_set_skip_delays(true);
+	ut_assertok(usb_init());
+	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev));
+	ut_assertok(uclass_get_device_by_name(UCLASS_USB_EMUL, "flash-stick@0",
+					      &emul));
+
+	for (i = 0; i < ARRAY_SIZE(cases); i++) {
+		/* Probe the stick again, so that it reads the block size */
+		console_record_reset();
+		sandbox_flash_set_block_size(emul, cases[i].blksz);
+		ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
+		ut_assertok(device_chld_unbind(dev, NULL));
+		ut_assertok(device_probe(dev));
+
+		ret = device_find_first_child_by_uclass(dev, UCLASS_BLK, &blk);
+		if (!cases[i].ok) {
+			/* usb_stor_get_info() refuses the LUN */
+			ut_assert_nextline("LUN 0: unsupported block size %d, skipping",
+					   cases[i].blksz);
+			ut_assert_console_end();
+			ut_asserteq(-ENODEV, ret);
+			continue;
+		}
+		ut_assert_console_end();
+		ut_assertok(ret);
+		desc = dev_get_uclass_plat(blk);
+		ut_asserteq(cases[i].blksz, desc->blksz);
+	}
+
+	ut_assertok(usb_stop());
+
+	return 0;
+}
+DM_TEST(dm_test_usb_flash_blksz, UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
+
 /* test that we can handle multiple storage devices */
 static int dm_test_usb_multi(struct unit_test_state *uts)
 {

[PATCH v2 05/12] scsi: Refuse a block size the block layer cannot use

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

scsi_detect_dev() takes the block size from the READ CAPACITY reply of
the device as it is. do_scsi_scan_one() then hands it to
blk_create_devicef() as an int, which drops the upper bits of the
unsigned long, so the block layer may not even see the value the device
reported.

Check the block size where it is read, so that no block device is
created for a disk reporting one that blk_valid_blksz() rejects.

Add a test where the sandbox SCSI disk reports an unsupported block
size.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)

 arch/sandbox/include/asm/test.h | 10 +++++++++
 drivers/scsi/sandbox_scsi.c     |  8 +++++++
 drivers/scsi/scsi.c             |  4 ++++
 test/dm/scsi.c                  | 37 +++++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+)

diff --git a/arch/sandbox/include/asm/test.h b/arch/sandbox/include/asm/test.h
index e5590d01a92..76e2f11b059 100644
--- a/arch/sandbox/include/asm/test.h
+++ b/arch/sandbox/include/asm/test.h
@@ -133,6 +133,16 @@ int sandbox_usb_keyb_add_string(struct udevice *dev, const char *str);
  */
 void sandbox_flash_set_block_size(struct udevice *dev, int block_size);
 
+/**
+ * sandbox_scsi_set_block_size() - Set the block size a SCSI disk reports
+ *
+ * @dev:	Sandbox SCSI controller
+ * @block_size:	Block size in bytes
+ *
+ * The host reads it when it next scans the bus.
+ */
+void sandbox_scsi_set_block_size(struct udevice *dev, int block_size);
+
 /**
  * sandbox_osd_get_mem() - get the internal memory of a sandbox OSD
  *
diff --git a/drivers/scsi/sandbox_scsi.c b/drivers/scsi/sandbox_scsi.c
index 5f0b01d86d5..34fc7cda915 100644
--- a/drivers/scsi/sandbox_scsi.c
+++ b/drivers/scsi/sandbox_scsi.c
@@ -14,6 +14,7 @@
 #include <malloc.h>
 #include <scsi.h>
 #include <scsi_emul.h>
+#include <asm/test.h>
 
 enum {
 	SANDBOX_SCSI_BLOCK_LEN		= 512,
@@ -118,6 +119,13 @@ static int sandbox_scsi_probe(struct udevice *dev)
 	return 0;
 }
 
+void sandbox_scsi_set_block_size(struct udevice *dev, int block_size)
+{
+	struct sandbox_scsi_priv *priv = dev_get_priv(dev);
+
+	priv->eminfo.block_size = block_size;
+}
+
 static int sandbox_scsi_remove(struct udevice *dev)
 {
 	struct sandbox_scsi_priv *priv = dev_get_priv(dev);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 50e7d749921..a732db726ac 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -543,6 +543,10 @@ static int scsi_detect_dev(struct udevice *dev, int target, int lun,
 		scsi_print_error(pccb);
 		return -EINVAL;
 	}
+	if (!blk_valid_blksz(blksz)) {
+		printf("Unsupported block size %lu\n", blksz);
+		return -EINVAL;
+	}
 	dev_desc->lba = capacity;
 	dev_desc->blksz = blksz;
 	dev_desc->log2blksz = LOG2(dev_desc->blksz);
diff --git a/test/dm/scsi.c b/test/dm/scsi.c
index 66859c991df..7617ee54893 100644
--- a/test/dm/scsi.c
+++ b/test/dm/scsi.c
@@ -6,6 +6,7 @@
 #include <dm.h>
 #include <part.h>
 #include <scsi.h>
+#include <asm/test.h>
 #include <dm/test.h>
 #include <test/test.h>
 #include <test/ut.h>
@@ -36,3 +37,39 @@ static int dm_test_scsi_base(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_scsi_base, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/* Test that a disk reporting an unsupported block size is refused */
+static int dm_test_scsi_blksz(struct unit_test_state *uts)
+{
+	static const struct {
+		int blksz;
+		bool ok;
+	} cases[] = {
+		{ 256, false },
+		{ 520, false },
+		{ 2 * BLK_MAX_BLKSZ, false },
+		{ SZ_4K, true },
+	};
+	struct udevice *dev, *blk;
+	struct blk_desc *desc;
+	int i, ret;
+
+	ut_assertok(uclass_first_device_err(UCLASS_SCSI, &dev));
+
+	for (i = 0; i < ARRAY_SIZE(cases); i++) {
+		sandbox_scsi_set_block_size(dev, cases[i].blksz);
+		ut_assertok(scsi_scan(false));
+
+		ret = device_find_first_child_by_uclass(dev, UCLASS_BLK, &blk);
+		if (!cases[i].ok) {
+			ut_asserteq(-ENODEV, ret);
+			continue;
+		}
+		ut_assertok(ret);
+		desc = dev_get_uclass_plat(blk);
+		ut_asserteq(cases[i].blksz, desc->blksz);
+	}
+
+	return 0;
+}
+DM_TEST(dm_test_scsi_blksz, UTF_SCAN_PDATA | UTF_SCAN_FDT);

[PATCH v2 06/12] ide: Refuse an ATAPI block size atapi_read() cannot read

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

atapi_inquiry() takes the block size from the READ CAPACITY reply of an
ATAPI device as it is, while atapi_read() asks the device for, and moves
the buffer on by, ATAPI_READ_BLOCK_SIZE (2048) bytes for every block.
With any other block size the callers' buffers, which are sized by
blksz, do not match what atapi_read() writes into them: a 512-byte block
size makes a read of several blocks write past the end of the buffer.

Some CD drives report another block size although they read 2048-byte
blocks: 2340 (HP 4020i CD-Recorder), 2352 (Philips CD-Writers) or 0.
Take these as ATAPI_READ_BLOCK_SIZE, as Linux does in get_sectorsize(),
so that a drive reporting 0, which ide_probe() skipped until now, is
read with 2048-byte blocks too. Refuse any other value: a device
reporting one is left with a zero block size, which ide_probe() skips,
as for a drive without a medium. This refuses drives with 512-byte
blocks, such as ZIP drives, whose reads of more than one block already
overran the buffer; supporting them needs atapi_read() to use the block
size of the device.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Take 0, 2340 and 2352 as 2048, as Linux get_sectorsize() does (Simon)
- Say what is skipped in the message (Simon)

 drivers/block/ide.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/block/ide.c b/drivers/block/ide.c
index 44ce054bdf5..56e55bc8bc1 100644
--- a/drivers/block/ide.c
+++ b/drivers/block/ide.c
@@ -450,6 +450,7 @@ static void atapi_inquiry(struct blk_desc *desc)
 {
 	u8 ccb[12];	/* Command descriptor block */
 	u8 iobuf[64];	/* temp buf */
+	ulong blksz;
 	u8 c;
 	int device;
 
@@ -514,10 +515,28 @@ static void atapi_inquiry(struct blk_desc *desc)
 		  iobuf[0], iobuf[1], iobuf[2], iobuf[3],
 		  iobuf[4], iobuf[5], iobuf[6], iobuf[7]);
 
+	blksz = (ulong)iobuf[4] << 24 | (ulong)iobuf[5] << 16 |
+		(ulong)iobuf[6] << 8 | (ulong)iobuf[7];
+	/*
+	 * atapi_read() transfers ATAPI_READ_BLOCK_SIZE bytes per block. Some
+	 * CD drives report another size although they read 2048-byte blocks,
+	 * which Linux takes as 2048 in get_sectorsize(): do the same.
+	 */
+	switch (blksz) {
+	case 0:
+	case 2340:	/* HP 4020i CD-Recorder */
+	case 2352:	/* Philips CD-Writers */
+		blksz = ATAPI_READ_BLOCK_SIZE;
+		break;
+	case ATAPI_READ_BLOCK_SIZE:
+		break;
+	default:
+		printf("ATAPI block size %lu not supported, skipping\n", blksz);
+		return;
+	}
 	desc->lba = (ulong)iobuf[0] << 24 | (ulong)iobuf[1] << 16 |
 		(ulong)iobuf[2] << 8 | (ulong)iobuf[3];
-	desc->blksz = (ulong)iobuf[4] << 24 | (ulong)iobuf[5] << 16 |
-		(ulong)iobuf[6] << 8 | (ulong)iobuf[7];
+	desc->blksz = blksz;
 	desc->log2blksz = LOG2(desc->blksz);
 
 	/* ATAPI devices cannot use 48bit addressing (ATA/ATAPI v7) */

[PATCH v2 07/12] nvme: Skip a namespace with an unsupported LBA data size

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

nvme_blk_probe() takes the LBA data size (log2 of the block size) of the
namespace's current LBA format as the device reports it, and shifts by
it: 1 << ns->lba_shift is undefined for a value of 31 or more, and
nvme_blk_rw() sizes each transfer with
1 << (dev->max_transfer_shift - ns->lba_shift), a negative shift for a
block larger than the maximum transfer size of the controller.

Check it, through a new nvme_lba_shift(), against the block sizes
blk_valid_blksz() accepts and the maximum transfer size. nvme_init()
skips a namespace with an unsupported LBA data size, as it skips an
inactive one, rather than create a block device for it that then fails
to probe, failing nvme_init() and leaving the namespaces it has already
probed on a controller that failed to probe. nvme_blk_probe() checks the
value it uses again.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)

 drivers/nvme/nvme.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index c815a967137..3df9b531bfd 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -15,6 +15,7 @@
 #include <time.h>
 #include <dm/device-internal.h>
 #include <linux/compat.h>
+#include <linux/log2.h>
 #include "nvme.h"
 
 #define NVME_Q_DEPTH		2
@@ -701,6 +702,26 @@ int nvme_scan_namespace(void)
 	return 0;
 }
 
+/**
+ * nvme_lba_shift() - Get the LBA data size of a namespace
+ *
+ * @dev: NVMe controller
+ * @id: Identify Namespace data of the namespace
+ * Return: log2 of its block size, or -EINVAL if that size is not supported
+ */
+static int nvme_lba_shift(struct nvme_dev *dev, struct nvme_id_ns *id)
+{
+	u8 ds = id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ds;
+
+	if (ds < ilog2(DEFAULT_BLKSZ) || ds > ilog2(BLK_MAX_BLKSZ) ||
+	    ds > dev->max_transfer_shift) {
+		log_err("Unsupported LBA data size 2^%d\n", ds);
+		return -EINVAL;
+	}
+
+	return ds;
+}
+
 static int nvme_blk_probe(struct udevice *udev)
 {
 	struct nvme_dev *ndev = dev_get_priv(udev->parent);
@@ -708,6 +729,7 @@ static int nvme_blk_probe(struct udevice *udev)
 	struct nvme_ns *ns = dev_get_priv(udev);
 	u8 flbas;
 	struct nvme_id_ns *id;
+	int ret;
 
 	id = memalign(ndev->page_size, sizeof(struct nvme_id_ns));
 	if (!id)
@@ -724,7 +746,12 @@ static int nvme_blk_probe(struct udevice *udev)
 	memcpy(&ns->eui64, &id->eui64, sizeof(id->eui64));
 	flbas = id->flbas & NVME_NS_FLBAS_LBA_MASK;
 	ns->flbas = flbas;
-	ns->lba_shift = id->lbaf[flbas].ds;
+	ret = nvme_lba_shift(ndev, id);
+	if (ret < 0) {
+		free(id);
+		return ret;
+	}
+	ns->lba_shift = ret;
 	list_add(&ns->list, &ndev->namespaces);
 
 	desc->lba = le64_to_cpu(id->nsze);
@@ -899,6 +926,10 @@ int nvme_init(struct udevice *udev)
 		if (!id->nsze)
 			continue;
 
+		/* skip a namespace whose block size is not supported */
+		if (nvme_lba_shift(ndev, id) < 0)
+			continue;
+
 		/*
 		 * Encode the namespace id to the device name so that
 		 * we can extract it when doing the probe.

[PATCH v2 08/12] block: Refuse a device with an unsupported block size

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

blk_post_probe() scans a block device for partitions with the block size
its driver set from what the device reports, and nothing checks it in
between. The USB storage, SCSI, IDE, NVMe and virtio block drivers and the
host command now check it where they read it, but other drivers do not,
such as the Xen PV block driver, and the partition and filesystem code
relies on it, as the patch adding blk_valid_blksz() describes.

Refuse to probe a block device whose block size blk_valid_blksz()
rejects, or whose log2blksz does not match its blksz, before its
partitions are scanned. This covers every block driver in one place
when its device is probed.

The check does not depend on PARTITIONS, since the filesystem code and
raw reads rely on the block size too. It is left out of SPL and TPL,
though: there it adds some 180 bytes, which takes the SPL of imx28_btt3
and popmetal-rk3288 over their size limits, and its message needs a
putc() that the SPL of evb-rk3036 does not have. In SPL the USB storage,
SCSI, IDE, NVMe and virtio block drivers still check the block size
where they read it.

A device with neither a size nor a block size, which is how the SCSI
code leaves a removable drive without a medium, is still probed, but
outside SPL and TPL its partitions are no longer scanned: part_test_dos()
could otherwise parse 512 bytes of a zero-sized buffer.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Split out of v1 patch 1 and moved after the driver patches, so that
  each step is bisectable (Simon)
- Leave the check out of SPL and TPL, and say why in the commit message,
  along with why it is outside the PARTITIONS condition (Simon). On the
  code size: in SPL the v1 check and its message added 184 bytes to
  imx28_btt3, taking it over its SPL size limit, and 176 bytes to
  popmetal-rk3288, over its limit too, and the SPL of evb-rk3036 no
  longer linked (its tiny printf has no putc()). With the check left
  out, the SPL of these boards is the same size as without the series

 drivers/block/blk-uclass.c | 15 ++++++++++++++
 test/dm/blk.c              | 41 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 37d87be3a57..0c45a57f75f 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -828,6 +828,21 @@ static int part_create_block_devices(struct udevice *blk_dev)
 
 static int blk_post_probe(struct udevice *dev)
 {
+	if (!IS_ENABLED(CONFIG_XPL_BUILD)) {
+		struct blk_desc *desc = dev_get_uclass_plat(dev);
+
+		/* A removable device without a medium has no size yet */
+		if (!desc->blksz && !desc->lba)
+			return 0;
+
+		if (!blk_valid_blksz(desc->blksz) ||
+		    desc->log2blksz != LOG2(desc->blksz)) {
+			log_err("%s: unsupported block size %lu (log2 %d)\n",
+				dev->name, desc->blksz, desc->log2blksz);
+			return -EINVAL;
+		}
+	}
+
 	if (CONFIG_IS_ENABLED(PARTITIONS) && blk_enabled()) {
 		struct blk_desc *desc = dev_get_uclass_plat(dev);
 
diff --git a/test/dm/blk.c b/test/dm/blk.c
index f67869107da..80441f723c7 100644
--- a/test/dm/blk.c
+++ b/test/dm/blk.c
@@ -9,6 +9,7 @@
 #include <sandbox_host.h>
 #include <usb.h>
 #include <asm/state.h>
+#include <dm/device-internal.h>
 #include <dm/test.h>
 #include <test/test.h>
 #include <test/ut.h>
@@ -113,6 +114,46 @@ static int dm_test_blk_find(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_blk_find, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 
+/* Test that a block device with an unsupported block size is refused */
+static int dm_test_blk_blksz(struct unit_test_state *uts)
+{
+	static const ulong good[] = { DEFAULT_BLKSZ, SZ_4K, BLK_MAX_BLKSZ };
+	static const ulong bad[] = { 256, 520, 3 * DEFAULT_BLKSZ,
+				     2 * BLK_MAX_BLKSZ };
+	struct udevice *blk, *dev;
+	struct blk_desc *desc;
+	char label[10];
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(good); i++) {
+		snprintf(label, sizeof(label), "good%d", i);
+		ut_assertok(host_create_device(label, false, good[i], &dev));
+		ut_assertok(blk_get_from_parent(dev, &blk));
+		desc = dev_get_uclass_plat(blk);
+		ut_asserteq(good[i], desc->blksz);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(bad); i++) {
+		snprintf(label, sizeof(label), "bad%d", i);
+		ut_assertok(host_create_device(label, false, bad[i], &dev));
+		ut_asserteq(-EINVAL, blk_get_from_parent(dev, &blk));
+	}
+
+	/* A device without a medium has no block size yet */
+	ut_assertok(host_create_device("none", false, 0, &dev));
+	ut_assertok(blk_get_from_parent(dev, &blk));
+
+	/* The block size and its log2 must agree */
+	ut_assertok(host_create_device("log2", false, DEFAULT_BLKSZ, &dev));
+	ut_assertok(blk_find_from_parent(dev, &blk));
+	desc = dev_get_uclass_plat(blk);
+	desc->log2blksz = 10;
+	ut_asserteq(-EINVAL, device_probe(blk));
+
+	return 0;
+}
+DM_TEST(dm_test_blk_blksz, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
 /* Test that block device numbering works as expected */
 static int dm_test_blk_devnum(struct unit_test_state *uts)
 {

[PATCH v2 09/12] disk: part_efi: Check the header and entry sizes of a GPT

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

validate_gpt_header() computes the CRC of a GPT header over header_size
bytes, as the header gives it, and nothing checks the size of the
partition entries before they are used:

- the header is read into a buffer of one block, on the stack behind
  part_get_info(), part_print_efi(), gpt_repair_headers() and the gpt
  command, so a header_size larger than the block size makes the CRC
  read past the end of that buffer
- alloc_read_gpt_entries() allocates and reads
  num_partition_entries * sizeof_partition_entry bytes, computed in 32
  bits, but the entries are then used as an array of
  num_partition_entries gpt_entry structures: with a smaller entry size,
  or a product that wraps, part_get_gpt_pte() and part_print_efi() read
  past the end of the allocation. On a 32-bit board, rounding a product
  close to 4GiB up to whole blocks also wraps, to a tiny allocation that
  the entries are then read into.

As Linux does in is_gpt_valid(), refuse a header whose header_size is
smaller than the header or larger than the block it is read from, one
whose sizeof_partition_entry is not sizeof(gpt_entry), and one whose
entry array is larger than 4MiB (KMALLOC_MAX_SIZE in Linux with 4KiB
pages, 32768 entries). Larger entries are allowed by the UEFI
specification, but U-Boot has never read them correctly, and Linux does
not accept them either.

The header size is checked before the CRC is computed over it. All the
checks are in validate_gpt_header(), so they also apply to the header
that is_valid_gpt_buf() checks when fastboot is sent a GPT. That path
still takes partition_entry_lba from the header without checking it
against the size of the download, which is left for a follow-up.

Add a test that gives the primary header of a GPT on a blkmap device
each of these sizes, with CRCs to match, and checks that it is refused
and that the backup header is used instead.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Include linux/sizes.h, rather than rely on blk.h for SZ_4M (Simon)
- Don't word the commit message as if the fastboot path were fully
  covered: its partition_entry_lba is left for a follow-up (Simon)
- Check the header allocation with ut_assertnonnull() in the test, so
  that ut_assertok() never sees a known -ENOMEM (as Coverity CID 661179
  reports for lib_test_gpt_blkmap())

 disk/part_efi.c     |  47 +++++++++++--
 test/lib/part_efi.c | 163 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 205 insertions(+), 5 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index d8751297fbe..85945634915 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -28,8 +28,15 @@
 #include <linux/compiler.h>
 #include <linux/ctype.h>
 #include <linux/printk.h>
+#include <linux/sizes.h>
 #include <u-boot/crc.h>
 
+/*
+ * Largest partition entry array accepted, as in Linux (KMALLOC_MAX_SIZE with
+ * 4KiB pages): 32768 entries, far more than any tool writes
+ */
+#define GPT_ENTRIES_MAX_SIZE	SZ_4M
+
 /* GUID for basic data partitons */
 #if CONFIG_IS_ENABLED(EFI_PARTITION)
 static const efi_guid_t partition_basic_data_guid = PARTITION_BASIC_DATA_GUID;
@@ -89,10 +96,11 @@ static int get_bootable(gpt_entry *p)
 }
 
 static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
-		lbaint_t lastlba)
+		lbaint_t lastlba, unsigned long blksz)
 {
 	uint32_t crc32_backup = 0;
 	uint32_t calc_crc32;
+	u32 header_size, entry_size, num_entries;
 
 	/* Check the GPT header signature */
 	if (le64_to_cpu(gpt_h->signature) != GPT_HEADER_SIGNATURE_UBOOT) {
@@ -103,12 +111,22 @@ static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
 		return -1;
 	}
 
+	/*
+	 * The CRC covers header_size bytes, so the header has to fit in the
+	 * one block that was read into gpt_h
+	 */
+	header_size = le32_to_cpu(gpt_h->header_size);
+	if (header_size < sizeof(gpt_header) || header_size > blksz) {
+		log_debug("GPT: header_size incorrect: %u not in %zu..%lu\n",
+			  header_size, sizeof(gpt_header), blksz);
+		return -1;
+	}
+
 	/* Check the GUID Partition Table CRC */
 	memcpy(&crc32_backup, &gpt_h->header_crc32, sizeof(crc32_backup));
 	memset(&gpt_h->header_crc32, 0, sizeof(gpt_h->header_crc32));
 
-	calc_crc32 = efi_crc32((const unsigned char *)gpt_h,
-		le32_to_cpu(gpt_h->header_size));
+	calc_crc32 = efi_crc32((const unsigned char *)gpt_h, header_size);
 
 	memcpy(&gpt_h->header_crc32, &crc32_backup, sizeof(crc32_backup));
 
@@ -143,6 +161,24 @@ static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
 		return -1;
 	}
 
+	/*
+	 * The partition entries are used as an array of gpt_entry, read into
+	 * a buffer whose size is the product of these two fields, worked out
+	 * in 32 bits and then rounded up to whole blocks
+	 */
+	entry_size = le32_to_cpu(gpt_h->sizeof_partition_entry);
+	if (entry_size != sizeof(gpt_entry)) {
+		log_debug("GPT: sizeof_partition_entry incorrect: %u != %zu\n",
+			  entry_size, sizeof(gpt_entry));
+		return -1;
+	}
+	num_entries = le32_to_cpu(gpt_h->num_partition_entries);
+	if (num_entries > GPT_ENTRIES_MAX_SIZE / entry_size) {
+		log_debug("GPT: num_partition_entries too large: %u\n",
+			  num_entries);
+		return -1;
+	}
+
 	debug("GPT: first_usable_lba: %llX last_usable_lba: %llX last lba: "
 	      LBAF "\n", le64_to_cpu(gpt_h->first_usable_lba),
 	      le64_to_cpu(gpt_h->last_usable_lba), lastlba);
@@ -925,7 +961,7 @@ int is_valid_gpt_buf(struct blk_desc *desc, void *buf)
 	/* determine start of GPT Header in the buffer */
 	gpt_h = buf + (GPT_PRIMARY_PARTITION_TABLE_LBA * desc->blksz);
 	if (validate_gpt_header(gpt_h, GPT_PRIMARY_PARTITION_TABLE_LBA,
-				desc->lba))
+				desc->lba, desc->blksz))
 		return -1;
 
 	/* determine start of GPT Entries in the buffer */
@@ -1095,7 +1131,8 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
 		return 2;
 	}
 
-	if (validate_gpt_header(pgpt_head, (lbaint_t)lba, desc->lba))
+	if (validate_gpt_header(pgpt_head, (lbaint_t)lba, desc->lba,
+				desc->blksz))
 		return 0;
 
 	if (desc->sig_type == SIG_TYPE_NONE) {
diff --git a/test/lib/part_efi.c b/test/lib/part_efi.c
index 82153fdabfd..42de9109360 100644
--- a/test/lib/part_efi.c
+++ b/test/lib/part_efi.c
@@ -12,6 +12,7 @@
 #include <part.h>
 #include <part_efi.h>
 #include <asm/byteorder.h>
+#include <u-boot/crc.h>
 #include <test/lib.h>
 #include <test/test.h>
 #include <test/ut.h>
@@ -280,4 +281,166 @@ static int lib_test_gpt_blkmap(struct unit_test_state *uts)
 }
 LIB_TEST(lib_test_gpt_blkmap, 0);
 
+/**
+ * gpt_sizes_check() - check the size fields of a GPT header one by one
+ *
+ * Give the primary header of a good table each set of sizes in turn, with
+ * both CRCs recomputed over what those sizes cover, as a crafted disk would
+ * have them. Only the checks on the sizes themselves can then refuse it.
+ *
+ * @uts:	test state
+ * @dev:	blkmap device with nothing mapped yet
+ * @lba:	size of the device in blocks
+ * @buf:	memory to back the device with, @lba blocks of DEFAULT_BLKSZ
+ * @hdr:	two blocks of memory, to rewrite the header in
+ * Return: 0 if OK, other value on error
+ */
+static int gpt_sizes_check(struct unit_test_state *uts, struct udevice *dev,
+			   lbaint_t lba, void *buf, gpt_header *hdr)
+{
+	static const struct {
+		u32 header_size;
+		u32 num_entries;
+		u32 entry_size;
+		int ret;
+	} cases[] = {
+		{ sizeof(gpt_header), GPT_ENTRY_NUMBERS, sizeof(gpt_entry), 0 },
+		/* the reserved rest of the block may be part of the header */
+		{ DEFAULT_BLKSZ, GPT_ENTRY_NUMBERS, sizeof(gpt_entry), 0 },
+		/* a header longer than the block it is read from */
+		{ DEFAULT_BLKSZ + 1, GPT_ENTRY_NUMBERS, sizeof(gpt_entry), -1 },
+		{ sizeof(gpt_header) - 1, GPT_ENTRY_NUMBERS, sizeof(gpt_entry),
+		  -1 },
+		/* entries are only ever read as a gpt_entry array */
+		{ sizeof(gpt_header), GPT_ENTRY_NUMBERS, sizeof(gpt_entry) / 2,
+		  -1 },
+		{ sizeof(gpt_header), GPT_ENTRY_NUMBERS, sizeof(gpt_entry) * 2,
+		  -1 },
+		/* an array whose size comes to a single entry in 32 bits */
+		{ sizeof(gpt_header), U32_MAX / sizeof(gpt_entry) + 2,
+		  sizeof(gpt_entry), -1 },
+	};
+
+	char str_disk_guid[UUID_STR_LEN + 1] =
+		"8d60b397-1bb6-4d33-80ee-b1587d24c2f8";
+	const unsigned long blksz = DEFAULT_BLKSZ;
+	struct disk_partition parts[1], info;
+	struct blk_desc *desc;
+	struct udevice *blk;
+	gpt_entry *gpt_pte;
+	void *entries;
+	int i, ret;
+
+	ut_assertok(blkmap_map_mem(dev, 0, lba, buf));
+	ut_assertok(blk_get_from_parent(dev, &blk));
+	desc = dev_get_uclass_plat(blk);
+
+	memset(parts, '\0', sizeof(parts));
+	strcpy((char *)parts[0].name, "first");
+	parts[0].size = 0x100;
+	disk_partition_set_uuid(&parts[0],
+				"a2a0d0eb-e5b9-3344-87c0-68b6b72699c7");
+	ut_assertok(gpt_restore(desc, str_disk_guid, parts, 1));
+
+	for (i = 0; i < ARRAY_SIZE(cases); i++) {
+		u32 header_size = cases[i].header_size;
+		u32 array_size;
+
+		/* start from the primary header as it is on the disk */
+		memset(hdr, '\0', 2 * blksz);
+		ut_asserteq(1, blk_dread(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
+					 1, hdr));
+
+		hdr->header_size = cpu_to_le32(header_size);
+		hdr->num_partition_entries = cpu_to_le32(cases[i].num_entries);
+		hdr->sizeof_partition_entry = cpu_to_le32(cases[i].entry_size);
+
+		/* the array CRC covers the size as the parser works it out */
+		array_size = cases[i].num_entries * cases[i].entry_size;
+		entries = buf + le64_to_cpu(hdr->partition_entry_lba) * blksz;
+		hdr->partition_entry_array_crc32 =
+			cpu_to_le32(crc32(0, entries, array_size));
+		hdr->header_crc32 = 0;
+		hdr->header_crc32 = cpu_to_le32(crc32(0, (void *)hdr,
+						      header_size));
+		ut_asserteq(1, blk_dwrite(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
+					  1, hdr));
+
+		/*
+		 * Read it back into a buffer as large as the one above, so
+		 * that a header longer than its block is refused by the size
+		 * check rather than by a CRC over whatever follows the buffer
+		 */
+		memset(hdr, '\0', 2 * blksz);
+		gpt_pte = NULL;
+		ret = gpt_verify_headers(desc, hdr, &gpt_pte);
+		free(gpt_pte);
+		ut_assertf(ret == cases[i].ret,
+			   "case %d: header %u, %u entries of %u: %d\n", i,
+			   header_size, cases[i].num_entries,
+			   cases[i].entry_size, ret);
+
+		/*
+		 * fastboot checks a GPT it is sent in memory the same way.
+		 * There a header longer than its block is also refused by its
+		 * CRC, which then takes in the first entry that follows it.
+		 */
+		ut_asserteq(cases[i].ret, is_valid_gpt_buf(desc, buf));
+
+		/* either way the disk still reads, from the backup if needed */
+		desc->part_type = PART_TYPE_UNKNOWN;
+		part_init(desc);
+		ut_asserteq(PART_TYPE_EFI, desc->part_type);
+		ut_assertok(part_get_info(desc, 1, &info));
+		ut_asserteq_str((char *)parts[0].name, (char *)info.name);
+		ut_asserteq_64(parts[0].size, info.size);
+	}
+
+	return 0;
+}
+
+/*
+ * A GPT header gives its own size and the size of its partition entries, and
+ * its CRCs cover only what those say, so a CRC does not show that either size
+ * is one the parser can use. Check that a primary header with a size the
+ * parser cannot use is refused, CRCs or not, and that the backup is used.
+ */
+static int lib_test_gpt_sizes(struct unit_test_state *uts)
+{
+	const unsigned long blksz = DEFAULT_BLKSZ;
+	const lbaint_t lba = 0x400;
+	struct udevice *dev;
+	gpt_header *hdr;
+	int ret, err;
+	void *buf;
+
+	buf = memalign(ARCH_DMA_MINALIGN, lba * blksz);
+	ut_assertnonnull(buf);
+	memset(buf, '\0', lba * blksz);
+
+	hdr = malloc_cache_aligned(2 * blksz);
+	if (!hdr) {
+		free(buf);
+		ut_assertnonnull(hdr);
+	}
+	ret = blkmap_create(GPT_TEST_LABEL, &dev);
+	if (ret) {
+		free(hdr);
+		free(buf);
+		ut_assertok(ret);
+	}
+
+	/* clean up here, as lib_test_gpt_blkmap() explains */
+	ret = gpt_sizes_check(uts, dev, lba, buf, hdr);
+	err = blkmap_destroy(dev);
+	free(hdr);
+	free(buf);
+	if (ret)
+		return ret;
+	ut_assertok(err);
+
+	return 0;
+}
+LIB_TEST(lib_test_gpt_sizes, 0);
+
 #endif /* BLKMAP */

[PATCH v2 10/12] disk: part_dos: Walk the extended partition tables in a bounded loop

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

part_get_info_extended() and print_partition_extended() follow the
links from one extended partition table to the next by recursing, and
stop at MAX_EXT_PARTS partitions, the limit added for CVE-2019-13103.
But a link adds no partition: an extended partition table holding only
a link back to itself does not advance part_num, so the recursion goes
on until the stack runs out, whenever a logical partition of the device
is looked up. Even within the limit, every level keeps a buffer of one
block on the stack, so a long chain of tables on a device with large
blocks needs megabytes of stack.

Follow the links in a loop instead, reusing the one buffer, with the
code that finds the next link in a helper, dos_next_table(), which both
functions use. As Linux does in parse_extended(), stop after
MAX_EXT_EMPTY (16) tables in a row that hold no partition: tables
linking back to each other are then given up after a few reads. Also
stop after MAX_EXT_TABLES tables in all, as many as MAX_EXT_PARTS
partitions need when each table holds one, so that no walk reads more
than that. A chain that also holds tables without a partition then
reads fewer than MAX_EXT_PARTS partitions; partitioning tools write one
logical partition per table.

print_partition_extended() follows every link of a table, where
part_get_info_extended() follows only the first, so it can list
partitions that cannot be looked up, under numbers that clash, and its
walk is not a chain but a tree. Follow only the first link there too.

Add tests with a table holding only a link between two logical
partitions and then a table that links only to itself, and with 15 and
16 such tables in a row before a logical partition.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Use a for loop rather than a backward goto, and move the code that
  follows a link into a helper used by both functions (Simon)
- Also stop after 16 tables in a row without a partition, as Linux does,
  so that the table in the test costs about 20 reads per lookup at probe
  rather than 256 (Simon)
- Tidy the style of the lines the loop re-indents
- Use the present tense in the commit message (Simon)
- Call part_init() in the test rather than setting part_type (Simon)
- Add a table holding only a link to the test, and a test of the limit
  of 16 tables in a row without a partition

 disk/part_dos.c | 289 +++++++++++++++++++++++++++---------------------
 test/dm/part.c  | 175 +++++++++++++++++++++++++++++
 2 files changed, 340 insertions(+), 124 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index 4b560fde1fb..bc40d9d2829 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -28,6 +28,18 @@
  * to use large numbers of partitions */
 #define MAX_EXT_PARTS 256
 
+/*
+ * A link to the next extended partition table adds no partition, so tables
+ * linking back to each other do not advance part_num. As Linux does in
+ * parse_extended(), stop after MAX_EXT_EMPTY tables in a row that hold no
+ * partition, so that such a loop ends after a few reads. Also stop after
+ * MAX_EXT_TABLES tables in all, as many as MAX_EXT_PARTS partitions need,
+ * so that tables with and without partitions in turn cannot make a walk
+ * read more.
+ */
+#define MAX_EXT_EMPTY	16
+#define MAX_EXT_TABLES	MAX_EXT_PARTS
+
 static inline int is_extended(int part_type)
 {
     return (part_type == DOS_PART_TYPE_EXTENDED ||
@@ -126,6 +138,42 @@ static int part_test_dos(struct blk_desc *desc)
 	return 0;
 }
 
+/**
+ * dos_next_table() - Follow the link from a partition table to the next one
+ *
+ * Only the first link of a table is followed: fdisk numbers the logical
+ * partitions along that chain.
+ *
+ * @buffer:		Partition table just read
+ * @ext_part_sector:	Sector of that table (0 for the MBR), updated to the
+ *			sector of the next table
+ * @relative:		Start of the extended partition, which the links of
+ *			its tables are relative to, set from the link in the
+ *			MBR
+ * Return:		true if the table links to a next one, false if not
+ */
+static bool dos_next_table(unsigned char *buffer, lbaint_t *ext_part_sector,
+			   lbaint_t *relative)
+{
+	dos_partition_t *pt;
+	int i;
+
+	pt = (dos_partition_t *)(buffer + DOS_PART_TBL_OFFSET);
+	for (i = 0; i < 4; i++, pt++) {
+		if (is_extended(pt->sys_ind)) {
+			lbaint_t lba_start = get_unaligned_le32(&pt->start_sect) +
+					     *relative;
+
+			if (!*ext_part_sector)
+				*relative = lba_start;
+			*ext_part_sector = lba_start;
+			return true;
+		}
+	}
+
+	return false;
+}
+
 /*  Print a partition that is relative to its Extended partition table
  */
 static void print_partition_extended(struct blk_desc *desc,
@@ -135,65 +183,61 @@ static void print_partition_extended(struct blk_desc *desc,
 {
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, desc->blksz);
 	dos_partition_t *pt;
+	int tables, empty = 0;
 	int i;
 
-	/* set a maximum recursion level */
-	if (part_num > MAX_EXT_PARTS)
-	{
-		printf("** Nested DOS partitions detected, stopping **\n");
-		return;
-    }
-
-	if (blk_dread(desc, ext_part_sector, 1, (ulong *)buffer) != 1) {
-		printf ("** Can't read partition table on %d:" LBAFU " **\n",
-			desc->devnum, ext_part_sector);
-		return;
-	}
-	i=test_block_type(buffer);
-	if (i != DOS_MBR) {
-		printf ("bad MBR sector signature 0x%02x%02x\n",
-			buffer[DOS_PART_MAGIC_OFFSET],
-			buffer[DOS_PART_MAGIC_OFFSET + 1]);
-		return;
-	}
-
-	if (!ext_part_sector)
-		disksig = get_unaligned_le32(&buffer[DOS_PART_DISKSIG_OFFSET]);
+	for (tables = 0; ; tables++) {
+		int prev_num = part_num;
 
-	/* Print all primary/logical partitions */
-	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);
-	for (i = 0; i < 4; i++, pt++) {
-		/*
-		 * fdisk does not show the extended partitions that
-		 * are not in the MBR
-		 */
-
-		if ((pt->sys_ind != 0) &&
-		    (ext_part_sector == 0 || !is_extended (pt->sys_ind)) ) {
-			print_one_part(pt, ext_part_sector, part_num, disksig);
+		/* bound the walk */
+		if (part_num > MAX_EXT_PARTS || tables >= MAX_EXT_TABLES ||
+		    empty >= MAX_EXT_EMPTY) {
+			printf("** Nested DOS partitions detected, stopping **\n");
+			return;
 		}
 
-		/* Reverse engr the fdisk part# assignment rule! */
-		if ((ext_part_sector == 0) ||
-		    (pt->sys_ind != 0 && !is_extended (pt->sys_ind)) ) {
-			part_num++;
+		if (blk_dread(desc, ext_part_sector, 1, (ulong *)buffer) != 1) {
+			printf("** Can't read partition table on %d:" LBAFU " **\n",
+			       desc->devnum, ext_part_sector);
+			return;
+		}
+		i = test_block_type(buffer);
+		if (i != DOS_MBR) {
+			printf("bad MBR sector signature 0x%02x%02x\n",
+			       buffer[DOS_PART_MAGIC_OFFSET],
+			       buffer[DOS_PART_MAGIC_OFFSET + 1]);
+			return;
 		}
-	}
 
-	/* Follows the extended partitions */
-	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);
-	for (i = 0; i < 4; i++, pt++) {
-		if (is_extended (pt->sys_ind)) {
-			lbaint_t lba_start
-				= get_unaligned_le32 (&pt->start_sect) + relative;
+		if (!ext_part_sector)
+			disksig = get_unaligned_le32(&buffer[DOS_PART_DISKSIG_OFFSET]);
+
+		/* Print all primary/logical partitions */
+		pt = (dos_partition_t *)(buffer + DOS_PART_TBL_OFFSET);
+		for (i = 0; i < 4; i++, pt++) {
+			/*
+			 * fdisk does not show the extended partitions that
+			 * are not in the MBR
+			 */
+
+			if ((pt->sys_ind != 0) &&
+			    (ext_part_sector == 0 || !is_extended(pt->sys_ind))) {
+				print_one_part(pt, ext_part_sector, part_num,
+					       disksig);
+			}
 
-			print_partition_extended(desc, lba_start,
-						 !ext_part_sector ? lba_start :
-						 relative, part_num, disksig);
+			/* Reverse engr the fdisk part# assignment rule! */
+			if ((ext_part_sector == 0) ||
+			    (pt->sys_ind != 0 && !is_extended(pt->sys_ind))) {
+				part_num++;
+			}
 		}
-	}
+		empty = part_num > prev_num ? 0 : empty + 1;
 
-	return;
+		/* Follows the extended partitions */
+		if (!dos_next_table(buffer, &ext_part_sector, &relative))
+			break;
+	}
 }
 
 /**
@@ -220,93 +264,90 @@ static int part_get_info_extended(struct blk_desc *desc,
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, desc->blksz);
 	struct disk_partition wdinfo = { 0 };
 	dos_partition_t *pt;
+	int tables, empty = 0;
 	int i, ret;
 	int dos_type;
 
-	/* set a maximum recursion level */
-	if (part_num > MAX_EXT_PARTS)
-	{
-		printf("** Nested DOS partitions detected, stopping **\n");
-		return -1;
-    }
+	for (tables = 0; ; tables++) {
+		int prev_num = part_num;
 
-	if (blk_dread(desc, ext_part_sector, 1, (ulong *)buffer) != 1) {
-		printf ("** Can't read partition table on %d:" LBAFU " **\n",
-			desc->devnum, ext_part_sector);
-		return -1;
-	}
-	if (buffer[DOS_PART_MAGIC_OFFSET] != 0x55 ||
-		buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) {
-		printf ("bad MBR sector signature 0x%02x%02x\n",
-			buffer[DOS_PART_MAGIC_OFFSET],
-			buffer[DOS_PART_MAGIC_OFFSET + 1]);
-		return -1;
-	}
+		/* bound the walk */
+		if (part_num > MAX_EXT_PARTS || tables >= MAX_EXT_TABLES ||
+		    empty >= MAX_EXT_EMPTY) {
+			printf("** Nested DOS partitions detected, stopping **\n");
+			return -1;
+		}
 
-	if (CONFIG_IS_ENABLED(PARTITION_UUIDS) && !ext_part_sector)
-		disksig = get_unaligned_le32(&buffer[DOS_PART_DISKSIG_OFFSET]);
+		if (blk_dread(desc, ext_part_sector, 1, (ulong *)buffer) != 1) {
+			printf("** Can't read partition table on %d:" LBAFU " **\n",
+			       desc->devnum, ext_part_sector);
+			return -1;
+		}
+		if (buffer[DOS_PART_MAGIC_OFFSET] != 0x55 ||
+		    buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) {
+			printf("bad MBR sector signature 0x%02x%02x\n",
+			       buffer[DOS_PART_MAGIC_OFFSET],
+			       buffer[DOS_PART_MAGIC_OFFSET + 1]);
+			return -1;
+		}
 
-	if (info) {
-		ret = part_get_info_whole_disk(desc, &wdinfo);
-		if (ret)
-			return ret;
-	}
+		if (CONFIG_IS_ENABLED(PARTITION_UUIDS) && !ext_part_sector)
+			disksig = get_unaligned_le32(&buffer[DOS_PART_DISKSIG_OFFSET]);
 
-	/* Print all primary/logical partitions */
-	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);
-	for (i = 0; i < 4; i++, pt++) {
-		/*
-		 * fdisk does not show the extended partitions that
-		 * are not in the MBR
-		 */
-		if (((pt->boot_ind & ~0x80) == 0) &&
-		    (pt->sys_ind != 0) &&
-		    (part_num == which_part) &&
-		    (ext_part_sector == 0 || is_extended(pt->sys_ind) == 0)) {
-			if (info) {
-				if (wdinfo.blksz > DOS_PART_DEFAULT_SECTOR)
-					info->blksz = wdinfo.blksz;
-				else
-					info->blksz = DOS_PART_DEFAULT_SECTOR;
-				info->start = (lbaint_t)(ext_part_sector +
-							 get_unaligned_le32(&pt->start_sect));
-				info->size  = (lbaint_t)get_unaligned_le32(&pt->nr_sects);
-				part_set_generic_name(desc, part_num,
-						      (char *)info->name);
-				snprintf(info->type, sizeof(info->type), "0x%x", pt->sys_ind);
-				info->bootable = get_bootable(pt);
-				if (CONFIG_IS_ENABLED(PARTITION_UUIDS)) {
-					char str[12];
-
-					sprintf(str, "%08x-%02x", disksig, part_num);
-					disk_partition_set_uuid(info, str);
+		if (info) {
+			ret = part_get_info_whole_disk(desc, &wdinfo);
+			if (ret)
+				return ret;
+		}
+
+		/* Print all primary/logical partitions */
+		pt = (dos_partition_t *)(buffer + DOS_PART_TBL_OFFSET);
+		for (i = 0; i < 4; i++, pt++) {
+			/*
+			 * fdisk does not show the extended partitions that
+			 * are not in the MBR
+			 */
+			if (((pt->boot_ind & ~0x80) == 0) &&
+			    (pt->sys_ind != 0) &&
+			    (part_num == which_part) &&
+			    (ext_part_sector == 0 || is_extended(pt->sys_ind) == 0)) {
+				if (info) {
+					if (wdinfo.blksz > DOS_PART_DEFAULT_SECTOR)
+						info->blksz = wdinfo.blksz;
+					else
+						info->blksz = DOS_PART_DEFAULT_SECTOR;
+					info->start = ext_part_sector +
+						get_unaligned_le32(&pt->start_sect);
+					info->size = get_unaligned_le32(&pt->nr_sects);
+					part_set_generic_name(desc, part_num,
+							      (char *)info->name);
+					snprintf(info->type, sizeof(info->type),
+						 "0x%x", pt->sys_ind);
+					info->bootable = get_bootable(pt);
+					if (CONFIG_IS_ENABLED(PARTITION_UUIDS)) {
+						char str[12];
+
+						sprintf(str, "%08x-%02x", disksig, part_num);
+						disk_partition_set_uuid(info, str);
+					}
+					info->sys_ind = pt->sys_ind;
 				}
-				info->sys_ind = pt->sys_ind;
+				if (mbr)
+					memcpy(mbr, pt, sizeof(*mbr));
+				return 0;
 			}
-			if (mbr)
-				memcpy(mbr, pt, sizeof(*mbr));
-			return 0;
-		}
 
-		/* Reverse engr the fdisk part# assignment rule! */
-		if ((ext_part_sector == 0) ||
-		    (pt->sys_ind != 0 && !is_extended (pt->sys_ind)) ) {
-			part_num++;
+			/* Reverse engr the fdisk part# assignment rule! */
+			if ((ext_part_sector == 0) ||
+			    (pt->sys_ind != 0 && !is_extended(pt->sys_ind))) {
+				part_num++;
+			}
 		}
-	}
+		empty = part_num > prev_num ? 0 : empty + 1;
 
-	/* Follows the extended partitions */
-	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);
-	for (i = 0; i < 4; i++, pt++) {
-		if (is_extended (pt->sys_ind)) {
-			lbaint_t lba_start
-				= get_unaligned_le32 (&pt->start_sect) + relative;
-
-			return part_get_info_extended(desc, lba_start,
-				 ext_part_sector == 0 ? lba_start : relative,
-						      part_num, which_part, info,
-						      mbr, disksig);
-		}
+		/* Follows the extended partitions */
+		if (!dos_next_table(buffer, &ext_part_sector, &relative))
+			break;
 	}
 
 	/* Check for DOS PBR if no partition is found */
diff --git a/test/dm/part.c b/test/dm/part.c
index ad37d7f406f..4938d962144 100644
--- a/test/dm/part.c
+++ b/test/dm/part.c
@@ -3,10 +3,13 @@
  * Copyright (C) 2020 Sean Anderson <sean.anderson@seco.com>
  */
 
+#include <blk.h>
 #include <dm.h>
 #include <env.h>
+#include <memalign.h>
 #include <mmc.h>
 #include <part.h>
+#include <part_dos.h>
 #include <part_efi.h>
 #include <dm/test.h>
 #include <test/ut.h>
@@ -248,3 +251,175 @@ static int dm_test_part_get_info_by_uuid(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_part_get_info_by_uuid, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/**
+ * dos_set_entry() - fill in one entry of a DOS partition table
+ *
+ * @pt:		entry to fill in
+ * @sys_ind:	partition type
+ * @start:	first sector, relative to what the table type says
+ * @size:	number of sectors
+ */
+static void dos_set_entry(dos_partition_t *pt, u8 sys_ind, u32 start,
+			  u32 size)
+{
+	memset(pt, '\0', sizeof(*pt));
+	pt->sys_ind = sys_ind;
+	pt->start_sect = cpu_to_le32(start);
+	pt->nr_sects = cpu_to_le32(size);
+}
+
+/**
+ * dos_write_table() - write a DOS partition table to a sector
+ *
+ * @desc:	block device to write to, with 512-byte blocks
+ * @lba:	sector to write the table to
+ * @pt:		the four entries of the table
+ * Return: 0 if OK, -EIO if the sector could not be written
+ */
+static int dos_write_table(struct blk_desc *desc, lbaint_t lba,
+			   const dos_partition_t pt[4])
+{
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 512);
+
+	memset(buf, '\0', 512);
+	memcpy(buf + DOS_PART_TBL_OFFSET, pt, 4 * sizeof(*pt));
+	buf[DOS_PART_MAGIC_OFFSET] = 0x55;
+	buf[DOS_PART_MAGIC_OFFSET + 1] = 0xaa;
+
+	return blk_dwrite(desc, lba, 1, buf) == 1 ? 0 : -EIO;
+}
+
+/*
+ * A link from one extended partition table to the next adds no partition,
+ * so a walk through tables that link to each other in a loop finds no more
+ * partitions as it goes. Check that it ends anyway, both when looking for a
+ * partition and when listing them, and that a table holding only a link is
+ * still followed.
+ */
+static int dm_test_part_dos_ext_loop(struct unit_test_state *uts)
+{
+	struct disk_partition info;
+	dos_partition_t pt[4];
+	struct blk_desc *desc;
+
+	ut_asserteq(2, blk_get_device_by_str("mmc", "2", &desc));
+	ut_asserteq(512, desc->blksz);
+
+	/* an MBR with one extended partition, from sector 2 */
+	memset(pt, '\0', sizeof(pt));
+	dos_set_entry(&pt[0], DOS_PART_TYPE_EXTENDED, 2, 100);
+	ut_assertok(dos_write_table(desc, 0, pt));
+
+	/*
+	 * logical partition 5 in a table of its own, then a table holding only
+	 * a link, then logical partition 6; a link is relative to the extended
+	 * partition, a partition to its own table
+	 */
+	dos_set_entry(&pt[0], 0x83, 1, 4);
+	dos_set_entry(&pt[1], DOS_PART_TYPE_EXTENDED, 10, 10);
+	ut_assertok(dos_write_table(desc, 2, pt));
+	memset(pt, '\0', sizeof(pt));
+	dos_set_entry(&pt[0], DOS_PART_TYPE_EXTENDED, 20, 10);
+	ut_assertok(dos_write_table(desc, 12, pt));
+	dos_set_entry(&pt[0], 0x83, 1, 4);
+	dos_set_entry(&pt[1], DOS_PART_TYPE_EXTENDED, 30, 10);
+	ut_assertok(dos_write_table(desc, 22, pt));
+
+	/* then a table with no partition, only a link back to itself */
+	memset(pt, '\0', sizeof(pt));
+	dos_set_entry(&pt[0], DOS_PART_TYPE_EXTENDED, 30, 10);
+	ut_assertok(dos_write_table(desc, 32, pt));
+
+	ut_assertok(part_get_info_by_type(desc, 5, PART_TYPE_DOS, &info));
+	ut_asserteq(3, info.start);
+	ut_asserteq(4, info.size);
+	ut_assertok(part_get_info_by_type(desc, 6, PART_TYPE_DOS, &info));
+	ut_asserteq(23, info.start);
+	ut_asserteq(4, info.size);
+
+	/* there is no partition 7, and looking for it has to end */
+	ut_asserteq(-ENOENT,
+		    part_get_info_by_type(desc, 7, PART_TYPE_DOS, &info));
+	ut_assert_nextline("** Nested DOS partitions detected, stopping **");
+	ut_assert_console_end();
+
+	/* and so does listing them, once the table is detected as DOS */
+	part_init(desc);
+	ut_asserteq(PART_TYPE_DOS, desc->part_type);
+	part_print(desc);
+	ut_assert_skip_to_line("%3d\t%-10d\t%-10d\t%08x-%02x\t%02x", 6, 23, 4,
+			       0, 6, 0x83);
+	ut_assert_nextline("** Nested DOS partitions detected, stopping **");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_part_dos_ext_loop, UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
+
+/**
+ * dos_write_chain() - write a chain of extended partition tables
+ *
+ * The MBR holds an extended partition from sector 2, whose first table
+ * holds logical partition 5 and a link, then come @empty tables holding only
+ * a link, and then a table holding logical partition 6.
+ *
+ * @desc:	block device to write to, with 512-byte blocks
+ * @empty:	number of tables holding only a link
+ * Return: sector where partition 6 starts, or -EIO on error
+ */
+static int dos_write_chain(struct blk_desc *desc, int empty)
+{
+	dos_partition_t pt[4];
+	int i;
+
+	memset(pt, '\0', sizeof(pt));
+	dos_set_entry(&pt[0], DOS_PART_TYPE_EXTENDED, 2, 1000);
+	if (dos_write_table(desc, 0, pt))
+		return -EIO;
+
+	/* table n is at sector 2 + 10 * n, its link 10 * (n + 1) */
+	for (i = 0; i <= empty + 1; i++) {
+		memset(pt, '\0', sizeof(pt));
+		if (i == 0 || i == empty + 1)
+			dos_set_entry(&pt[0], 0x83, 1, 4);
+		if (i <= empty)
+			dos_set_entry(&pt[1], DOS_PART_TYPE_EXTENDED,
+				      10 * (i + 1), 10);
+		if (dos_write_table(desc, 2 + 10 * i, pt))
+			return -EIO;
+	}
+
+	return 2 + 10 * (empty + 1) + 1;
+}
+
+/*
+ * Check that a walk through the extended partition tables follows up to 15
+ * tables in a row holding no partition, as a damaged but valid chain may
+ * have, but gives up at the 16th
+ */
+static int dm_test_part_dos_ext_empty(struct unit_test_state *uts)
+{
+	struct disk_partition info;
+	struct blk_desc *desc;
+	int start;
+
+	ut_asserteq(2, blk_get_device_by_str("mmc", "2", &desc));
+	ut_asserteq(512, desc->blksz);
+
+	start = dos_write_chain(desc, 15);
+	ut_assert(start > 0);
+	ut_assertok(part_get_info_by_type(desc, 6, PART_TYPE_DOS, &info));
+	ut_asserteq(start, info.start);
+	ut_assert_console_end();
+
+	ut_assert(dos_write_chain(desc, 16) > 0);
+	ut_assertok(part_get_info_by_type(desc, 5, PART_TYPE_DOS, &info));
+	ut_asserteq(-ENOENT,
+		    part_get_info_by_type(desc, 6, PART_TYPE_DOS, &info));
+	ut_assert_nextline("** Nested DOS partitions detected, stopping **");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_part_dos_ext_empty, UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);

[PATCH v2 11/12] disk: part_dos: Make room for a three-digit partition number in the UUID

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

part_get_info_extended() makes up the UUID of a DOS partition as
"%08x-%02x" from the disk signature and the partition number, into a
buffer of 12 bytes. That is enough up to partition 255, but
MAX_EXT_PARTS lets the number reach 256, whose three digits make
sprintf() write the terminating NUL past the end of the buffer.

Size the buffer like the UUID it is copied into, and use snprintf().

Add a test with 256 partitions, which catches the overflow on sandbox
with CONFIG_ASAN.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)

 disk/part_dos.c |  6 ++++--
 test/dm/part.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index bc40d9d2829..3029ee0c31a 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -325,9 +325,11 @@ static int part_get_info_extended(struct blk_desc *desc,
 						 "0x%x", pt->sys_ind);
 					info->bootable = get_bootable(pt);
 					if (CONFIG_IS_ENABLED(PARTITION_UUIDS)) {
-						char str[12];
+						/* part_num may take three digits */
+						char str[UUID_STR_LEN + 1];
 
-						sprintf(str, "%08x-%02x", disksig, part_num);
+						snprintf(str, sizeof(str), "%08x-%02x",
+							 disksig, part_num);
 						disk_partition_set_uuid(info, str);
 					}
 					info->sys_ind = pt->sys_ind;
diff --git a/test/dm/part.c b/test/dm/part.c
index 4938d962144..f8a0fa974c1 100644
--- a/test/dm/part.c
+++ b/test/dm/part.c
@@ -423,3 +423,49 @@ static int dm_test_part_dos_ext_empty(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_part_dos_ext_empty, UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
+
+/*
+ * Partition numbers above 255 take three digits in the UUID made up for a
+ * DOS partition, from the disk signature and the partition number
+ */
+static int dm_test_part_dos_uuid(struct unit_test_state *uts)
+{
+	struct disk_partition info;
+	dos_partition_t pt[4];
+	struct blk_desc *desc;
+	int part;
+
+	if (!CONFIG_IS_ENABLED(PARTITION_UUIDS))
+		return -EAGAIN;
+
+	ut_asserteq(2, blk_get_device_by_str("mmc", "2", &desc));
+	ut_asserteq(512, desc->blksz);
+
+	/* an extended partition from sector 2 */
+	memset(pt, '\0', sizeof(pt));
+	dos_set_entry(&pt[0], DOS_PART_TYPE_EXTENDED, 2, 512);
+	ut_assertok(dos_write_table(desc, 0, pt));
+
+	/*
+	 * holding logical partitions 5 to 256, each of one sector after its
+	 * table, with the tables two sectors apart from sector 2 on
+	 */
+	for (part = 5; part <= 256; part++) {
+		memset(pt, '\0', sizeof(pt));
+		dos_set_entry(&pt[0], 0x83, 1, 1);
+		if (part < 256)
+			dos_set_entry(&pt[1], DOS_PART_TYPE_EXTENDED,
+				      (part - 4) * 2, 2);
+		ut_assertok(dos_write_table(desc, 2 + (part - 5) * 2, pt));
+	}
+
+	ut_assertok(part_get_info_by_type(desc, 255, PART_TYPE_DOS, &info));
+	ut_asserteq(2 + 250 * 2 + 1, info.start);
+	ut_asserteq_str("00000000-ff", disk_partition_uuid(&info));
+	ut_assertok(part_get_info_by_type(desc, 256, PART_TYPE_DOS, &info));
+	ut_asserteq(2 + 251 * 2 + 1, info.start);
+	ut_asserteq_str("00000000-100", disk_partition_uuid(&info));
+
+	return 0;
+}
+DM_TEST(dm_test_part_dos_uuid, UTF_SCAN_PDATA | UTF_SCAN_FDT);

[PATCH v2 12/12] efi_loader: Measure the GPT the partition driver uses

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

tcg2_measure_gpt_data() measures the GPT of the disk an EFI application
is started from into PCR 5, so that a remote verifier, or a key sealed
to PCR 5, can tell whether the partition table is the expected one. It
reads the primary GPT itself and measures it without any check, while
the partition driver checks the primary header and entries and, if
either is not valid, finds the partitions in the backup GPT. PCR 5 then
holds a GPT the partitions were not found in. Unused entries are not
measured but are covered by the CRC of the entry array, so a change to
one of them leaves PCR 5 as it was while the partitions come from the
backup GPT, whatever that holds: a disk whose partitions were changed
that way still attests, and unseals, as the original one.

The sizes in the header are not checked either. The entries are read
into a buffer whose size is num_partition_entries *
sizeof_partition_entry, worked out in 32 bits, and walked
num_partition_entries times with sizeof_partition_entry as the stride,
so a product that wraps, or an entry size smaller than the GUID read
from each entry, makes the walks read past the end of the buffer.

Measure the GPT the partitions are found in instead, read by the
partition driver's own code, which checks the sizes too since "disk:
part_efi: Check the header and entry sizes of a GPT". Add
part_get_gpt_primary(), which finds the GPT as find_valid_gpt() does,
and gives a backup header as the primary header that
gpt_repair_headers() makes from it. EDK2 also measures the GPT its
partition driver uses, since 7c12d4359abd
("SecurityPkg/DxeTpm2MeasureBootLib: Use GptLib parser"), and that
driver restores a primary GPT that is not valid from the backup on the
disk before it is measured. Here the disk is not written: the primary
header is only rebuilt in memory, so a disk whose primary GPT was lost
gives the PCR 5 it has once repaired with gpt repair. A disk that is
not a U-Boot block device has no partitions found by that driver, and
gets no GPT event.

If no valid GPT can be read from the disk when the application is
started, measure no GPT event but return success, as an error would
make efi_tcg2_measure_efi_app_invocation() skip the separators.

Add a test that starts an EFI application from a GPT disk on sandbox
and checks PCR 5: it holds the GPT, the same GPT when the primary header
gives an entry array that comes to one block in 32 bits, and the backup
GPT when that differs from a primary GPT with a changed unused entry.
Without this change sandbox crashes in the second case, and PCR 5 holds
the primary GPT in the third. Check part_get_gpt_primary() in
lib_test_gpt_sizes too.

Fixes: ce3dbc5d080d ("efi_loader: add UEFI GPT measurement")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- New subject: measure the GPT the partition driver uses, read with its
  own code through a new part_get_gpt_primary(), so that the CRC of the
  entries is checked too, as EDK2 does (Simon)
- When the partitions come from the backup GPT, measure that, with the
  primary header gpt repair makes from it, rather than no GPT, as EDK2
  measures the GPT its partition driver restores (Ilias)
- Say in the commit message what the bug does to PCR 5 (Ilias)
- Keep validate_gpt_header() static, as efi_tcg2 no longer reads the GPT
  itself
- Test a backup GPT that differs from the primary one, and rename the
  test to test_efi_tcg2_gpt

 disk/part_efi.c                |  21 ++++
 include/part.h                 |  26 ++++
 lib/efi_loader/efi_tcg2.c      |  51 ++++----
 test/lib/part_efi.c            |  42 ++++++-
 test/py/tests/test_efi_tcg2.py | 222 +++++++++++++++++++++++++++++++++
 5 files changed, 334 insertions(+), 28 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 85945634915..3030482d168 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -875,6 +875,27 @@ int gpt_repair_headers(struct blk_desc *desc)
 	return ret;
 }
 
+int part_get_gpt_primary(struct blk_desc *desc, gpt_header *gpt_h,
+			 gpt_entry **gpt_pte)
+{
+	/* This function validates AND fills in the GPT header and PTE */
+	if (find_valid_gpt(desc, gpt_h, gpt_pte) != 1) {
+		/* is_gpt_valid() may leave a freed array here */
+		*gpt_pte = NULL;
+		return -EINVAL;
+	}
+
+	/*
+	 * The partitions come from the backup GPT, as the primary one is not
+	 * valid, or is marked to be ignored: give its header as the primary
+	 * header that gpt_repair_headers() makes from a backup one
+	 */
+	if (le64_to_cpu(gpt_h->my_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
+		restore_primary_gpt_header(gpt_h, desc);
+
+	return 0;
+}
+
 int gpt_verify_partitions(struct blk_desc *desc,
 			  struct disk_partition *partitions, int parts,
 			  gpt_header *gpt_head, gpt_entry **gpt_pte)
diff --git a/include/part.h b/include/part.h
index 1dc510cb6b1..43c82919967 100644
--- a/include/part.h
+++ b/include/part.h
@@ -680,6 +680,32 @@ int get_disk_guid(struct blk_desc *desc, char *guid);
  */
 int part_get_gpt_pte(struct blk_desc *desc, int part, gpt_entry *gpt_e);
 
+/**
+ * part_get_gpt_primary() - Get the GPT the partitions of a disk are found in
+ *
+ * Read the GPT as the partition driver does: the primary one if it is valid,
+ * else the backup one. A backup header is given as the primary header that
+ * gpt_repair_headers() makes from it. Nothing is written to the disk.
+ *
+ * @desc:	block device descriptor
+ * @gpt_h:	buffer of @desc->blksz bytes, filled with the header
+ * @gpt_pte:	set to the partition entries, which the caller frees, or to
+ *		NULL on error
+ *
+ * Return:	0 on success, -EINVAL if no valid GPT was read from the disk,
+ *		-ENOSYS if GPT support is not built in
+ */
+int part_get_gpt_primary(struct blk_desc *desc, gpt_header *gpt_h,
+			 gpt_entry **gpt_pte);
+
+#else
+static inline int part_get_gpt_primary(struct blk_desc *desc,
+				       gpt_header *gpt_h, gpt_entry **gpt_pte)
+{
+	*gpt_pte = NULL;
+
+	return -ENOSYS;
+}
 #endif
 
 #if CONFIG_IS_ENABLED(DOS_PARTITION)
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 5f9bbe97455..65c2833074a 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -16,6 +16,8 @@
 #include <efi_tcg2.h>
 #include <log.h>
 #include <malloc.h>
+#include <memalign.h>
+#include <part.h>
 #include <smbios.h>
 #include <version_string.h>
 #include <tpm_api.h>
@@ -1157,11 +1159,11 @@ tcg2_measure_gpt_data(struct udevice *dev,
 {
 	efi_status_t ret;
 	efi_handle_t handle;
-	struct efi_handler *dp_handler, *io_handler;
+	struct efi_handler *dp_handler;
 	struct efi_device_path *orig_device_path;
 	struct efi_device_path *device_path;
 	struct efi_device_path *dp;
-	struct efi_block_io *block_io;
+	struct blk_desc *desc;
 	struct efi_gpt_data *event = NULL;
 	efi_guid_t null_guid = NULL_GUID;
 	gpt_header *gpt_h;
@@ -1170,7 +1172,6 @@ tcg2_measure_gpt_data(struct udevice *dev,
 	u32 num_of_valid_entry = 0;
 	u32 event_size;
 	u32 i;
-	u32 total_gpt_entry_size;
 
 	ret = efi_search_protocol(&loaded_image->header,
 				  &efi_guid_loaded_image_device_path,
@@ -1202,40 +1203,36 @@ tcg2_measure_gpt_data(struct udevice *dev,
 	if (ret != EFI_SUCCESS)
 		goto out1;
 
-	ret = efi_search_protocol(handle, &efi_block_io_guid, &io_handler);
-	if (ret != EFI_SUCCESS)
+	/*
+	 * Measure the GPT the partitions were found in: read it with the
+	 * partition driver's own code, which checks it and uses the backup GPT
+	 * if the primary one is not valid. Only a block device of U-Boot's has
+	 * partitions found by it.
+	 */
+	if (!handle->dev || device_get_uclass_id(handle->dev) != UCLASS_BLK) {
+		log_debug("Not a U-Boot block device, GPT not measured\n");
+		ret = EFI_SUCCESS;
 		goto out1;
-	block_io = io_handler->protocol_interface;
+	}
+	desc = dev_get_uclass_plat(handle->dev);
 
-	gpt_h = memalign(block_io->media->io_align, block_io->media->block_size);
+	gpt_h = malloc_cache_aligned(desc->blksz);
 	if (!gpt_h) {
 		ret = EFI_OUT_OF_RESOURCES;
 		goto out2;
 	}
 
-	ret = EFI_CALL(block_io->read_blocks(block_io,
-					     block_io->media->media_id, 1,
-					     block_io->media->block_size,
-					     gpt_h));
-	if (ret != EFI_SUCCESS)
-		goto out2;
-
-	/* read GPT entry */
-	total_gpt_entry_size = gpt_h->num_partition_entries *
-			       gpt_h->sizeof_partition_entry;
-	entry = memalign(block_io->media->io_align, total_gpt_entry_size);
-	if (!entry) {
-		ret = EFI_OUT_OF_RESOURCES;
+	/*
+	 * The disk may have changed since its partitions were found, or not
+	 * read. Measure no GPT then, but return success, as an error would
+	 * skip the separators.
+	 */
+	if (part_get_gpt_primary(desc, gpt_h, &entry)) {
+		log_warning("No valid GPT read, not measured\n");
+		ret = EFI_SUCCESS;
 		goto out2;
 	}
 
-	ret = EFI_CALL(block_io->read_blocks(block_io,
-					     block_io->media->media_id,
-					     gpt_h->partition_entry_lba,
-					     total_gpt_entry_size, entry));
-	if (ret != EFI_SUCCESS)
-		goto out2;
-
 	/* count valid GPT entry */
 	gpt_e = entry;
 	for (i = 0; i < gpt_h->num_partition_entries; i++) {
diff --git a/test/lib/part_efi.c b/test/lib/part_efi.c
index 42de9109360..8fcd4148b13 100644
--- a/test/lib/part_efi.c
+++ b/test/lib/part_efi.c
@@ -328,6 +328,7 @@ static int gpt_sizes_check(struct unit_test_state *uts, struct udevice *dev,
 	struct blk_desc *desc;
 	struct udevice *blk;
 	gpt_entry *gpt_pte;
+	gpt_header orig, written;
 	void *entries;
 	int i, ret;
 
@@ -341,6 +342,8 @@ static int gpt_sizes_check(struct unit_test_state *uts, struct udevice *dev,
 	disk_partition_set_uuid(&parts[0],
 				"a2a0d0eb-e5b9-3344-87c0-68b6b72699c7");
 	ut_assertok(gpt_restore(desc, str_disk_guid, parts, 1));
+	ut_asserteq(1, blk_dread(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, 1, hdr));
+	memcpy(&orig, hdr, sizeof(orig));
 
 	for (i = 0; i < ARRAY_SIZE(cases); i++) {
 		u32 header_size = cases[i].header_size;
@@ -365,6 +368,22 @@ static int gpt_sizes_check(struct unit_test_state *uts, struct udevice *dev,
 						      header_size));
 		ut_asserteq(1, blk_dwrite(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
 					  1, hdr));
+		memcpy(&written, hdr, sizeof(written));
+
+		/*
+		 * efi_tcg2 measures the GPT this gives: the header just
+		 * written if it is valid, else the backup header turned into
+		 * the primary one, which is then the header gpt_restore() wrote
+		 */
+		memset(hdr, '\0', 2 * blksz);
+		gpt_pte = NULL;
+		ret = part_get_gpt_primary(desc, hdr, &gpt_pte);
+		free(gpt_pte);
+		ut_assertf(!ret, "case %d: GPT not found: %d\n", i, ret);
+		if (cases[i].ret)
+			ut_asserteq_mem(&orig, hdr, sizeof(gpt_header));
+		else
+			ut_asserteq_mem(&written, hdr, sizeof(gpt_header));
 
 		/*
 		 * Read it back into a buffer as large as the one above, so
@@ -396,6 +415,26 @@ static int gpt_sizes_check(struct unit_test_state *uts, struct udevice *dev,
 		ut_asserteq_64(parts[0].size, info.size);
 	}
 
+	/*
+	 * With the primary header valid again but not its entries, and the
+	 * backup header broken, there is no GPT left, and no entries either
+	 */
+	memset(hdr, '\0', 2 * blksz);
+	memcpy(hdr, &orig, sizeof(orig));
+	ut_asserteq(1, blk_dwrite(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, 1,
+				  hdr));
+	ut_asserteq(1, blk_dread(desc, le64_to_cpu(orig.partition_entry_lba),
+				 1, hdr));
+	((gpt_entry *)hdr)->partition_name[0] ^= 1;
+	ut_asserteq(1, blk_dwrite(desc, le64_to_cpu(orig.partition_entry_lba),
+				  1, hdr));
+	ut_asserteq(1, blk_dread(desc, lba - 1, 1, hdr));
+	hdr->signature = 0;
+	ut_asserteq(1, blk_dwrite(desc, lba - 1, 1, hdr));
+	gpt_pte = (gpt_entry *)hdr;
+	ut_asserteq(-EINVAL, part_get_gpt_primary(desc, hdr, &gpt_pte));
+	ut_assertnull(gpt_pte);
+
 	return 0;
 }
 
@@ -403,7 +442,8 @@ static int gpt_sizes_check(struct unit_test_state *uts, struct udevice *dev,
  * A GPT header gives its own size and the size of its partition entries, and
  * its CRCs cover only what those say, so a CRC does not show that either size
  * is one the parser can use. Check that a primary header with a size the
- * parser cannot use is refused, CRCs or not, and that the backup is used.
+ * parser cannot use is refused, CRCs or not, and that the backup is used, for
+ * the GPT efi_tcg2 measures too.
  */
 static int lib_test_gpt_sizes(struct unit_test_state *uts)
 {
diff --git a/test/py/tests/test_efi_tcg2.py b/test/py/tests/test_efi_tcg2.py
new file mode 100644
index 00000000000..02e1e9e1535
--- /dev/null
+++ b/test/py/tests/test_efi_tcg2.py
@@ -0,0 +1,222 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+"""Test the measurements made by the EFI_TCG2_PROTOCOL on sandbox
+
+The sandbox TPM starts with every PCR clear, and the events measured when an
+EFI application is started (boot variables, GPT, separators) are only measured
+the first time one is. So each case restarts U-Boot, starts one application
+and checks PCR 5 against the events it should then hold.
+"""
+
+import hashlib
+import os
+import re
+import shutil
+import struct
+import zlib
+
+import pytest
+
+import utils
+from tests.fs_helper import FsHelper
+
+# EFI System Partition
+ESP_TYPE = 'C12A7328-F81F-11D2-BA4B-00A0C93EC93B'
+SECTOR = 512
+
+# Fields of the GPT header, by offset
+GPT_HEADER_SIZE = 92
+GPT_HDR_SIZE_OFF = 12
+GPT_HDR_CRC_OFF = 16
+GPT_ALTERNATE_OFF = 32
+GPT_ENTRIES_OFF = 72    # partition_entry_lba, num_partition_entries, ...
+GPT_NUM_ENTRIES_OFF = 80
+GPT_ENTRY_SIZE = 128
+
+
+def extend(pcr, data):
+    """Extend a SHA256 PCR value with the digest of some data
+
+    Args:
+        pcr (bytes): PCR value before
+        data (bytes): Event data
+
+    Returns:
+        bytes: PCR value after
+    """
+    return hashlib.sha256(pcr + hashlib.sha256(data).digest()).digest()
+
+
+def make_gpt_disk(ubman, fname):
+    """Write a GPT disk image with helloworld.efi on its only partition
+
+    Args:
+        ubman (ConsoleBase): U-Boot console
+        fname (str): Filename of the disk image
+    """
+    with FsHelper(ubman.config, 'vfat', 1, 'tcg2') as fsh:
+        shutil.copy(os.path.join(ubman.config.build_dir,
+                                 'lib/efi_loader/helloworld.efi'),
+                    fsh.srcdir)
+        fsh.mk_fs()
+        with open(fsh.fs_img, 'rb') as inf:
+            fs_data = inf.read()
+
+    # 1MiB for the primary GPT, the partition, 1MiB for the backup GPT
+    with open(fname, 'wb') as outf:
+        outf.truncate((2 << 20) + len(fs_data))
+    utils.run_and_log(ubman, f'sfdisk {fname}',
+                      stdin=(f'label: gpt\nstart=2048, '
+                             f'size={len(fs_data) // SECTOR}, '
+                             f'type={ESP_TYPE}\n').encode())
+    with open(fname, 'r+b') as outf:
+        outf.seek(1 << 20)
+        outf.write(fs_data)
+
+
+def gpt_event(fname):
+    """Get the EV_EFI_GPT_EVENT data for the primary GPT of a disk image
+
+    Args:
+        fname (str): Filename of the disk image
+
+    Returns:
+        bytes: The header, the number of partitions, then their entries
+    """
+    with open(fname, 'rb') as inf:
+        disk = inf.read()
+    hdr = disk[SECTOR:SECTOR + GPT_HEADER_SIZE]
+    entry_lba, num, size = struct.unpack_from('<QII', hdr, GPT_ENTRIES_OFF)
+    entries = []
+    for i in range(num):
+        start = entry_lba * SECTOR + i * size
+        entry = disk[start:start + size]
+        if any(entry[:16]):
+            entries.append(entry)
+    return hdr + struct.pack('<Q', len(entries)) + b''.join(entries)
+
+
+def set_num_entries(fname, num):
+    """Set num_partition_entries in the primary GPT header and fix its CRC
+
+    Args:
+        fname (str): Filename of the disk image
+        num (int): Number of partition entries
+    """
+    with open(fname, 'r+b') as outf:
+        outf.seek(SECTOR)
+        hdr = bytearray(outf.read(SECTOR))
+        struct.pack_into('<I', hdr, GPT_NUM_ENTRIES_OFF, num)
+        struct.pack_into('<I', hdr, GPT_HDR_CRC_OFF, 0)
+        hdr_size = struct.unpack_from('<I', hdr, GPT_HDR_SIZE_OFF)[0]
+        struct.pack_into('<I', hdr, GPT_HDR_CRC_OFF,
+                         zlib.crc32(hdr[:hdr_size]))
+        outf.seek(SECTOR)
+        outf.write(hdr)
+
+
+def corrupt_entries(fname):
+    """Change an unused entry of the primary GPT, leaving the header as it is
+
+    The CRC of the entries in the header no longer matches them.
+
+    Args:
+        fname (str): Filename of the disk image
+    """
+    with open(fname, 'r+b') as outf:
+        outf.seek(SECTOR)
+        hdr = outf.read(GPT_HEADER_SIZE)
+        entry_lba = struct.unpack_from('<Q', hdr, GPT_ENTRIES_OFF)[0]
+        # the name of the second entry, whose type GUID stays zero
+        outf.seek(entry_lba * SECTOR + GPT_ENTRY_SIZE + 56)
+        outf.write(b'x')
+
+
+def copy_backup_gpt(src, dst):
+    """Copy the backup GPT of a disk image to another of the same size
+
+    Args:
+        src (str): Filename of the disk image to copy it from
+        dst (str): Filename of the disk image to copy it to
+    """
+    with open(src, 'rb') as inf:
+        inf.seek(SECTOR)
+        hdr = inf.read(GPT_HEADER_SIZE)
+        backup_lba = struct.unpack_from('<Q', hdr, GPT_ALTERNATE_OFF)[0]
+        inf.seek(backup_lba * SECTOR)
+        hdr = inf.read(GPT_HEADER_SIZE)
+        entry_lba = struct.unpack_from('<Q', hdr, GPT_ENTRIES_OFF)[0]
+        inf.seek(entry_lba * SECTOR)
+        data = inf.read()
+    with open(dst, 'r+b') as outf:
+        outf.seek(entry_lba * SECTOR)
+        outf.write(data)
+
+
+def boot_and_read_pcr5(ubman, fname):
+    """Start helloworld.efi from a disk image on a fresh U-Boot
+
+    Args:
+        ubman (ConsoleBase): U-Boot console
+        fname (str): Filename of the disk image
+
+    Returns:
+        bytes: PCR 5 after the application has run
+    """
+    addr = utils.find_ram_base(ubman) + (4 << 20)
+    ubman.restart_uboot()
+    ubman.run_command(f'host bind 0 {fname}')
+    output = ubman.run_command(f'load host 0:1 {addr:x} helloworld.efi')
+    assert 'bytes read' in output
+    output = ubman.run_command(f'bootefi {addr:x}')
+    assert 'Hello, world!' in output
+    output = ubman.run_command(f'tpm2 pcr_read 5 {addr:x}')
+    assert 'PCR #5 sha256 32 byte content' in output
+    digest = output.split('content', 1)[1].split('\n', 1)[1]
+    return bytes.fromhex(''.join(re.findall(r'\b[0-9a-f]{2}\b', digest)))
+
+
+@pytest.mark.boardspec('sandbox')
+# EFI applications are built with the sanitizer but cannot run with it
+@pytest.mark.notbuildconfigspec('asan')
+@pytest.mark.buildconfigspec('efi_tcg2_protocol')
+@pytest.mark.buildconfigspec('efi_partition')
+@pytest.mark.buildconfigspec('cmd_tpm_v2')
+@pytest.mark.buildconfigspec('bootefi_hello_compile')
+def test_efi_tcg2_gpt(ubman):
+    """Check that PCR 5 holds the GPT the partitions are found in
+
+    PCR 5 holds the GPT event and then the separator. When the partition
+    driver refuses the primary GPT, it finds the partitions in the backup
+    one, and the GPT event holds that, with the primary header it restores.
+    """
+    fname = os.path.join(ubman.config.persistent_data_dir, 'tcg2_gpt.img')
+    other = os.path.join(ubman.config.persistent_data_dir, 'tcg2_other.img')
+    separator = struct.pack('<I', 0)
+    try:
+        make_gpt_disk(ubman, fname)
+        expect = extend(extend(bytes(32), gpt_event(fname)), separator)
+        assert boot_and_read_pcr5(ubman, fname) == expect
+
+        # an entry array whose size comes to one block in 32 bits: the
+        # backup GPT is the same GPT, so PCR 5 is too
+        set_num_entries(fname, (1 << 32) // GPT_ENTRY_SIZE +
+                        SECTOR // GPT_ENTRY_SIZE)
+        assert boot_and_read_pcr5(ubman, fname) == expect
+
+        # a backup GPT whose partition has another GUID, and primary entries
+        # that do not match their CRC, though the change is in an unused
+        # entry, which is not measured
+        make_gpt_disk(ubman, fname)
+        shutil.copy(fname, other)
+        utils.run_and_log(ubman, f'sfdisk --part-uuid {other} 1 '
+                          '0a6a1f54-1b55-4d0c-9a52-6d1e5e0e5d01')
+        expect = extend(extend(bytes(32), gpt_event(other)), separator)
+        copy_backup_gpt(other, fname)
+        corrupt_entries(fname)
+        assert boot_and_read_pcr5(ubman, fname) == expect
+    finally:
+        for path in (fname, other):
+            if os.path.exists(path):
+                os.remove(path)
+        ubman.restart_uboot()

Re: [PATCH v2 01/12] treewide: Use is_power_of_2() from linux/log2.h

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> treewide: Use is_power_of_2() from linux/log2.h
>
> The btrfs code and the bcm2835_sdhost driver each define their own
> is_power_of_2(), which clashes with the one in linux/log2.h once a
> header they include brings that in, as include/blk.h does after the next
> patch. Drop both and use the common one.
>
> The btrfs copy is the same as the common one. The bcm2835_sdhost copy
> takes a u64 and also takes 0 as a power of two; its only caller checks
> the block size of a data transfer, which the MMC core never sets to 0,
> and the Linux driver it comes from makes the same check with the common
> is_power_of_2().
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  drivers/mmc/bcm2835_sdhost.c | 6 +-----
>  fs/btrfs/compat.h            | 6 +-----
>  2 files changed, 2 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 02/12] block: Add blk_valid_blksz() to check a block size

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> block: Add blk_valid_blksz() to check a block size
>
> A block driver sets blksz and log2blksz from what the device reports: the
> READ CAPACITY reply of a USB stick or a SCSI disk, the LBA format of an
> NVMe namespace, the sector size of a Xen backend. Apart from the virtio
> block driver and the host command, none of these drivers checks the
> value, yet the partition and filesystem code relies on it:
>
> - part_get_info_extended() and print_partition_extended() read one block
>   into a buffer of blksz bytes and parse the partition table at offset
>   0x1be, past the end of a block smaller than 512 bytes
> - fs_devread() converts bytes to blocks with shifts by log2blksz, masks
>   by blksz - 1 and divisions by blksz, which only agree when blksz is a
>   power of two; with any other value it reads more bytes into the
>   caller's buffer than the caller asked for
> - buffers of blksz bytes go on the stack, so a block size in the
>   gigabytes overruns it
>
> Add blk_valid_blksz(), which accepts a power of two from 512 bytes up to
> BLK_MAX_BLKSZ (64KiB, the largest logical block size Linux accepts), so
> that the block drivers can check what their devices report. Use it in
> the virtio block driver and the host command, which then also refuse a
> block size above 64KiB.
>
> The next patches check the block size in the USB storage, SCSI, IDE and
> NVMe drivers, and the last one in blk_post_probe() for every block
> device.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  cmd/host.c                  |  5 +++--
>  drivers/virtio/virtio_blk.c |  2 +-
>  include/blk.h               | 21 +++++++++++++++++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 03/12] usb: storage: Fix the capacity faked when READ CAPACITY fails

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> usb: storage: Fix the capacity faked when READ CAPACITY fails
>
> When READ CAPACITY fails, usb_stor_get_info() makes up a reply of its own
> for a 1.44MB floppy, 2880 blocks of 512 bytes. But it sets the values in
> host byte order, while the reply is read back as big endian, so on a
> little-endian host the device gets 0x400b0001 blocks (about 1G) of 128KiB
> rather than 2880 of 512 bytes. It also puts 2880 where READ CAPACITY
> returns the last LBA, so even on a big-endian host the device gets 2881
> blocks.
>
> Set the reply in big endian like the one it stands in for, with 2879 as
> the last LBA, so that such a device has 2880 blocks of 512 bytes on any
> host.
>
> Fixes: 149dded2b178 ("* Add support for USB Mass Storage Devices (BBB)   (tested with USB memory sticks only)")
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  common/usb_storage.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 04/12] usb: storage: Refuse a block size the block layer cannot use

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> usb: storage: Refuse a block size the block layer cannot use
>
> usb_stor_get_info() takes the block size from the READ CAPACITY reply of
> the device as it is. Once blk_post_probe() refuses a block size that
> blk_valid_blksz() rejects, at the end of this series, such a device would
> fail to probe there, and usb_stor_probe_device() would stop, skipping the
> other LUNs of the device.
>
> Check the block size where it is read, and treat a LUN reporting one
> that blk_valid_blksz() rejects as unusable, as for a LUN that is not
> ready, saying which LUN is skipped. This also covers the legacy (non-BLK)
> path, which calls part_init() itself.
>
> Add a test where the sandbox flash stick reports an unsupported block
> size.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  arch/sandbox/include/asm/test.h  | 10 ++++++++
>  common/usb_storage.c             |  5 ++++
>  drivers/usb/emul/sandbox_flash.c |  8 +++++++
>  test/dm/usb.c                    | 51 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 06/12] ide: Refuse an ATAPI block size atapi_read() cannot read

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> ide: Refuse an ATAPI block size atapi_read() cannot read
>
> atapi_inquiry() takes the block size from the READ CAPACITY reply of an
> ATAPI device as it is, while atapi_read() asks the device for, and moves
> the buffer on by, ATAPI_READ_BLOCK_SIZE (2048) bytes for every block.
> With any other block size the callers' buffers, which are sized by
> blksz, do not match what atapi_read() writes into them: a 512-byte block
> size makes a read of several blocks write past the end of the buffer.
>
> Some CD drives report another block size although they read 2048-byte
> blocks: 2340 (HP 4020i CD-Recorder), 2352 (Philips CD-Writers) or 0.
> Take these as ATAPI_READ_BLOCK_SIZE, as Linux does in get_sectorsize(),
> so that a drive reporting 0, which ide_probe() skipped until now, is
> read with 2048-byte blocks too. Refuse any other value: a device
> reporting one is left with a zero block size, which ide_probe() skips,
> as for a drive without a medium. This refuses drives with 512-byte
> blocks, such as ZIP drives, whose reads of more than one block already
> overran the buffer; supporting them needs atapi_read() to use the block
> size of the device.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  drivers/block/ide.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 08/12] block: Refuse a device with an unsupported block size

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> block: Refuse a device with an unsupported block size
>
> blk_post_probe() scans a block device for partitions with the block size
> its driver set from what the device reports, and nothing checks it in
> between. The USB storage, SCSI, IDE, NVMe and virtio block drivers and the
> host command now check it where they read it, but other drivers do not,
> such as the Xen PV block driver, and the partition and filesystem code
> relies on it, as the patch adding blk_valid_blksz() describes.
>
> Refuse to probe a block device whose block size blk_valid_blksz()
> rejects, or whose log2blksz does not match its blksz, before its
> partitions are scanned. This covers every block driver in one place
> when its device is probed.
>
> The check does not depend on PARTITIONS, since the filesystem code and
> raw reads rely on the block size too. It is left out of SPL and TPL,
> though: there it adds some 180 bytes, which takes the SPL of imx28_btt3
> and popmetal-rk3288 over their size limits, and its message needs a
> putc() that the SPL of evb-rk3036 does not have. In SPL the USB storage,
> SCSI, IDE, NVMe and virtio block drivers still check the block size
> where they read it.
>
> A device with neither a size nor a block size, which is how the SCSI
> code leaves a removable drive without a medium, is still probed, but
> outside SPL and TPL its partitions are no longer scanned: part_test_dos()
> could otherwise parse 512 bytes of a zero-sized buffer.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  drivers/block/blk-uclass.c | 15 +++++++++++++++
>  test/dm/blk.c              | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 09/12] disk: part_efi: Check the header and entry sizes of a GPT

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> disk: part_efi: Check the header and entry sizes of a GPT
>
> validate_gpt_header() computes the CRC of a GPT header over header_size
> bytes, as the header gives it, and nothing checks the size of the
> partition entries before they are used:
>
> - the header is read into a buffer of one block, on the stack behind
>   part_get_info(), part_print_efi(), gpt_repair_headers() and the gpt
>   command, so a header_size larger than the block size makes the CRC
>   read past the end of that buffer
> - alloc_read_gpt_entries() allocates and reads
>   num_partition_entries * sizeof_partition_entry bytes, computed in 32
>   bits, but the entries are then used as an array of
>   num_partition_entries gpt_entry structures: with a smaller entry size,
>   or a product that wraps, part_get_gpt_pte() and part_print_efi() read
>   past the end of the allocation. On a 32-bit board, rounding a product
>   close to 4GiB up to whole blocks also wraps, to a tiny allocation that
>   the entries are then read into.
>
> As Linux does in is_gpt_valid(), refuse a header whose header_size is
> smaller than the header or larger than the block it is read from, one
> whose sizeof_partition_entry is not sizeof(gpt_entry), and one whose
> entry array is larger than 4MiB (KMALLOC_MAX_SIZE in Linux with 4KiB
> pages, 32768 entries). Larger entries are allowed by the UEFI
> specification, but U-Boot has never read them correctly, and Linux does
> not accept them either.
>
> The header size is checked before the CRC is computed over it. All the
> checks are in validate_gpt_header(), so they also apply to the header
> that is_valid_gpt_buf() checks when fastboot is sent a GPT. That path
> still takes partition_entry_lba from the header without checking it
> against the size of the download, which is left for a follow-up.
>
> Add a test that gives the primary header of a GPT on a blkmap device
> each of these sizes, with CRCs to match, and checks that it is refused
> and that the backup header is used instead.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  disk/part_efi.c     |  47 +++++++++++++--
>  test/lib/part_efi.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 205 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 10/12] disk: part_dos: Walk the extended partition tables in a bounded loop

[Simon Glass]

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> disk: part_dos: Walk the extended partition tables in a bounded loop
>
> part_get_info_extended() and print_partition_extended() follow the
> links from one extended partition table to the next by recursing, and
> stop at MAX_EXT_PARTS partitions, the limit added for CVE-2019-13103.
> But a link adds no partition: an extended partition table holding only
> a link back to itself does not advance part_num, so the recursion goes
> on until the stack runs out, whenever a logical partition of the device
> is looked up. Even within the limit, every level keeps a buffer of one
> block on the stack, so a long chain of tables on a device with large
> blocks needs megabytes of stack.
>
> Follow the links in a loop instead, reusing the one buffer, with the
> code that finds the next link in a helper, dos_next_table(), which both
> functions use. As Linux does in parse_extended(), stop after
> MAX_EXT_EMPTY (16) tables in a row that hold no partition: tables
> linking back to each other are then given up after a few reads. Also
> stop after MAX_EXT_TABLES tables in all, as many as MAX_EXT_PARTS
> partitions need when each table holds one, so that no walk reads more
> than that. A chain that also holds tables without a partition then
> reads fewer than MAX_EXT_PARTS partitions; partitioning tools write one
> logical partition per table.
>
> print_partition_extended() follows every link of a table, where
> part_get_info_extended() follows only the first, so it can list
> partitions that cannot be looked up, under numbers that clash, and its
> walk is not a chain but a tree. Follow only the first link there too.
>
> Add tests with a table holding only a link between two logical
> partitions and then a table that links only to itself, and with 15 and
> 16 such tables in a row before a logical partition.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  disk/part_dos.c | 289 ++++++++++++++++++++++++++++++++------------------------
>  test/dm/part.c  | 175 ++++++++++++++++++++++++++++++++++
>  2 files changed, 340 insertions(+), 124 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 12/12] efi_loader: Measure the GPT the partition driver uses

[Simon Glass]

Hi Ana,

On 2026-10-28T05:50:48, Ana Bobillo <anabobillo3@gmail.com> wrote:
> efi_loader: Measure the GPT the partition driver uses
>
> tcg2_measure_gpt_data() measures the GPT of the disk an EFI application
> is started from into PCR 5, so that a remote verifier, or a key sealed
> to PCR 5, can tell whether the partition table is the expected one. It
> reads the primary GPT itself and measures it without any check, while
> the partition driver checks the primary header and entries and, if
> either is not valid, finds the partitions in the backup GPT. PCR 5 then
> holds a GPT the partitions were not found in. Unused entries are not
> measured but are covered by the CRC of the entry array, so a change to
> one of them leaves PCR 5 as it was while the partitions come from the
> backup GPT, whatever that holds: a disk whose partitions were changed
> that way still attests, and unseals, as the original one.

The commit message is very long for the change. Please can you trim it
to the problem (the GPT measured is not the one the partitions came
from, and its sizes are not checked) and the fix? The EDK2 comparison
and the test-case details could be a sentence each.

>
> The sizes in the header are not checked either. The entries are read
> into a buffer whose size is num_partition_entries *
> sizeof_partition_entry, worked out in 32 bits, and walked
> num_partition_entries times with sizeof_partition_entry as the stride,
> so a product that wraps, or an entry size smaller than the GUID read
> from each entry, makes the walks read past the end of the buffer.
> [...]
>
>  disk/part_efi.c                |  21 ++++
>  include/part.h                 |  26 +++++
>  lib/efi_loader/efi_tcg2.c      |  51 +++++-----
>  test/lib/part_efi.c            |  42 +++++++-
>  test/py/tests/test_efi_tcg2.py | 222 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 334 insertions(+), 28 deletions(-)

> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -875,6 +875,27 @@ int gpt_repair_headers(struct blk_desc *desc)
> +int part_get_gpt_primary(struct blk_desc *desc, gpt_header *gpt_h,
> +			 gpt_entry **gpt_pte)

The name is a bit confusing, since the GPT may come from the backup.
Something like part_get_gpt() or gpt_get_used() would fit better, with
the header comment saying that a backup header is turned into a
primary one. What do you think?

> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -875,6 +875,27 @@ int gpt_repair_headers(struct blk_desc *desc)
> +	/*
> +	 * The partitions come from the backup GPT, as the primary one is not
> +	 * valid, or is marked to be ignored: give its header as the primary
> +	 * header that gpt_repair_headers() makes from a backup one
> +	 */
> +	if (le64_to_cpu(gpt_h->my_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
> +		restore_primary_gpt_header(gpt_h, desc);

This comment does not hold for the ChromeOS 'IGNOREME' case. There
is_gpt_valid() returns 2, which gpt_repair_headers() treats as valid,
so it never rebuilds the primary header. The measured header is then
one that no 'gpt repair' would ever write. Since the primary header is
deliberately ignored, I wonder if measuring the backup header as it is
would be more honest. Either way, please can you correct the comment,
and the matching claim in the commit message?

> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> @@ -1202,40 +1203,36 @@ tcg2_measure_gpt_data(struct udevice *dev,
> +	if (part_get_gpt_primary(desc, gpt_h, &entry)) {
> +		log_warning("No valid GPT read, not measured\n");
> +		ret = EFI_SUCCESS;
> +		goto out2;
> +	}

It would be worth including the device name in the warning, so the
user can tell which disk it refers to.

BTW now that validate_gpt_header() ensures sizeof_partition_entry
equals sizeof(gpt_entry), the two loops below could just index
entry[i] directly. They also use num_partition_entries and
sizeof_partition_entry without le32_to_cpu(), which could be tidied up
in a follow-up.

Regards,
Simon

Re: [PATCH v2 12/12] efi_loader: Measure the GPT the partition driver uses

[Ana Bobillo]

Hi Simon,

On Wed, 28 Oct 2026 at 08:57, Simon Glass <sjg@chromium.org> wrote:
> The commit message is very long for the change. Please can you trim it
> to the problem (the GPT measured is not the one the partitions came
> from, and its sizes are not checked) and the fix? The EDK2 comparison
> and the test-case details could be a sentence each.

OK, I'll cut it down to that in v3.

>> +int part_get_gpt_primary(struct blk_desc *desc, gpt_header *gpt_h,
>> +			 gpt_entry **gpt_pte)
>
> The name is a bit confusing, since the GPT may come from the backup.
> Something like part_get_gpt() or gpt_get_used() would fit better, with
> the header comment saying that a backup header is turned into a
> primary one. What do you think?

Yes, part_get_gpt() reads better, and sits well next to
part_get_gpt_pte(). I'll rename it, and say in the header comment when
a backup header is turned into a primary one and when it is not (below).

>> +	if (le64_to_cpu(gpt_h->my_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
>> +		restore_primary_gpt_header(gpt_h, desc);
>
> This comment does not hold for the ChromeOS 'IGNOREME' case. There
> is_gpt_valid() returns 2, which gpt_repair_headers() treats as valid,
> so it never rebuilds the primary header. The measured header is then
> one that no 'gpt repair' would ever write. Since the primary header is
> deliberately ignored, I wonder if measuring the backup header as it is
> would be more honest. Either way, please can you correct the comment,
> and the matching claim in the commit message?

You're right, I missed that case, and I agree: with IGNOREME the backup
GPT is the one the disk is meant to be used with, and 'gpt repair'
leaves it that way, so the backup header should be measured as it is.

In v3 part_get_gpt() calls is_gpt_valid() on each header itself, in the
same order as find_valid_gpt(), so that it can tell the two cases apart:

- primary GPT not valid: the backup header is turned into the primary
  header that gpt_repair_headers() writes from it, so PCR 5 is the same
  as after a 'gpt repair', as discussed with Ilias on v1
- primary header marked IGNOREME: the backup header is given as it is

I'll correct the comment and the commit message to match, and add an
IGNOREME case to the tests (lib_test_gpt_sizes and the PCR 5 test).

>> +		log_warning("No valid GPT read, not measured\n");
>
> It would be worth including the device name in the warning, so the
> user can tell which disk it refers to.

Will do.

> BTW now that validate_gpt_header() ensures sizeof_partition_entry
> equals sizeof(gpt_entry), the two loops below could just index
> entry[i] directly. They also use num_partition_entries and
> sizeof_partition_entry without le32_to_cpu(), which could be tidied up
> in a follow-up.

Good point. As there will be a v3 anyway, I'll add that as a small
patch of its own after this one, so that this one stays about the fix.

I'll wait a few days for comments on the rest of the series before
sending v3.

Thanks,
Ana