On Mon, Oct 25, 2021 at 03:11:57PM +0200, Ard Biesheuvel wrote:

On Mon, 25 Oct 2021 at 14:47, Chen Yu [off-list ref] wrote:

quoted

On Mon, Oct 25, 2021 at 02:02:24PM +0200, Greg Kroah-Hartman wrote:

quoted

On Mon, Oct 25, 2021 at 07:45:19PM +0800, Chen Yu wrote:

quoted

On Mon, Oct 25, 2021 at 08:44:00AM +0200, Greg Kroah-Hartman wrote:

quoted

On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote:

quoted

Platform Firmware Runtime Update image starts with UEFI headers, and the
headers are defined in UEFI specification, but some of them have not been
defined in the kernel yet.

For example, the header layout of a capsule file looks like this:

EFI_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER
EFI_FIRMWARE_IMAGE_AUTHENTICATION

These structures would be used by the Platform Firmware Runtime Update
driver to parse the format of capsule file to verify if the corresponding
version number is valid. The EFI_CAPSULE_HEADER has been defined in the
kernel, however the rest are not, thus introduce corresponding UEFI
structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and
so the corresponding data types should be packed.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v6: No change since v5.
v5: No change since v4.
v4: Revise the commit log to make it more clear. (Rafael J. Wysocki)
---
 include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6b5d36babfcc..19ff834e1388 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h

@@ -148,6 +148,56 @@ typedef struct {
        u32 imagesize;
 } efi_capsule_header_t;

+#pragma pack(1)

Why is this pragma suddenly needed now in this file?

If you really need this for a specific structure, use the "__packed"
attribute please.

These two structures are required to be packed in the uefi spec, I'll change
them to "__packed".

And they are the _only_ ones in this .h file that require this?  I would
think that they all require this.

I did a search in the uefi specification, and found 42 pack(1) structures,
while the other structures do not have pack(1) attribute.

It seems to me that whether the structures are required to be strictly packed
depends on the use case. Here's my understanding and I might be wrong: In this
patch, according to the skeleton of capsule file described in
[Figure 23-6 Firmware Management and Firmware Image Management headers]
in the uefi spec [1], the two structures are located at the beginning of
the capsule file, and followed by real payload. If these structure are packed
then the the adjacent binary payload could start on byte boundary without
padding, which might save space for capsule file.

Packing only affects internal padding, and a struct's size is never
padded to be a multiple of its alignment (which equals the largest
alignment of all its members). This of course assumes that you don't
abuse array indexing as a sizeof() operator.

However, the __packed attribute does indicate to the compiler that the
entire thing can appear misaligned in memory. So if one follows the
other in the capsule header, the __packed attribute may be appropriate
to ensure that the second one is not accessed using misaligned loads
and stores.

And then there is of course the ambiguity in alignment of uint64_t on
x86, which could be either 4 or 8 bytes depending on the context (and
UEFI targets all of them). So __packed may be used to disambiguate
between those if a uint64_t field appears on a boundary whose offset %
8 == 4.

So please use __packed rather than the pragma(), and apply it wherever
it is applied in the spec.

Thanks for the explaination in detail! Ard. Will do in next version.

Thanks,
Chenyu