From d7c59bfc3b20bc6d602b33ac24d7ae8698650b87 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Thu, 12 Sep 2024 15:41:39 +0200 Subject: [PATCH 1/3] env: mmc: refactor mmc_offset_try_partition() In preparation for fixing the handling of a the case of redundant environment defined in two separate partitions with the U-Boot env GUID, refactor the for () if (str) ... #ifdef CONFIG_FOO if (!str) .. #endif to if (str) for () else if (CONFIG_FOO && !str) for () and put those for loops in separate functions. No functional change intended, but I did change the direct access of info.type_guid into using the disk_partition_type_guid() helper, so that I could avoid the #ifdef and use IS_ENABLED() in the if() statement. Signed-off-by: Rasmus Villemoes --- env/mmc.c | 59 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/env/mmc.c b/env/mmc.c index 0338aa6c56a..db2d35e9bd4 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -53,11 +53,45 @@ DECLARE_GLOBAL_DATA_PTR; #endif #if CONFIG_IS_ENABLED(OF_CONTROL) + +static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str, + struct disk_partition *info) +{ + int i, ret; + + for (i = 1;; i++) { + ret = part_get_info(desc, i, info); + if (ret < 0) + return ret; + + if (!strncmp((const char *)info->name, str, sizeof(info->name))) + return 0; + } +} + +static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info) +{ + const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT; + efi_guid_t type_guid; + int i, ret; + + for (i = 1;; i++) { + ret = part_get_info(desc, i, info); + if (ret < 0) + return ret; + + uuid_str_to_bin(disk_partition_type_guid(info), type_guid.b, UUID_STR_FORMAT_GUID); + if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t))) + return 0; + } +} + + static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val) { struct disk_partition info; struct blk_desc *desc; - int len, i, ret; + int len, ret; char dev_str[4]; snprintf(dev_str, sizeof(dev_str), "%d", mmc_get_env_dev()); @@ -65,24 +99,13 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val) if (ret < 0) return (ret); - for (i = 1;;i++) { - ret = part_get_info(desc, i, &info); - if (ret < 0) - return ret; - - if (str && !strncmp((const char *)info.name, str, sizeof(info.name))) - break; -#ifdef CONFIG_PARTITION_TYPE_GUID - if (!str) { - const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT; - efi_guid_t type_guid; - - uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID); - if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t))) - break; - } -#endif + if (str) { + ret = mmc_env_partition_by_name(desc, str, &info); + } else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) { + ret = mmc_env_partition_by_guid(desc, &info); } + if (ret < 0) + return ret; /* round up to info.blksz */ len = DIV_ROUND_UP(CONFIG_ENV_SIZE, info.blksz); From 9402e3bb8c5dea255af7ad2854aed8f09a08bcf8 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Thu, 12 Sep 2024 15:41:40 +0200 Subject: [PATCH 2/3] env: mmc: do not return an offset before the start of the partition I have an GPT layout containing two partitions with the type GUID for U-Boot environment: partition U-Boot-env-1 { offset = 0x1fc000 size = 0x2000 partition-type-uuid = "3de21764-95bd-54bd-a5c3-4abe786f38a8" } partition U-Boot-env-2 { offset = 0x1fe000 size = 0x2000 partition-type-uuid = "3de21764-95bd-54bd-a5c3-4abe786f38a8" } and have set CONFIG_ENV_OFFSET=0x1fc000, CONFIG_ENV_OFFSET_REDUND=0x1fe000 and CCONFIG_ENV_SIZE=0x2000. This usually works just fine, but on an stm32mp, I was seeing weird behaviour. It turns out that can be tracked down to that board setting CONFIG_PARTITION_TYPE_GUID, so the logic in mmc.c ends up only finding the first of the two partitions, but then in the copy=1 case ends up computing 0x1fa000 as the *val returned (that is, the end of the partition minus two times the environment size). That is of course outside the found partition and leads to random corruption of the partition preceding U-Boot-env-1. Add a sanity check that the partition found is at least as large as needed for the "one or two copies from the end of the partition" logic to at least produce something within that partition. That will also catch a bug where the partition is too small for even one copy of the environment. Signed-off-by: Rasmus Villemoes --- env/mmc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/env/mmc.c b/env/mmc.c index db2d35e9bd4..5d09140655f 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -91,7 +91,8 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val) { struct disk_partition info; struct blk_desc *desc; - int len, ret; + lbaint_t len; + int ret; char dev_str[4]; snprintf(dev_str, sizeof(dev_str), "%d", mmc_get_env_dev()); @@ -110,6 +111,13 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val) /* round up to info.blksz */ len = DIV_ROUND_UP(CONFIG_ENV_SIZE, info.blksz); + if ((1 + copy) * len > info.size) { + printf("Partition '%s' [0x"LBAF"; 0x"LBAF"] too small for %senvironment, required size 0x"LBAF" blocks\n", + (const char*)info.name, info.start, info.size, + copy ? "two copies of " : "", (1 + copy)*len); + return -ENOSPC; + } + /* use the top of the partion for the environment */ *val = (info.start + info.size - (1 + copy) * len) * info.blksz; From c1131aca9adfb2861dd34ba0e45f593874adcd81 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Thu, 12 Sep 2024 15:41:41 +0200 Subject: [PATCH 3/3] env: mmc: rework mmc_env_partition_by_guid() to work with two separate partitions Having two separate partitions for use in a redundant environment setup works just fine, if one only relies on setting CONFIG_ENV_OFFSET and CONFIG_ENV_OFFSET_REDUND. However, if CONFIG_PARTITION_TYPE_GUID is enabled, the current logic in mmc_env_partition_by_guid() means that only the first partition will ever be considered, and prior to the previous commit, lead to silent data corruption. Extend the logic so that, when we are looking for the location for the second copy of the environment, we keep track of whether we have already found one matching partition. If a second match is found, return that, but also modify *copy so that the logic in the caller will use the last ENV_SIZE bytes of that second partition - in my case, and I suppose that would be typical, both partitions have been created with a size of exactly the desired ENV_SIZE. When only a single matching partition exists, the behaviour is unchanged: We return that single partition, and *copy is left as-is, so the logic in the caller will either use the last (copy==0) or second-to-last (copy==1) ENV_SIZE bytes. Signed-off-by: Rasmus Villemoes --- env/mmc.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/env/mmc.c b/env/mmc.c index 5d09140655f..e2f8e7ece28 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -69,21 +69,49 @@ static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str, } } -static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info) +/* + * Look for one or two partitions with the U-Boot environment GUID. + * + * If *copy is 0, return the first such partition. + * + * If *copy is 1 on entry and two partitions are found, return the + * second partition and set *copy = 0. + * + * If *copy is 1 on entry and only one partition is found, return that + * partition, leaving *copy unmodified. + */ +static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info, + int *copy) { const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT; efi_guid_t type_guid; - int i, ret; + int i, ret, found = 0; + struct disk_partition dp; for (i = 1;; i++) { - ret = part_get_info(desc, i, info); + ret = part_get_info(desc, i, &dp); if (ret < 0) - return ret; + break; - uuid_str_to_bin(disk_partition_type_guid(info), type_guid.b, UUID_STR_FORMAT_GUID); - if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t))) - return 0; + uuid_str_to_bin(disk_partition_type_guid(&dp), type_guid.b, UUID_STR_FORMAT_GUID); + if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t))) { + memcpy(info, &dp, sizeof(dp)); + /* If *copy is 0, we are only looking for the first partition. */ + if (*copy == 0) + return 0; + /* This was the second such partition. */ + if (found) { + *copy = 0; + return 0; + } + found = 1; + } } + + /* The loop ended after finding at most one matching partition. */ + if (found) + ret = 0; + return ret; } @@ -103,7 +131,7 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val) if (str) { ret = mmc_env_partition_by_name(desc, str, &info); } else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) { - ret = mmc_env_partition_by_guid(desc, &info); + ret = mmc_env_partition_by_guid(desc, &info, ©); } if (ret < 0) return ret;