Hi Vladimir. Thanks for the analysis and for providing the candidate patches. I'll be away from my computer for a couple of days but will give it a try as soon as I can find a sliver of time.
Pedro On Aug 13, 2023, at 08:47, Vladimir 'phcoder' Serbinenko <phco...@gmail.com> wrote: 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<mailto:phco...@gmail.com>> a écrit : Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <pm...@texair.net<mailto:pm...@texair.net>> a écrit : > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz > <glaub...@physik.fu-berlin.de<mailto: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<mailto:ostef...@redhat.com>> >>>>> Reviewed-by: Daniel Kiper >>>>> <daniel.ki...@oracle.com<mailto: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 <0002-Mark-grub_gpt_partentry-as-aligned-to-8-bytes.patch> <0001-Add-missing-static-qualifier.patch> <0004-Add-compile-time-asserts-for-guid-and-gpt_partentry-.patch> <0003-Split-aligned-and-packed-guids.patch> <0005-Fix-typo.patch>
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
