| Message ID | 20170405092317.27921-9-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi: add support for non-standard capsule headers | expand |
On Wed, 05 Apr, at 10:23:17AM, Ard Biesheuvel wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > The firmware for Quark X102x prepends a security header to the capsule > which is needed to support the mandatory secure boot on this processor. > The header can be detected by checking for the "_CSH" signature and - > to avoid any GUID conflict - validating its size field to contain the > expected value. Then we need to look for the EFI header right after the > security header and pass the real header to __efi_capsule_setup_info. > > To be minimally invasive and maximally safe, the quirk version of > efi_capsule_identify_image is only effective on Quark processors. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Cc: Matt Fleming <matt@codeblueprint.co.uk> > [ardb: refactor using an override of efi_capsule_setup_info()] > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/x86/platform/efi/quirks.c | 112 ++++++++++++++++++++ > drivers/firmware/efi/Kconfig | 9 ++ > 2 files changed, 121 insertions(+) [...] > @@ -495,3 +549,61 @@ bool efi_poweroff_required(void) > { > return acpi_gbl_reduced_hardware || acpi_no_s5; > } > + > +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH > + > +static const struct x86_cpu_id quark_ids[] = { > + { X86_VENDOR_INTEL, 5, 9 }, /* Intel Quark X1000 */ > + { } > +}; > + > +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff, > + size_t hdr_bytes) > +{ > + struct quark_security_header *csh = kbuff; > + > + cap_info->total_size = 0; > + > + if (!x86_match_cpu(quark_ids)) > + goto fallback; > + I'd prefer to see the quark quirk pulled out into its own function and referenced from the __weak efi_capsule_setup_info() function, which makes it easier to people to read the EFI capsule code flow if they're not interested in the Quark quick. Something like this, int efi_capsule_setup_info(...) { ... if (x86_match_cpu(quark_ids)) return efi_capsule_quark_setup_quirk(cap_info, kbuff, hdr_bytes); > + if (hdr_bytes < sizeof(efi_capsule_header_t)) > + return 0; > + > + memcpy(&cap_info->header, kbuff, sizeof(cap_info->header)); > + > + cap_info->total_size += cap_info->header.imagesize; > + > + return __efi_capsule_setup_info(cap_info); > +} Or something. Otherwise this looks fine to me. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 18, 2017 at 3:48 PM, Matt Fleming <matt@codeblueprint.co.uk> wrote: > On Wed, 05 Apr, at 10:23:17AM, Ard Biesheuvel wrote: >> The firmware for Quark X102x prepends a security header to the capsule >> which is needed to support the mandatory secure boot on this processor. >> The header can be detected by checking for the "_CSH" signature and - >> to avoid any GUID conflict - validating its size field to contain the >> expected value. Then we need to look for the EFI header right after the >> security header and pass the real header to __efi_capsule_setup_info. >> >> To be minimally invasive and maximally safe, the quirk version of >> efi_capsule_identify_image is only effective on Quark processors. >> +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH >> + >> +static const struct x86_cpu_id quark_ids[] = { >> + { X86_VENDOR_INTEL, 5, 9 }, /* Intel Quark X1000 */ >> + { } >> +}; >> + >> +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff, >> + size_t hdr_bytes) >> +{ >> + struct quark_security_header *csh = kbuff; >> + >> + cap_info->total_size = 0; >> + >> + if (!x86_match_cpu(quark_ids)) >> + goto fallback; >> + > > I'd prefer to see the quark quirk pulled out into its own function and > referenced from the __weak efi_capsule_setup_info() function, which > makes it easier to people to read the EFI capsule code flow if they're > not interested in the Quark quick. > > Something like this, > > int efi_capsule_setup_info(...) > { > ... > > if (x86_match_cpu(quark_ids)) > return efi_capsule_quark_setup_quirk(cap_info, kbuff, hdr_bytes); Previously I had proposed to use driver_data field Something like struct _info { int(*setup)(...); }; <<< this is optional, might be useful in the future, otherwise just simple typedef. int efi_capsule_quark_setup_quirk(...) { } struct _info efi_capsule_quark_info = { .setup = ..._setup_quirk, }; #define ICPU(family, model, ddata) ... static const struct x86_cpu_id quark_ids[] = { ICPU(5, 9, efi_capsule_quark_info), {} }; Though didn't hear back on it. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-04-18 14:55, Andy Shevchenko wrote: > On Tue, Apr 18, 2017 at 3:48 PM, Matt Fleming <matt@codeblueprint.co.uk> wrote: >> On Wed, 05 Apr, at 10:23:17AM, Ard Biesheuvel wrote: > >>> The firmware for Quark X102x prepends a security header to the capsule >>> which is needed to support the mandatory secure boot on this processor. >>> The header can be detected by checking for the "_CSH" signature and - >>> to avoid any GUID conflict - validating its size field to contain the >>> expected value. Then we need to look for the EFI header right after the >>> security header and pass the real header to __efi_capsule_setup_info. >>> >>> To be minimally invasive and maximally safe, the quirk version of >>> efi_capsule_identify_image is only effective on Quark processors. > >>> +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH >>> + >>> +static const struct x86_cpu_id quark_ids[] = { >>> + { X86_VENDOR_INTEL, 5, 9 }, /* Intel Quark X1000 */ >>> + { } >>> +}; >>> + >>> +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff, >>> + size_t hdr_bytes) >>> +{ >>> + struct quark_security_header *csh = kbuff; >>> + >>> + cap_info->total_size = 0; >>> + >>> + if (!x86_match_cpu(quark_ids)) >>> + goto fallback; >>> + >> >> I'd prefer to see the quark quirk pulled out into its own function and >> referenced from the __weak efi_capsule_setup_info() function, which >> makes it easier to people to read the EFI capsule code flow if they're >> not interested in the Quark quick. >> >> Something like this, >> >> int efi_capsule_setup_info(...) >> { >> ... >> >> if (x86_match_cpu(quark_ids)) >> return efi_capsule_quark_setup_quirk(cap_info, kbuff, hdr_bytes); > > Previously I had proposed to use driver_data field > > Something like > > struct _info { > int(*setup)(...); > }; <<< this is optional, might be useful in the future, otherwise just > simple typedef. > > int efi_capsule_quark_setup_quirk(...) > { > } > > struct _info efi_capsule_quark_info = { > .setup = ..._setup_quirk, > }; > > #define ICPU(family, model, ddata) ... > > static const struct x86_cpu_id quark_ids[] = { > ICPU(5, 9, efi_capsule_quark_info), > {} > }; > > Though didn't hear back on it. > I've implemented this, but for the old design, and Ard took over then. So it never made it to the list. Whatever layout of these bits is preferred, it can probably be done. I just need an indication that there is (likely) a consensus. Thanks, Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote: > > I've implemented this, but for the old design, and Ard took over then. > So it never made it to the list. > > Whatever layout of these bits is preferred, it can probably be done. I > just need an indication that there is (likely) a consensus. Post what you already have on top of Ard's series and we'll review it. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 April 2017 at 14:44, Matt Fleming <matt@codeblueprint.co.uk> wrote: > On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote: >> >> I've implemented this, but for the old design, and Ard took over then. >> So it never made it to the list. >> >> Whatever layout of these bits is preferred, it can probably be done. I >> just need an indication that there is (likely) a consensus. > > Post what you already have on top of Ard's series and we'll review it. For the record, other than the change Matt suggested (to take the Quirk handling out of the normal flow), I don't think there is a need to add a lot of parametrization just to implement this single quirk. IMO it can wait for the next one. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 April 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 April 2017 at 14:44, Matt Fleming <matt@codeblueprint.co.uk> wrote: >> On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote: >>> >>> I've implemented this, but for the old design, and Ard took over then. >>> So it never made it to the list. >>> >>> Whatever layout of these bits is preferred, it can probably be done. I >>> just need an indication that there is (likely) a consensus. >> >> Post what you already have on top of Ard's series and we'll review it. > > For the record, other than the change Matt suggested (to take the > Quirk handling out of the normal flow), I don't think there is a need > to add a lot of parametrization just to implement this single quirk. > IMO it can wait for the next one. OK, I have pushed patches 1 - 5 to efi/next branch, and the entire series (with Matt's review feedback regarding #6 and #7 incorporated) to my own branch here https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule Jan, would you mind taking over again, i.e, update patch #8 and repost patches #6, #7 and #8 after retesting? (just remove my S-o-b from #8 after you've reworked it) Thanks, Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-04-18 19:10, Ard Biesheuvel wrote: > On 18 April 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 18 April 2017 at 14:44, Matt Fleming <matt@codeblueprint.co.uk> wrote: >>> On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote: >>>> >>>> I've implemented this, but for the old design, and Ard took over then. >>>> So it never made it to the list. >>>> >>>> Whatever layout of these bits is preferred, it can probably be done. I >>>> just need an indication that there is (likely) a consensus. >>> >>> Post what you already have on top of Ard's series and we'll review it. >> >> For the record, other than the change Matt suggested (to take the >> Quirk handling out of the normal flow), I don't think there is a need >> to add a lot of parametrization just to implement this single quirk. >> IMO it can wait for the next one. > > OK, I have pushed patches 1 - 5 to efi/next branch, and the entire > series (with Matt's review feedback regarding #6 and #7 incorporated) > to my own branch here > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule > > Jan, would you mind taking over again, i.e, update patch #8 and repost > patches #6, #7 and #8 after retesting? (just remove my S-o-b from #8 > after you've reworked it) Sure, I'll pick them up ASAP. Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c index 30031d5293c4..ee922e1b7008 100644 --- a/arch/x86/platform/efi/quirks.c +++ b/arch/x86/platform/efi/quirks.c @@ -13,12 +13,66 @@ #include <linux/dmi.h> #include <asm/efi.h> #include <asm/uv/uv.h> +#include <asm/cpu_device_id.h> #define EFI_MIN_RESERVE 5120 #define EFI_DUMMY_GUID \ EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9) +#define QUARK_CSH_SIGNATURE 0x5f435348 /* _CSH */ +#define QUARK_SECURITY_HEADER_SIZE 0x400 + +/* + * Header prepended to the standard EFI capsule on Quark systems the are based + * on Intel firmware BSP. + * @csh_signature: Unique identifier to sanity check signed module + * presence ("_CSH"). + * @version: Current version of CSH used. Should be one for Quark A0. + * @modulesize: Size of the entire module including the module header + * and payload. + * @security_version_number_index: Index of SVN to use for validation of signed + * module. + * @security_version_number: Used to prevent against roll back of modules. + * @rsvd_module_id: Currently unused for Clanton (Quark). + * @rsvd_module_vendor: Vendor Identifier. For Intel products value is + * 0x00008086. + * @rsvd_date: BCD representation of build date as yyyymmdd, where + * yyyy=4 digit year, mm=1-12, dd=1-31. + * @headersize: Total length of the header including including any + * padding optionally added by the signing tool. + * @hash_algo: What Hash is used in the module signing. + * @cryp_algo: What Crypto is used in the module signing. + * @keysize: Total length of the key data including including any + * padding optionally added by the signing tool. + * @signaturesize: Total length of the signature including including any + * padding optionally added by the signing tool. + * @rsvd_next_header: 32-bit pointer to the next Secure Boot Module in the + * chain, if there is a next header. + * @rsvd: Reserved, padding structure to required size. + * + * See also QuartSecurityHeader_t in + * Quark_EDKII_v1.2.1.1/QuarkPlatformPkg/Include/QuarkBootRom.h + * from https://downloadcenter.intel.com/download/23197/Intel-Quark-SoC-X1000-Board-Support-Package-BSP + */ +struct quark_security_header { + u32 csh_signature; + u32 version; + u32 modulesize; + u32 security_version_number_index; + u32 security_version_number; + u32 rsvd_module_id; + u32 rsvd_module_vendor; + u32 rsvd_date; + u32 headersize; + u32 hash_algo; + u32 cryp_algo; + u32 keysize; + u32 signaturesize; + u32 rsvd_next_header; + u32 rsvd[2]; +}; + static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 }; static bool efi_no_storage_paranoia; @@ -495,3 +549,61 @@ bool efi_poweroff_required(void) { return acpi_gbl_reduced_hardware || acpi_no_s5; } + +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH + +static const struct x86_cpu_id quark_ids[] = { + { X86_VENDOR_INTEL, 5, 9 }, /* Intel Quark X1000 */ + { } +}; + +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff, + size_t hdr_bytes) +{ + struct quark_security_header *csh = kbuff; + + cap_info->total_size = 0; + + if (!x86_match_cpu(quark_ids)) + goto fallback; + + /* Only process data block that is larger than the security header */ + if (hdr_bytes < sizeof(struct quark_security_header)) + return 0; + + if (csh->csh_signature != QUARK_CSH_SIGNATURE || + csh->headersize != QUARK_SECURITY_HEADER_SIZE) + goto fallback; + + /* Only process data block if EFI header is included */ + if (hdr_bytes < QUARK_SECURITY_HEADER_SIZE + + sizeof(efi_capsule_header_t)) + return 0; + + pr_debug("Quark security header detected\n"); + + if (csh->rsvd_next_header != 0) { + pr_err("multiple Quark security headers not supported\n"); + return -EINVAL; + } + + kbuff += csh->headersize; + cap_info->total_size = csh->headersize; + + /* + * Update the first page pointer to skip over the CSH header. + */ + cap_info->pages[0] += csh->headersize; + +fallback: + if (hdr_bytes < sizeof(efi_capsule_header_t)) + return 0; + + memcpy(&cap_info->header, kbuff, sizeof(cap_info->header)); + + cap_info->total_size += cap_info->header.imagesize; + + return __efi_capsule_setup_info(cap_info); +} + +#endif diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index 2e78b0b96d74..394db40ed374 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -112,6 +112,15 @@ config EFI_CAPSULE_LOADER Most users should say N. +config EFI_CAPSULE_QUIRK_QUARK_CSH + boolean "Add support for Quark capsules with non-standard headers" + depends on X86 && !64BIT + select EFI_CAPSULE_LOADER + default y + help + Add support for processing Quark X1000 EFI capsules, whose header + layout deviates from the layout mandated by the UEFI specification. + config EFI_TEST tristate "EFI Runtime Service Tests Support" depends on EFI