Send Grub-devel mailing list submissions to
grub-devel@gnu.org
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.gnu.org/mailman/listinfo/grub-devel
or, via email, send a message with subject or body 'help' to
grub-devel-request@gnu.org
You can reach the person managing the list at
grub-devel-owner@gnu.org
When replying, please edit your Subject line so it is more specific
than "Re: Contents of Grub-devel digest..."
Today's Topics:
1. Re: [PATCH v9 02/11] Unify GUID types
(Vladimir 'phcoder' Serbinenko)
----------------------------------------------------------------------
Message: 1
Date: Sun, 13 Aug 2023 09:46:45 +0200
From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
To: Pedro Miguel Justo <pmsjt@texair.net>
Cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>, The
development of GNU GRUB <grub-devel@gnu.org>, Oliver Steffen
<osteffen@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Frank
Scheiner <frank.scheiner@web.de>
Subject: Re: [PATCH v9 02/11] Unify GUID types
Message-ID:
<CAEaD8JOWL9gyWt6qs36pDFioi3uE86YQhp=etjiR47qgJk6X1g@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
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 <
phcoder@gmail.com> a écrit :
>
>
> Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <pmsjt@texair.net> a
> écrit :
>
>>
>>
>> > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz <
>> glaubitz@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 <osteffen@redhat.com>
>> >>>>> Reviewed-by: Daniel Kiper <daniel.kiper@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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Mark-grub_gpt_partentry-as-aligned-to-8-bytes.patch
Type: text/x-diff
Size: 723 bytes
Desc: not available
URL: <https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-missing-static-qualifier.patch
Type: text/x-diff
Size: 644 bytes
Desc: not available
URL: <https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-compile-time-asserts-for-guid-and-gpt_partentry-.patch
Type: text/x-diff
Size: 821 bytes
Desc: not available
URL: <https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Split-aligned-and-packed-guids.patch
Type: text/x-diff
Size: 9171 bytes
Desc: not available
URL: <https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Fix-typo.patch
Type: text/x-diff
Size: 1078 bytes
Desc: not available
URL: <https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0004.patch>
------------------------------
Subject: Digest Footer
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
------------------------------
End of Grub-devel Digest, Vol 234, Issue 14
*******************************************