| Message ID | 68d1c452bbf7f742793cb39ebb66f6b4ba6a3fb3.1635953446.git.yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce Platform Firmware Runtime Update and Telemetry drivers | expand |
On Wed, Nov 3, 2021 at 4:44 PM Chen Yu <yu.c.chen@intel.com> wrote: > > 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. Why does the driver need to do that? The firmware will reject the update if the version is invalid anyway, won't it? > The EFI_CAPSULE_HEADER has been defined in the > kernel, however the rest are not, thus introduce corresponding UEFI > structures accordingly. I would change the above in the following way: "EFI_CAPSULE_HEADER has been defined in the kernel, but the other structures have not been defined yet, so do that." > Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed > in the uefi specification. > Ard has pointed out that, 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. "For this reason, use the __packed attribute to indicate to the compiler that the entire structure can appear misaligned in memory (as suggested by Ard) in case one of them follows the other directly in a capsule header." > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > v8: Use efi_guid_t instead of guid_t. (Andy Shevchenko) > v7: Use __packed instead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve) > 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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 6b5d36babfcc..1ec73c5ab6c9 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -148,6 +148,52 @@ typedef struct { > u32 imagesize; > } efi_capsule_header_t; > > +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ > +struct efi_manage_capsule_header { > + u32 ver; > + u16 emb_drv_cnt; > + u16 payload_cnt; > + /* > + * Variable array indicated by number of > + * (emb_drv_cnt + payload_cnt) * Variable-size array of the size given by the sum of * emb_drv_cnt and payload_cnt. > + */ > + u64 offset_list[]; > +} __packed; > + > +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ > +struct efi_manage_capsule_image_header { > + u32 ver; > + efi_guid_t image_type_id; > + u8 image_index; > + u8 reserved_bytes[3]; > + u32 image_size; > + u32 vendor_code_size; > + /* ver = 2. */ What does this mean? > + u64 hw_ins; > + /* ver = v3. */ And same here? > + u64 capsule_support; > +} __packed; > + > +/* WIN_CERTIFICATE */ > +struct win_cert { > + u32 len; > + u16 rev; > + u16 cert_type; > +}; > + > +/* WIN_CERTIFICATE_UEFI_GUID */ > +struct win_cert_uefi_guid { > + struct win_cert hdr; > + efi_guid_t cert_type; > + u8 cert_data[]; > +}; > + > +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */ The "N" character at the end is missing. > +struct efi_image_auth { > + u64 mon_count; > + struct win_cert_uefi_guid auth_info; > +}; > + > /* > * EFI capsule flags > */ > --
Hi Rafael, On Thu, Nov 18, 2021 at 04:49:35PM +0100, Rafael J. Wysocki wrote: > On Wed, Nov 3, 2021 at 4:44 PM Chen Yu <yu.c.chen@intel.com> wrote: > > > > 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. > > Why does the driver need to do that? > > The firmware will reject the update if the version is invalid anyway, won't it? > Yes, the firmware will reject the update if the version does not match. The motivation of checking it in kernel before the firmware is mainly to deal with a corner case that, if the user provides an invalid capsule image, the kernel could be used as a guard to reject it, without switching to the MM update mode(which might be costly). > > The EFI_CAPSULE_HEADER has been defined in the > > kernel, however the rest are not, thus introduce corresponding UEFI > > structures accordingly. > > I would change the above in the following way: > > "EFI_CAPSULE_HEADER has been defined in the kernel, but the other > structures have not been defined yet, so do that." > Ok, will do. > > Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed > > in the uefi specification. > > > Ard has pointed out that, 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. > > "For this reason, use the __packed attribute to indicate to the > compiler that the entire structure can appear misaligned in memory (as > suggested by Ard) in case one of them follows the other directly in a > capsule header." > Ok, will do. > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > v8: Use efi_guid_t instead of guid_t. (Andy Shevchenko) > > v7: Use __packed instead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve) > > 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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index 6b5d36babfcc..1ec73c5ab6c9 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -148,6 +148,52 @@ typedef struct { > > u32 imagesize; > > } efi_capsule_header_t; > > > > +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ > > +struct efi_manage_capsule_header { > > + u32 ver; > > + u16 emb_drv_cnt; > > + u16 payload_cnt; > > + /* > > + * Variable array indicated by number of > > + * (emb_drv_cnt + payload_cnt) > > * Variable-size array of the size given by the sum of > * emb_drv_cnt and payload_cnt. > Ok, will change it. > > + */ > > + u64 offset_list[]; > > +} __packed; > > + > > +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ > > +struct efi_manage_capsule_image_header { > > + u32 ver; > > + efi_guid_t image_type_id; > > + u8 image_index; > > + u8 reserved_bytes[3]; > > + u32 image_size; > > + u32 vendor_code_size; > > + /* ver = 2. */ > > What does this mean? > > > + u64 hw_ins; > > + /* ver = v3. */ > > And same here? > The hw_ins was introduced in version 2, and capsule_support was introduced in version 3 of the capsule image format. I'll revise the comment in next version. > > + u64 capsule_support; > > +} __packed; > > + > > +/* WIN_CERTIFICATE */ > > +struct win_cert { > > + u32 len; > > + u16 rev; > > + u16 cert_type; > > +}; > > + > > +/* WIN_CERTIFICATE_UEFI_GUID */ > > +struct win_cert_uefi_guid { > > + struct win_cert hdr; > > + efi_guid_t cert_type; > > + u8 cert_data[]; > > +}; > > + > > +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */ > > The "N" character at the end is missing. > Ok, will fix it. thanks, Chenyu
On Thu, Nov 18, 2021 at 5:21 PM Chen Yu <yu.c.chen@intel.com> wrote: > > Hi Rafael, > On Thu, Nov 18, 2021 at 04:49:35PM +0100, Rafael J. Wysocki wrote: > > On Wed, Nov 3, 2021 at 4:44 PM Chen Yu <yu.c.chen@intel.com> wrote: > > > > > > 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. > > > > Why does the driver need to do that? > > > > The firmware will reject the update if the version is invalid anyway, won't it? > > > Yes, the firmware will reject the update if the version does not match. The motivation > of checking it in kernel before the firmware is mainly to deal with a corner case that, > if the user provides an invalid capsule image, the kernel could be used as a guard to > reject it, without switching to the MM update mode(which might be costly). OK, but it would be good to mention this somewhere, preferably in he changelog and maybe also in a comment next to the related code. > > > The EFI_CAPSULE_HEADER has been defined in the > > > kernel, however the rest are not, thus introduce corresponding UEFI > > > structures accordingly. > > > > I would change the above in the following way: > > > > "EFI_CAPSULE_HEADER has been defined in the kernel, but the other > > structures have not been defined yet, so do that." > > > Ok, will do. > > > Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed > > > in the uefi specification. > > > > > Ard has pointed out that, 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. > > > > "For this reason, use the __packed attribute to indicate to the > > compiler that the entire structure can appear misaligned in memory (as > > suggested by Ard) in case one of them follows the other directly in a > > capsule header." > > > Ok, will do. > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > > --- > > > v8: Use efi_guid_t instead of guid_t. (Andy Shevchenko) > > > v7: Use __packed instead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve) > > > 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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 46 insertions(+) > > > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > index 6b5d36babfcc..1ec73c5ab6c9 100644 > > > --- a/include/linux/efi.h > > > +++ b/include/linux/efi.h > > > @@ -148,6 +148,52 @@ typedef struct { > > > u32 imagesize; > > > } efi_capsule_header_t; > > > > > > +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ > > > +struct efi_manage_capsule_header { > > > + u32 ver; > > > + u16 emb_drv_cnt; > > > + u16 payload_cnt; > > > + /* > > > + * Variable array indicated by number of > > > + * (emb_drv_cnt + payload_cnt) > > > > * Variable-size array of the size given by the sum of > > * emb_drv_cnt and payload_cnt. > > > Ok, will change it. > > > + */ > > > + u64 offset_list[]; > > > +} __packed; > > > + > > > +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ > > > +struct efi_manage_capsule_image_header { > > > + u32 ver; > > > + efi_guid_t image_type_id; > > > + u8 image_index; > > > + u8 reserved_bytes[3]; > > > + u32 image_size; > > > + u32 vendor_code_size; > > > + /* ver = 2. */ > > > > What does this mean? > > > > > + u64 hw_ins; > > > + /* ver = v3. */ > > > > And same here? > > > The hw_ins was introduced in version 2, and capsule_support > was introduced in version 3 of the capsule image format. > I'll revise the comment in next version. Please do.
diff --git a/include/linux/efi.h b/include/linux/efi.h index 6b5d36babfcc..1ec73c5ab6c9 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -148,6 +148,52 @@ typedef struct { u32 imagesize; } efi_capsule_header_t; +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ +struct efi_manage_capsule_header { + u32 ver; + u16 emb_drv_cnt; + u16 payload_cnt; + /* + * Variable array indicated by number of + * (emb_drv_cnt + payload_cnt) + */ + u64 offset_list[]; +} __packed; + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ +struct efi_manage_capsule_image_header { + u32 ver; + efi_guid_t image_type_id; + u8 image_index; + u8 reserved_bytes[3]; + u32 image_size; + u32 vendor_code_size; + /* ver = 2. */ + u64 hw_ins; + /* ver = v3. */ + u64 capsule_support; +} __packed; + +/* WIN_CERTIFICATE */ +struct win_cert { + u32 len; + u16 rev; + u16 cert_type; +}; + +/* WIN_CERTIFICATE_UEFI_GUID */ +struct win_cert_uefi_guid { + struct win_cert hdr; + efi_guid_t cert_type; + u8 cert_data[]; +}; + +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */ +struct efi_image_auth { + u64 mon_count; + struct win_cert_uefi_guid auth_info; +}; + /* * EFI capsule flags */
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 are required to be packed in the uefi specification. Ard has pointed out that, 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. Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- v8: Use efi_guid_t instead of guid_t. (Andy Shevchenko) v7: Use __packed instead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve) 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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)