Full analysis: gpt_partentry can be marked as aligned8. But following are
problem:
* protocols_per_handle may return unaligned guids
* configuration_tables array may be unaligned. On efi32 every entry is 20
bytes, so guids can't be 8-byte aligned
* The worst offender is device path as it's packed, unpredictable and
contains uuids.
* Efiemu gets guid as argument and probably should be able to handle
unaligned case. So far it's x86 only and we have no mmx and co so it's not
a problem right now unless we enable ubsan.
All in all we do need packed guid type unless those are resolved. I attach
patches for testing. If they work, I'll rearrange them a bit
Le dim. 13 août 2023, 05:27, Vladimir 'phcoder' Serbinenko <
phco...@gmail.com> a écrit :
>
>
> Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <pm...@texair.net> a
> écrit :
>
>>
>>
>> > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz <
>> glaub...@physik.fu-berlin.de> wrote:
>> >
>> > Hi Daniel!
>> >
>> > On Fri, 2023-08-11 at 17:31 +0200, Daniel Kiper wrote:
>> >> On Fri, Aug 11, 2023 at 04:10:14AM -0700, Oliver Steffen wrote:
>> >>> Quoting John Paul Adrian Glaubitz (2023-08-11 10:32:17)
>> >>>> Hi Oliver!
>> >>>>
>> >>>> On Fri, 2023-05-26 at 13:35 +0200, Oliver Steffen wrote:
>> >>>>> There are 3 implementations of a GUID in Grub. Replace them with a
>> >>>>> common one, placed in types.h.
>> >>>>>
>> >>>>> It uses the "packed" flavor of the GUID structs, the alignment
>> attribute
>> >>>>> is dropped, since it is not required.
>> >>>>>
>> >>>>> Signed-off-by: Oliver Steffen <ostef...@redhat.com>
>> >>>>> Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
>> >>
>> >> [...]
>> >>
>> >>>> According to [1], this change broke GRUB on ia64:
>> >>>>
>> >>>> Welcome to GRUB!
>> >>>>
>> >>>> 7 0 0x00006B 0x000000000000001E unexpected trap
>> >>>> 7 0 0x000066 0x000000000000001E trap taken, number in ext PE
>> >>>> 7 0 0x00003C 0x0000000000005A00 trap taken, offset in ext PE
>> >>>>
>> >>>> I assume this is because of the strict alignment requirements on
>> ia64.
>> >>>>
>> >>>> Could you have a look?
>> >>>
>> >>> I am very sorry for this mistake.
>> >>> My goal was to unify the two GUID types we had in grub but I missed
>> the
>> >>> fact that in my "solution" the alignments are not correct in all
>> cases.
>> >>>
>> >>> The quickest way out could be to revert the GUID unification and
>> printf
>> >>> format specifier commits:
>> >>>
>> >>> 6ad116e5f guid: Make use of GUID printf format specifier
>> >>> f82dbf2bd kern/misc: Add a format specifier GUIDs
>> >>> 06edd40db guid: Unify GUID types
>> >>>
>> >>> And use the explicit, long-winded format string for printing the GUID
>> >>> in the bli module instead (added in the commits following those).
>> >>>
>> >>> I am open to suggestions / comments.
>> >>
>> >> Adrian, could you check what will happen when you add alignment to the
>> >> grub_guid_t as it was suggested by Frank here [2]?
>> >>
>> >> Personally I would avoid adding another GUID type with just alignment
>> >> requirement as the difference. Making one GUID type with always
>> enforced
>> >> alignment should not cost us a lot. Or we can enforce alignment on EFI
>> >> platforms only.
>> >
>> > My Itanium hardware is not available for bootloader tests at the
>> moment, so I would
>> > like to ask Pedro Miguel Justo or Frank Scheiner to test the proposed
>> fix.
>>
>>
>> The reason that before this unification there were a packed and aligned
>> types suggest that the packed type was necessary in some cases. Frank had
>> shared the concern against his own suggestion that changing the unified
>> type to be aligned (as opposite to packed) would likely regress the cases
>> where the packed might be needed/expected.
>>
>> Just for kicks, I gave it a try:
>>
>> ```
>> diff --git a/include/grub/types.h b/include/grub/types.h
>> index 0d96006fe..ff415c970 100644
>> --- a/include/grub/types.h
>> +++ b/include/grub/types.h
>> @@ -376,7 +376,7 @@ struct grub_guid
>> grub_uint16_t data2;
>> grub_uint16_t data3;
>> grub_uint8_t data4[8];
>> -} GRUB_PACKED;
>> +} __attribute__ ((aligned(8)));
>> typedef struct grub_guid grub_guid_t;
>>
>> #endif /* ! GRUB_TYPES_HEADER */
>> ```
>>
>> As hypothesized, there are places where these structs (now unified) are
>> expected to be packed, meaning to have an alignment of 1 (even smaller than
>> the natural alignment of its larger member).
>>
>> One of the cases where the dependency is present is in the
>> grub_gpt_partentry structure. This structure is packed and includes a GUID
>> field. One struct of an enforced alignment ’n’ cannot host a member with
>> enforced alignment ‘m’ where m > n. Thus, an 8 byte aligned grub_guid
>> cannot be hosted inside a 1 byte aligned grub_gpt_partentry:
>>
>
> grub_gpt_partentry doesn't need
>
> GRUB_PACKED. To be on a safe side we can
>
> add compile time assert on its size
>
>
>
>> ```
>> In file included from grub-core/disk/ldm.c:26:
>> ./include/grub/gpt_partition.h:70:1: error: alignment 1 of ‘struct
>> grub_gpt_partentry’ is less than 8 [-Werror=packed-not-aligned]
>> 70 | } GRUB_PACKED;
>> | ^
>> ./include/grub/gpt_partition.h:70:1: error: alignment 1 of ‘struct
>> grub_gpt_partentry’ is less than 8 [-Werror=packed-not-aligned]
>> ```
>>
>> Now, we could go this rabbit hole on and try to hack grub_gpt_partentry,
>> or special-case the GUID inside grub_gpt_partentry to be packed… but at
>> which point will I just end up with the thing we were trying to avoid: more
>> than one definition of GUID?
>>
>> >
>> > Thanks,
>> > Adrian
>> >
>> > --
>> > .''`. John Paul Adrian Glaubitz
>> > : :' : Debian Developer
>> > `. `' Physicist
>> > `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
>>
>>
From 2464b685824e09f9e970497e30f43860e3d40ce2 Mon Sep 17 00:00:00 2001
From: Vladimir Serbinenko <phco...@gmail.com>
Date: Sun, 13 Aug 2023 09:17:22 +0200
Subject: [PATCH 2/5] Mark grub_gpt_partentry as aligned to 8 bytes
---
include/grub/gpt_partition.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h
index 19df7dfff..3c70b148d 100644
--- a/include/grub/gpt_partition.h
+++ b/include/grub/gpt_partition.h
@@ -74,7 +74,7 @@ struct grub_gpt_partentry
grub_uint64_t end;
grub_uint64_t attrib;
char name[72];
-} GRUB_PACKED;
+} __attribute__((aligned(8)));
grub_err_t
grub_gpt_partition_map_iterate (grub_disk_t disk,
--
2.39.2
From 74d86aa1e0ec87fa017ac661f033e1566b51088d Mon Sep 17 00:00:00 2001
From: Vladimir Serbinenko <phco...@gmail.com>
Date: Sun, 13 Aug 2023 09:15:54 +0200
Subject: [PATCH 1/5] Add missing static qualifier
---
grub-core/commands/efi/lsefi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
index 05e2e511e..95cba5baf 100644
--- a/grub-core/commands/efi/lsefi.c
+++ b/grub-core/commands/efi/lsefi.c
@@ -29,7 +29,7 @@
GRUB_MOD_LICENSE ("GPLv3+");
-struct known_protocol
+static struct known_protocol
{
grub_guid_t guid;
const char *name;
--
2.39.2
From 3cd891ec5e8c51fe9e0d4872aa426cdd1335166e Mon Sep 17 00:00:00 2001
From: Vladimir Serbinenko <phco...@gmail.com>
Date: Sun, 13 Aug 2023 09:18:53 +0200
Subject: [PATCH 4/5] Add compile time asserts for guid and gpt_partentry sizes
---
grub-core/partmap/gpt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
index 877ceefc3..426f616ae 100644
--- a/grub-core/partmap/gpt.c
+++ b/grub-core/partmap/gpt.c
@@ -229,6 +229,9 @@ static struct grub_partition_map grub_gpt_partition_map =
GRUB_MOD_INIT(part_gpt)
{
+ COMPILE_TIME_ASSERT(sizeof(grub_guid_t) == 16);
+ COMPILE_TIME_ASSERT(sizeof(grub_packed_guid_t) == 16);
+ COMPILE_TIME_ASSERT(sizeof(struct grub_gpt_partentry) == 128);
grub_partition_map_register (&grub_gpt_partition_map);
}
--
2.39.2
From 580463c0194308149c5a92cd28dde0a6f40bd850 Mon Sep 17 00:00:00 2001
From: Vladimir Serbinenko <phco...@gmail.com>
Date: Sun, 13 Aug 2023 09:18:23 +0200
Subject: [PATCH 3/5] Split aligned and packed guids
---
grub-core/commands/efi/lsefi.c | 2 +-
grub-core/efiemu/runtime/efiemu.c | 14 +++++++-------
grub-core/kern/efi/efi.c | 2 +-
grub-core/kern/misc.c | 2 +-
grub-core/loader/i386/xnu.c | 2 +-
include/grub/efi/api.h | 12 ++++++------
include/grub/efiemu/efiemu.h | 4 ++--
include/grub/efiemu/runtime.h | 2 +-
include/grub/types.h | 11 ++++++++++-
9 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
index 95cba5baf..7b8316d41 100644
--- a/grub-core/commands/efi/lsefi.c
+++ b/grub-core/commands/efi/lsefi.c
@@ -96,7 +96,7 @@ grub_cmd_lsefi (grub_command_t cmd __attribute__ ((unused)),
grub_efi_handle_t handle = handles[i];
grub_efi_status_t status;
grub_efi_uintn_t num_protocols;
- grub_guid_t **protocols;
+ grub_packed_guid_t **protocols;
grub_efi_device_path_t *dp;
grub_printf ("Handle %p\n", handle);
diff --git a/grub-core/efiemu/runtime/efiemu.c b/grub-core/efiemu/runtime/efiemu.c
index c84b30652..b98c0f2b0 100644
--- a/grub-core/efiemu/runtime/efiemu.c
+++ b/grub-core/efiemu/runtime/efiemu.c
@@ -66,7 +66,7 @@ efiemu_convert_pointer (grub_efi_uintn_t debug_disposition,
grub_efi_status_t __grub_efi_api
efiemu_get_variable (grub_efi_char16_t *variable_name,
- const grub_guid_t *vendor_guid,
+ const grub_packed_guid_t *vendor_guid,
grub_efi_uint32_t *attributes,
grub_efi_uintn_t *data_size,
void *data);
@@ -74,11 +74,11 @@ efiemu_get_variable (grub_efi_char16_t *variable_name,
grub_efi_status_t __grub_efi_api
efiemu_get_next_variable_name (grub_efi_uintn_t *variable_name_size,
grub_efi_char16_t *variable_name,
- grub_guid_t *vendor_guid);
+ grub_packed_guid_t *vendor_guid);
grub_efi_status_t __grub_efi_api
efiemu_set_variable (grub_efi_char16_t *variable_name,
- const grub_guid_t *vendor_guid,
+ const grub_packed_guid_t *vendor_guid,
grub_efi_uint32_t attributes,
grub_efi_uintn_t data_size,
void *data);
@@ -416,7 +416,7 @@ EFI_FUNC (efiemu_convert_pointer) (grub_efi_uintn_t debug_disposition,
/* Find variable by name and GUID. */
static struct efi_variable *
-find_variable (const grub_guid_t *vendor_guid,
+find_variable (const grub_packed_guid_t *vendor_guid,
grub_efi_char16_t *variable_name)
{
grub_uint8_t *ptr;
@@ -438,7 +438,7 @@ find_variable (const grub_guid_t *vendor_guid,
grub_efi_status_t __grub_efi_api
EFI_FUNC (efiemu_get_variable) (grub_efi_char16_t *variable_name,
- const grub_guid_t *vendor_guid,
+ const grub_packed_guid_t *vendor_guid,
grub_efi_uint32_t *attributes,
grub_efi_uintn_t *data_size,
void *data)
@@ -464,7 +464,7 @@ EFI_FUNC (efiemu_get_variable) (grub_efi_char16_t *variable_name,
grub_efi_status_t __grub_efi_api EFI_FUNC
(efiemu_get_next_variable_name) (grub_efi_uintn_t *variable_name_size,
grub_efi_char16_t *variable_name,
- grub_guid_t *vendor_guid)
+ grub_packed_guid_t *vendor_guid)
{
struct efi_variable *efivar;
LOG ('l');
@@ -503,7 +503,7 @@ grub_efi_status_t __grub_efi_api EFI_FUNC
grub_efi_status_t __grub_efi_api
EFI_FUNC (efiemu_set_variable) (grub_efi_char16_t *variable_name,
- const grub_guid_t *vendor_guid,
+ const grub_packed_guid_t *vendor_guid,
grub_efi_uint32_t attributes,
grub_efi_uintn_t data_size,
void *data)
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 9bf4f571c..20728af6b 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -1039,7 +1039,7 @@ grub_efi_find_configuration_table(const grub_guid_t *target_guid)
for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
{
- grub_guid_t *guid =
+ grub_packed_guid_t *guid =
&grub_efi_system_table->configuration_table[i].vendor_guid;
if (! grub_memcmp (guid, target_guid, sizeof (grub_guid_t)))
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 2890aad49..499d7b981 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -1068,7 +1068,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
if (*(fmt) == 'G')
{
++fmt;
- grub_guid_t *guid = (grub_guid_t *)(grub_addr_t) curarg;
+ grub_packed_guid_t *guid = (grub_packed_guid_t *)(grub_addr_t) curarg;
write_number (str, &count, max_len, 8, 0, '0', 'x', guid->data1);
write_char (str, &count, max_len, '-');
write_number (str, &count, max_len, 4, 0, '0', 'x', guid->data2);
diff --git a/grub-core/loader/i386/xnu.c b/grub-core/loader/i386/xnu.c
index 4e31ce99a..b91e2f840 100644
--- a/grub-core/loader/i386/xnu.c
+++ b/grub-core/loader/i386/xnu.c
@@ -694,7 +694,7 @@ grub_cpu_xnu_fill_devicetree (grub_uint64_t *fsbfreq_out)
{
void *ptr;
struct grub_xnu_devtree_key *curkey;
- grub_guid_t guid;
+ grub_packed_guid_t guid;
char guidbuf[64];
/* Retrieve current key. */
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index d3eaef3fb..d44d00ad7 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -730,7 +730,7 @@ typedef struct grub_efi_memory_mapped_device_path grub_efi_memory_mapped_device_
struct grub_efi_vendor_device_path
{
grub_efi_device_path_t header;
- grub_guid_t vendor_guid;
+ grub_packed_guid_t vendor_guid;
grub_efi_uint8_t vendor_defined_data[0];
} GRUB_PACKED;
typedef struct grub_efi_vendor_device_path grub_efi_vendor_device_path_t;
@@ -974,7 +974,7 @@ typedef struct grub_efi_cdrom_device_path grub_efi_cdrom_device_path_t;
struct grub_efi_vendor_media_device_path
{
grub_efi_device_path_t header;
- grub_guid_t vendor_guid;
+ grub_packed_guid_t vendor_guid;
grub_efi_uint8_t vendor_defined_data[0];
} GRUB_PACKED;
typedef struct grub_efi_vendor_media_device_path grub_efi_vendor_media_device_path_t;
@@ -993,7 +993,7 @@ typedef struct grub_efi_file_path_device_path grub_efi_file_path_device_path_t;
struct grub_efi_protocol_device_path
{
grub_efi_device_path_t header;
- grub_guid_t guid;
+ grub_packed_guid_t guid;
} GRUB_PACKED;
typedef struct grub_efi_protocol_device_path grub_efi_protocol_device_path_t;
@@ -1002,7 +1002,7 @@ typedef struct grub_efi_protocol_device_path grub_efi_protocol_device_path_t;
struct grub_efi_piwg_device_path
{
grub_efi_device_path_t header;
- grub_guid_t guid;
+ grub_packed_guid_t guid;
} GRUB_PACKED;
typedef struct grub_efi_piwg_device_path grub_efi_piwg_device_path_t;
@@ -1287,7 +1287,7 @@ struct grub_efi_boot_services
grub_efi_status_t
(__grub_efi_api *protocols_per_handle) (grub_efi_handle_t handle,
- grub_guid_t ***protocol_buffer,
+ grub_packed_guid_t ***protocol_buffer,
grub_efi_uintn_t *protocol_buffer_count);
grub_efi_status_t
@@ -1386,7 +1386,7 @@ typedef struct grub_efi_runtime_services grub_efi_runtime_services_t;
struct grub_efi_configuration_table
{
- grub_guid_t vendor_guid;
+ grub_packed_guid_t vendor_guid;
void *vendor_table;
} GRUB_PACKED;
typedef struct grub_efi_configuration_table grub_efi_configuration_table_t;
diff --git a/include/grub/efiemu/efiemu.h b/include/grub/efiemu/efiemu.h
index caf0b505f..d6a868e94 100644
--- a/include/grub/efiemu/efiemu.h
+++ b/include/grub/efiemu/efiemu.h
@@ -183,13 +183,13 @@ struct grub_efiemu_configuration_table
};
struct grub_efiemu_configuration_table32
{
- grub_guid_t vendor_guid;
+ grub_packed_guid_t vendor_guid;
grub_efi_uint32_t vendor_table;
} GRUB_PACKED;
typedef struct grub_efiemu_configuration_table32 grub_efiemu_configuration_table32_t;
struct grub_efiemu_configuration_table64
{
- grub_guid_t vendor_guid;
+ grub_packed_guid_t vendor_guid;
grub_efi_uint64_t vendor_table;
} GRUB_PACKED;
typedef struct grub_efiemu_configuration_table64 grub_efiemu_configuration_table64_t;
diff --git a/include/grub/efiemu/runtime.h b/include/grub/efiemu/runtime.h
index c9ad9fdfa..2ff429845 100644
--- a/include/grub/efiemu/runtime.h
+++ b/include/grub/efiemu/runtime.h
@@ -29,7 +29,7 @@ struct grub_efiemu_ptv_rel
struct efi_variable
{
- grub_guid_t guid;
+ grub_packed_guid_t guid;
grub_uint32_t namelen;
grub_uint32_t size;
grub_efi_uint32_t attributes;
diff --git a/include/grub/types.h b/include/grub/types.h
index 0d96006fe..4028c32bf 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -376,7 +376,16 @@ struct grub_guid
grub_uint16_t data2;
grub_uint16_t data3;
grub_uint8_t data4[8];
-} GRUB_PACKED;
+} __attribute__ ((aligned(8)));
typedef struct grub_guid grub_guid_t;
+struct grub_packed_guid
+{
+ grub_uint32_t data1;
+ grub_uint16_t data2;
+ grub_uint16_t data3;
+ grub_uint8_t data4[8];
+} __attribute__ ((packed));
+typedef struct grub_packed_guid grub_packed_guid_t;
+
#endif /* ! GRUB_TYPES_HEADER */
--
2.39.2
From cfa21b94ee00035fbff713ea7787fcdc85e21bef Mon Sep 17 00:00:00 2001
From: Vladimir Serbinenko <phco...@gmail.com>
Date: Sun, 13 Aug 2023 09:19:02 +0200
Subject: [PATCH 5/5] Fix typo
---
include/grub/types.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/grub/types.h b/include/grub/types.h
index 4028c32bf..c31c707e3 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -349,7 +349,7 @@ static inline void grub_set_unaligned64 (void *ptr, grub_uint64_t val)
/*
* The grub_absolute_pointer() macro borrows the idea from Linux kernel of using
* RELOC_HIDE() macro to stop GCC from checking the result of pointer arithmetic
- * and also it's conversion to be inside the symbol's boundary [1]. The check
+ * and also its conversion to be inside the symbol's boundary [1]. The check
* is sometimes false positive, especially it is controversial to emit the array
* bounds [-Warray-bounds] warning on all hardwired literal pointers since GCC
* 11/12 [2]. Unless a good solution can be settled, for the time being we
--
2.39.2
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
