[PATCH] fix: set default verbosity to false in fat parser

[Michael Adler]

This commit changes the default verbosity setting for the FAT parser to
false, aligning it with the general efibootguard behavior where
verbosity is not enabled by default. Users can still opt-in for verbose
output by using the -v or --verbose flag.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/ebgpart.c | 2 +-
tools/fat.c | 6 +++---
tools/fat.h | 2 +-
tools/tests/test_fat.c | 21 +++++++++++++--------
4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index b6384f2..298d766 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -124,7 +124,7 @@ static int check_GPT_FAT_entry(int fd, const struct EFIpartitionentry *e)
strerror(errno));
return -1;
}
- return determine_FAT_bits(&header);
+ return determine_FAT_bits(&header, verbosity);
}

static inline EbgFileSystemType fat_size_to_fs_type(int fat_size)
diff --git a/tools/fat.c b/tools/fat.c
index 0ff0f64..cc9d9ff 100644
--- a/tools/fat.c
+++ b/tools/fat.c
@@ -22,8 +22,6 @@
#include "linux_util.h"
#include "ebgpart.h"

-static bool verbosity = true;
-
#define fat_msg(sb, lvl, ...) do { VERBOSE(stderr, __VA_ARGS__); VERBOSE(stderr, "\n"); } while(0);
#define KERN_ERR "ERROR"

@@ -69,6 +67,8 @@ static int fat_read_bpb(void __attribute__((unused)) *sb,
const struct fat_boot_sector *b, int silent,
struct fat_bios_param_block *bpb)
{
+ bool verbosity = !silent; /* for our VERBOSE macro */
+
int error = -EINVAL;

/* Read in BPB ... */
@@ -147,7 +147,7 @@ out:
/* end of Linux kernel code */
/*****************************************************************************/

-int determine_FAT_bits(const struct fat_boot_sector *sector)
+int determine_FAT_bits(const struct fat_boot_sector *sector, bool verbosity)
{
struct fat_bios_param_block bpb;
if (fat_read_bpb(NULL, sector, !verbosity, &bpb)) {
diff --git a/tools/fat.h b/tools/fat.h
index 8c991ab..d0cf3c7 100644
--- a/tools/fat.h
+++ b/tools/fat.h
@@ -22,4 +22,4 @@
* to ensure it is a valid FAT boot sector. If the provided boot sector is not valid or an error
* occurs during the determination process, the function returns a value less than or equal to 0.
*/
-int determine_FAT_bits(const struct fat_boot_sector *sector);
+int determine_FAT_bits(const struct fat_boot_sector *sector, bool verbosity);
diff --git a/tools/tests/test_fat.c b/tools/tests/test_fat.c
index 2c5089f..42e0133 100644
--- a/tools/tests/test_fat.c
+++ b/tools/tests/test_fat.c
@@ -34,7 +34,7 @@ START_TEST(test_determine_FAT_bits_empty)
{
struct fat_boot_sector sector;
memset(&sector, 0, sizeof(sector));
- int ret = determine_FAT_bits(&sector);
+ int ret = determine_FAT_bits(&sector, true);
ck_assert_int_eq(ret, 0);
}
END_TEST
@@ -48,7 +48,7 @@ START_TEST(test_determine_FAT_bits_sec_per_clus_zero)
.media = 0xf8,
};
u16_to_le(512, sector.sector_size);
- int ret = determine_FAT_bits(&sector);
+ int ret = determine_FAT_bits(&sector, true);
ck_assert_int_eq(ret, 0);
}
END_TEST
@@ -62,7 +62,7 @@ START_TEST(test_determine_FAT_bits_fat_sector_size_zero)
.fats = 16,
.media = 0xf8,
};
- int ret = determine_FAT_bits(&sector);
+ int ret = determine_FAT_bits(&sector, true);
ck_assert_int_eq(ret, 0);
}
END_TEST
@@ -124,7 +124,8 @@ START_TEST(test_determine_FAT_bits_12)
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x55, 0xaa,
};
- int ret = determine_FAT_bits((const struct fat_boot_sector*) &sector);
+ int ret = determine_FAT_bits((const struct fat_boot_sector *)&sector,
+ true);
ck_assert_int_eq(ret, 12);
}
END_TEST
@@ -186,7 +187,8 @@ START_TEST(test_determine_FAT_bits_16)
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x55, 0xaa,
};
- int ret = determine_FAT_bits((const struct fat_boot_sector*) &sector);
+ int ret = determine_FAT_bits((const struct fat_boot_sector *)&sector,
+ true);
ck_assert_int_eq(ret, 16);
}
END_TEST
@@ -248,7 +250,8 @@ START_TEST(test_determine_FAT_bits_32)
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x55, 0xaa,
};
- int ret = determine_FAT_bits((const struct fat_boot_sector*) &sector);
+ int ret = determine_FAT_bits((const struct fat_boot_sector *)&sector,
+ true);
ck_assert_int_eq(ret, 32);
}
END_TEST
@@ -312,7 +315,8 @@ START_TEST(test_determine_FAT_bits_fat16_swupdate)
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x55, 0xaa,
};
- int ret = determine_FAT_bits((const struct fat_boot_sector*) &sector);
+ int ret = determine_FAT_bits((const struct fat_boot_sector *)&sector,
+ true);
ck_assert_int_eq(ret, 16);
}
END_TEST
@@ -373,7 +377,8 @@ START_TEST(test_determine_FAT_bits_squashfs)
0xd7, 0x40, 0xc1, 0xcc, 0x43, 0x7b, 0xbf, 0x8d, 0x76, 0x39,
0x3c, 0xd5,
};
- int ret = determine_FAT_bits((const struct fat_boot_sector *)&sector);
+ int ret = determine_FAT_bits((const struct fat_boot_sector *)&sector,
+ true);
ck_assert_int_eq(ret, 0);
}
END_TEST
--
2.42.0

[Jan Kiszka]

On 07.12.23 11:37, 'Michael Adler' via EFI Boot Guard wrote:
> This commit changes the default verbosity setting for the FAT parser to
> false, aligning it with the general efibootguard behavior where
> verbosity is not enabled by default. Users can still opt-in for verbose
> output by using the -v or --verbose flag.
>

This affects the output of tools only, right?

And is this a regression fix or a cleanup of an inconsistency?

Looks good otherwise.

Jan

--
Siemens AG, Technology
Linux Expert Center

[Michael Adler]

Hi Jan,

> This affects the output of tools only, right?

it impacts userspace, which includes tools and anything using libebgenv.

> And is this a regression fix or a cleanup of an inconsistency?

That's a good question. I guess it could be seen as a regression fix since in
earlier versions of bg_printenv, no messages would appear if efibootguard
encountered a partition that appeared to be FAT but was not actually FAT.

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

[MOESSBAUER, Felix]

On Thu, 2023-12-07 at 14:23 +0100, 'Michael Adler' via EFI Boot Guard
wrote:

> Hi Jan,
>
> > This affects the output of tools only, right?
>
> it impacts userspace, which includes tools and anything using
> libebgenv.
>
> > And is this a regression fix or a cleanup of an inconsistency?
>
> That's a good question. I guess it could be seen as a regression fix
> since in
> earlier versions of bg_printenv, no messages would appear if
> efibootguard
> encountered a partition that appeared to be FAT but was not actually
> FAT.

Hi, I would also consider this a regression. In the end, bg_printenv
does not provide a stable output anyways, so I wouldn't worry too much
about it.

Felix

[Jan Kiszka]

...but not to scripts/cppcheck.sh. Please cross-check as this would
likely also explode in CI:

tools/fat.c:97:4: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
fat_msg(sb, KERN_ERR,
^
tools/fat.c:70:19: note: 'verbosity' is assigned value '!silent' here.

bool verbosity = !silent; /* for our VERBOSE macro */

^
tools/fat.c:96:7: note: outer condition: !silent
if (!silent)
^
tools/fat.c:97:4: note: identical inner condition: verbosity
fat_msg(sb, KERN_ERR,
^
tools/fat.c:103:4: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
fat_msg(sb, KERN_ERR, "bogus number of FAT structure");
^
tools/fat.c:70:19: note: 'verbosity' is assigned value '!silent' here.

bool verbosity = !silent; /* for our VERBOSE macro */

^
tools/fat.c:102:7: note: outer condition: !silent
if (!silent)
^
tools/fat.c:103:4: note: identical inner condition: verbosity
fat_msg(sb, KERN_ERR, "bogus number of FAT structure");
^
tools/fat.c:114:4: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
fat_msg(sb, KERN_ERR, "invalid media value (0x%02x)",
^
tools/fat.c:70:19: note: 'verbosity' is assigned value '!silent' here.

bool verbosity = !silent; /* for our VERBOSE macro */

^
tools/fat.c:113:7: note: outer condition: !silent
if (!silent)
^
tools/fat.c:114:4: note: identical inner condition: verbosity
fat_msg(sb, KERN_ERR, "invalid media value (0x%02x)",
^
tools/fat.c:123:4: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
fat_msg(sb, KERN_ERR, "bogus logical sector size %u",
^
tools/fat.c:70:19: note: 'verbosity' is assigned value '!silent' here.

bool verbosity = !silent; /* for our VERBOSE macro */

^
tools/fat.c:122:7: note: outer condition: !silent
if (!silent)
^
tools/fat.c:123:4: note: identical inner condition: verbosity
fat_msg(sb, KERN_ERR, "bogus logical sector size %u",
^
tools/fat.c:130:4: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
fat_msg(sb, KERN_ERR, "bogus sectors per cluster %u",
^
tools/fat.c:70:19: note: 'verbosity' is assigned value '!silent' here.

bool verbosity = !silent; /* for our VERBOSE macro */

^
tools/fat.c:129:7: note: outer condition: !silent
if (!silent)
^
tools/fat.c:130:4: note: identical inner condition: verbosity
fat_msg(sb, KERN_ERR, "bogus sectors per cluster %u",
^
tools/fat.c:137:4: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
fat_msg(sb, KERN_ERR, "bogus number of FAT sectors");
^
tools/fat.c:70:19: note: 'verbosity' is assigned value '!silent' here.

bool verbosity = !silent; /* for our VERBOSE macro */

^
tools/fat.c:136:7: note: outer condition: !silent
if (!silent)
^
tools/fat.c:137:4: note: identical inner condition: verbosity
fat_msg(sb, KERN_ERR, "bogus number of FAT sectors");
^

[Michael Adler]

This commit changes the default verbosity setting for the FAT parser to
false, aligning it with the general efibootguard behavior where
verbosity is not enabled by default. Users can still opt-in for verbose
output by using the -v or --verbose flag.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/ebgpart.c | 2 +-

tools/fat.c | 10 ++++++----

tools/fat.h | 2 +-
tools/tests/test_fat.c | 21 +++++++++++++--------

4 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index b6384f2..298d766 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -124,7 +124,7 @@ static int check_GPT_FAT_entry(int fd, const struct EFIpartitionentry *e)
strerror(errno));
return -1;
}
- return determine_FAT_bits(&header);
+ return determine_FAT_bits(&header, verbosity);
}

static inline EbgFileSystemType fat_size_to_fs_type(int fat_size)
diff --git a/tools/fat.c b/tools/fat.c

index 0ff0f64..6b055dd 100644
--- a/tools/fat.c
+++ b/tools/fat.c
@@ -22,9 +22,11 @@

#include "linux_util.h"
#include "ebgpart.h"

-static bool verbosity = true;
-

-#define fat_msg(sb, lvl, ...) do { VERBOSE(stderr, __VA_ARGS__); VERBOSE(stderr, "\n"); } while(0);
+#define fat_msg(sb, lvl, ...) \
+ do { \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, "\n"); \
+ } while (0);
#define KERN_ERR "ERROR"

/******************************************************************************
@@ -147,7 +149,7 @@ out:

--
2.42.0

[Jan Kiszka]

Thanks, applied.