[PATCH] fix: ensure device probing continues after non-FAT partition error

[Michael Adler]

This fixes a regression introduced in commit b23816ab9626 which results
in efibootguard skipping the config file probing of the whole device if
it encounters some partition for which it could not determine the FAT
bit size.

Details:

The expected behavior of the `check_GPT_FAT_entry` function is to return
false if (and only if!) an actual error occurs. Being unable to
determine the FAT bit size must not be considered an error. The solution
is to return true in this case as well.

Fixes: b23816ab9626 ("fix: correctly calculate FAT bit size based on cluster count")
Reported-by: Felix Moessbauer <felix.mo...@siemens.com>
Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/ebgpart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index 6f60c24..b168903 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -121,7 +121,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
int fat_bits = determine_FAT_bits(&header);
if (fat_bits <= 0) {
/* not a FAT header */
- return false;
+ return true;
}
if (asprintf(&pfst->name, "fat%d", fat_bits) == -1) {
VERBOSE(stderr,
--
2.42.0

[Jan Kiszka]

On 17.10.23 17:42, 'Michael Adler' via EFI Boot Guard wrote:
> This fixes a regression introduced in commit b23816ab9626 which results
> in efibootguard skipping the config file probing of the whole device if
> it encounters some partition for which it could not determine the FAT
> bit size.
>
> Details:
>
> The expected behavior of the `check_GPT_FAT_entry` function is to return
> false if (and only if!) an actual error occurs. Being unable to
> determine the FAT bit size must not be considered an error. The solution
> is to return true in this case as well.
>

This looks fishy: We are now adding a partition in read_GPT_entries to
the list which is NOT a FAT partition. Why should that be correct?

Jan

> Fixes: b23816ab9626 ("fix: correctly calculate FAT bit size based on cluster count")
> Reported-by: Felix Moessbauer <felix.mo...@siemens.com>
> Signed-off-by: Michael Adler <michae...@siemens.com>
> ---
> tools/ebgpart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/ebgpart.c b/tools/ebgpart.c
> index 6f60c24..b168903 100644
> --- a/tools/ebgpart.c
> +++ b/tools/ebgpart.c
> @@ -121,7 +121,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
> int fat_bits = determine_FAT_bits(&header);
> if (fat_bits <= 0) {
> /* not a FAT header */
> - return false;
> + return true;
> }
> if (asprintf(&pfst->name, "fat%d", fat_bits) == -1) {
> VERBOSE(stderr,

--

Siemens AG, Technology
Linux Expert Center

[Michael Adler]

Hi Jan,

> This looks fishy: We are now adding a partition in read_GPT_entries to
> the list which is NOT a FAT partition. Why should that be correct?

it looks fishy because it's not so obvious what's actually going on. It took
me a while to understand the "grand scheme of things", so here is a brief
summary:

1. `check_GPT_FAT_entry` does more than just checking the FAT entry, it
actually writes a string (fatXX) to one of its arguments (pfst->name) if it
was able to determine the FAT partition type.

2. The return value of `check_GPT_FAT_entry` is a boolean indicating success.
If the return value is false (i.e. error), then `dev->part_list` is set to
NULL. The consequence is that *every* partition of the underlying device is
skipped. In other words, a single bad partition results in the whole device
being ignored.

3. Whether we probe (i.e. mount) a partition and check for BGENV.DAT depends
on the string value pfst->name which is written in step 1), see
`probe_config_partitions`.

Kind Regards,
Michael

--
Michael Adler

Siemens AG
Technology
Connectivity & Edge
Smart Embedded Systems
T CED SES-DE
Otto-Hahn-Ring 6
81739 Munich, Germany

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Jim Hagemann
Snabe; Managing Board: Roland Busch, Chairman, President and Chief Executive
Officer; Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese;
Registered offices: Berlin and Munich, Germany; Commercial registries:
Berlin-Charlottenburg, HRB 12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322

[Jan Kiszka]

On 18.10.23 11:00, Michael Adler wrote:
> Hi Jan,
>
>> This looks fishy: We are now adding a partition in read_GPT_entries to
>> the list which is NOT a FAT partition. Why should that be correct?
>
> it looks fishy because it's not so obvious what's actually going on. It took
> me a while to understand the "grand scheme of things", so here is a brief
> summary:
>
> 1. `check_GPT_FAT_entry` does more than just checking the FAT entry, it
> actually writes a string (fatXX) to one of its arguments (pfst->name) if it
> was able to determine the FAT partition type.
>
> 2. The return value of `check_GPT_FAT_entry` is a boolean indicating success.
> If the return value is false (i.e. error), then `dev->part_list` is set to
> NULL. The consequence is that *every* partition of the underlying device is
> skipped. In other words, a single bad partition results in the whole device
> being ignored.
>
> 3. Whether we probe (i.e. mount) a partition and check for BGENV.DAT depends
> on the string value pfst->name which is written in step 1), see
> `probe_config_partitions`.
>

Oookay... Then how about fixing this in code in a way that it no longer
needs such out-of-band documentation?

char *get_partition_type(int fd, struct EFIpartitionentry *e,
uint32_t partnum)

Would return the type name in a malloc'ed string or NULL on error. Maybe
there are even better ideas.

Jan

[Michael Adler]

This is patch series 2. I have created two commits for easier reviewing, but
they can be squashed if no further changes are requested.

Michael Adler (2):
fix: continue device probing after non-FAT partitions
refactor: replace fs type string with enum for clarity and efficiency

env/env_config_partitions.c | 7 +-
include/ebgpart.h | 13 ++--
tools/ebgpart.c | 133 ++++++++++++++++--------------------
tools/tests/fake_devices.c | 16 +----
4 files changed, 72 insertions(+), 97 deletions(-)

--
2.42.0

[JEMS EBERHARD HORBEL]

DIRECT SENDER IS HERE LETS DEAL.

JENS EBERHARD



MT103/202 DIRECT WIRE TRANSFER
PAYPAL TRANSFER
CASHAPP TRANSFER
ZELLE TRANSFER
TRANSFER WISE
WESTERN UNION TRANSFER
BITCOIN FLASHING 
BANK ACCOUNT LOADING/FLASHING
IBAN TO IBAN TRANSFER
MONEYGRAM TRANSFER
SLBC PROVIDER
CREDIT CARD TOP UP
SEPA TRANSFER
WIRE TRANSFER
GLOBALPAY INC US

Thanks.


NOTE; ONLY SERIOUS / RELIABLE RECEIVERS CAN CONTACT.

DM ME ON WHATSAPP FOR A SERIOUS DEAL.

+447405129573