[PATCH v5 00/11] efi_loader: capsule: improve capsule authentication support
As I proposed and discussed in [1] and [2], I have made a couple of improvements on the current implementation of capsule update in this patch set.
* add signing feature to mkeficapsule * add "--guid" option to mkeficapsule * add man page of mkeficapsule * update uefi document regarding capsule update * revise pytests * (as RFC) add CONFIG_EFI_CAPSULE_KEY_PATH
# We have had some discussion about fdtsig.sh. # So RFCs (patch#10,#11) are still included for further discussion # if they are useful or not. # For smooth merge, the rest (patch#1-9) should work without them.
[1] https://lists.denx.de/pipermail/u-boot/2021-April/447918.html [2] https://lists.denx.de/pipermail/u-boot/2021-July/455292.html
Prerequisite patches ==================== None
Test ==== * locally passed the pytest which is included in this patch series on sandbox built. (CONFIG_EFI_CAPSULE_AUTHENTICATE should explicitly be turned on in order to exercise the authentication code.)
Changes ======= v5 (Oct 27, 2021) * rebased on pre-v2022.01-rc1 (WIP/26Oct2021) * drop already-merged patches * drop __weak from efi_get_public_key_data() (patch#1) * describe the format of public key node in device tree (patch#4) * re-order patches by grouping closely-related patches (patch#6-8) * modify pytest to make the test results correctly verified either with or without CONFIG_EFI_CAPSULE_AUTHENTICATE (patch#9) * add RFCs for embedding public keys during the build process (patch#10,11)
v4 (Oct 7, 2021) * rebased on v2021.10 * align with "Revert "efi_capsule: Move signature from DTB to .rodata"" * add more missing *revert* commits (patch#1,#2,#3) * add fdtsig.sh, replacing dtb support in mkeficapsule (patch#4) * update/revise the man/uefi doc (patch#6,#7) * fix a bug in parsing guid string (patch#8) * add a test for "--guid" option (patch#10) * use dtb-based authentication test as done in v1 (patch#11)
v3 (Aug 31, 2021) * rebased on v2021.10-rc3 * remove pytest-related patches * add function descriptions in mkeficapsule.c * correct format specifiers in printf() * let main() return 0 or -1 only * update doc/develop/uefi/uefi.rst for syntax change of mkeficapsule
v2 (July 28, 2021) * rebased on v2021.10-rc* * removed dependency on target's configuration * removed fdtsig.sh and others * add man page * update the UEFI document * add dedicate defconfig for testing on sandbox * add gitlab CI support * add "--guid" option to mkeficapsule (yet rather RFC)
Initial release (May 12, 2021) * based on v2021.07-rc2
AKASHI Takahiro (11): efi_loader: capsule: drop __weak from efi_get_public_key_data() tools: mkeficapsule: add firmwware image signing tools: mkeficapsule: add man page doc: update UEFI document for usage of mkeficapsule test/py: efi_capsule: add image authentication test tools: mkeficapsule: allow for specifying GUID explicitly test/py: efi_capsule: align with the syntax change of mkeficapsule test/py: efi_capsule: add a test for "--guid" option test/py: efi_capsule: check the results in case of CAPSULE_AUTHENTICATE (RFC) tools: add fdtsig.sh (RFC) efi_loader, dts: add public keys for capsules to device tree
MAINTAINERS | 2 + doc/develop/uefi/uefi.rst | 143 +++-- doc/mkeficapsule.1 | 107 ++++ dts/Makefile | 23 +- lib/efi_loader/Kconfig | 7 + lib/efi_loader/efi_capsule.c | 2 +- .../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 42 +- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_efi_capsule/test_capsule_firmware.py | 91 +++- .../test_capsule_firmware_signed.py | 233 ++++++++ tools/Kconfig | 8 + tools/Makefile | 8 +- tools/fdtsig.sh | 40 ++ tools/mkeficapsule.c | 503 ++++++++++++++++-- 15 files changed, 1092 insertions(+), 132 deletions(-) create mode 100644 doc/mkeficapsule.1 create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py create mode 100755 tools/fdtsig.sh
As we discussed in ML, currently a device tree is the only place to store public keys for capsule authentication. So __weak is not necessary for now.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- lib/efi_loader/efi_capsule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 44f5da61a9be..850937fd120f 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -256,7 +256,7 @@ out: }
#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) -int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) { const void *fdt_blob = gd->fdt_blob; const void *blob;
On Thu, 28 Oct 2021 at 00:24, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
As we discussed in ML, currently a device tree is the only place to store public keys for capsule authentication. So __weak is not necessary for now.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_capsule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
With this enhancement, mkeficapsule will be able to sign a capsule file when it is created. A signature added will be used later in the verification at FMP's SetImage() call.
To do that, We need specify additional command parameters: -monotonic-cout <count> : monotonic count -private-key <private key file> : private key file -certificate <certificate file> : certificate file Only when all of those parameters are given, a signature will be added to a capsule file.
Users are expected to maintain and increment the monotonic count at every time of the update for each firmware image.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- tools/Kconfig | 8 + tools/Makefile | 8 +- tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 417 insertions(+), 34 deletions(-)
diff --git a/tools/Kconfig b/tools/Kconfig index 91ce8ae3e516..117c921da3fe 100644 --- a/tools/Kconfig +++ b/tools/Kconfig @@ -90,4 +90,12 @@ config TOOLS_SHA512 help Enable SHA512 support in the tools builds
+config TOOLS_MKEFICAPSULE + bool "Build efimkcapsule command" + default y if EFI_CAPSULE_ON_DISK + help + This command allows users to create a UEFI capsule file and, + optionally sign that file. If you want to enable UEFI capsule + update feature on your target, you certainly need this. + endmenu diff --git a/tools/Makefile b/tools/Makefile index b45219e2c30c..5a73cc4b363d 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -238,8 +238,12 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs hostprogs-$(CONFIG_ASN1_COMPILER) += asn1_compiler HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
-mkeficapsule-objs := mkeficapsule.o $(LIBFDT_OBJS) -hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule +HOSTLDLIBS_mkeficapsule += -luuid +ifeq ($(CONFIG_TOOLS_LIBCRYPTO),y) +HOSTLDLIBS_mkeficapsule += \ + $(shell pkg-config --libs libssl libcrypto 2> /dev/null || echo "-lssl -lcrypto") +endif +hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
# We build some files with extra pedantic flags to try to minimize things # that won't build on some weird host compiler -- though there are lots of diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 4995ba4e0c2a..5541e4bda894 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -15,6 +15,16 @@ #include <sys/stat.h> #include <sys/types.h>
+#include <linux/kconfig.h> +#ifdef CONFIG_TOOLS_LIBCRYPTO +#include <openssl/asn1.h> +#include <openssl/bio.h> +#include <openssl/evp.h> +#include <openssl/err.h> +#include <openssl/pem.h> +#include <openssl/pkcs7.h> +#endif + typedef __u8 u8; typedef __u16 u16; typedef __u32 u32; @@ -38,12 +48,25 @@ efi_guid_t efi_guid_image_type_uboot_fit = EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID; efi_guid_t efi_guid_image_type_uboot_raw = EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID; +efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; + +#ifdef CONFIG_TOOLS_LIBCRYPTO +static const char *opts_short = "f:r:i:I:v:p:c:m:dh"; +#else +static const char *opts_short = "f:r:i:I:v:h"; +#endif
static struct option options[] = { {"fit", required_argument, NULL, 'f'}, {"raw", required_argument, NULL, 'r'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'}, +#ifdef CONFIG_TOOLS_LIBCRYPTO + {"private-key", required_argument, NULL, 'p'}, + {"certificate", required_argument, NULL, 'c'}, + {"monotonic-count", required_argument, NULL, 'm'}, + {"dump-sig", no_argument, NULL, 'd'}, +#endif {"help", no_argument, NULL, 'h'}, {NULL, 0, NULL, 0}, }; @@ -57,16 +80,280 @@ static void print_usage(void) "\t-r, --raw <raw image> new raw image file\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n" +#ifdef CONFIG_TOOLS_LIBCRYPTO + "\t-p, --private-key <privkey file> private key file\n" + "\t-c, --certificate <cert file> signer's certificate file\n" + "\t-m, --monotonic-count <count> monotonic count\n" + "\t-d, --dump_sig dump signature (*.p7)\n" +#endif "\t-h, --help print a help message\n", tool_name); }
+/** + * auth_context - authentication context + * @key_file: Path to a private key file + * @cert_file: Path to a certificate file + * @image_data: Pointer to firmware data + * @image_size: Size of firmware data + * @auth: Authentication header + * @sig_data: Signature data + * @sig_size: Size of signature data + * + * Data structure used in create_auth_data(). @key_file through + * @image_size are input parameters. @auth, @sig_data and @sig_size + * are filled in by create_auth_data(). + */ +struct auth_context { + char *key_file; + char *cert_file; + u8 *image_data; + size_t image_size; + struct efi_firmware_image_authentication auth; + u8 *sig_data; + size_t sig_size; +}; + +static int dump_sig; + +#ifdef CONFIG_TOOLS_LIBCRYPTO +/** + * fileio-read_pkey - read out a private key + * @filename: Path to a private key file + * + * Read out a private key file and parse it into "EVP_PKEY" structure. + * + * Return: + * * Pointer to private key structure - on success + * * NULL - on failure + */ +static EVP_PKEY *fileio_read_pkey(const char *filename) +{ + EVP_PKEY *key = NULL; + BIO *bio; + + bio = BIO_new_file(filename, "r"); + if (!bio) + goto out; + + key = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL); + +out: + BIO_free_all(bio); + if (!key) { + printf("Can't load key from file '%s'\n", filename); + ERR_print_errors_fp(stderr); + } + + return key; +} + +/** + * fileio-read_cert - read out a certificate + * @filename: Path to a certificate file + * + * Read out a certificate file and parse it into "X509" structure. + * + * Return: + * * Pointer to certificate structure - on success + * * NULL - on failure + */ +static X509 *fileio_read_cert(const char *filename) +{ + X509 *cert = NULL; + BIO *bio; + + bio = BIO_new_file(filename, "r"); + if (!bio) + goto out; + + cert = PEM_read_bio_X509(bio, NULL, NULL, NULL); + +out: + BIO_free_all(bio); + if (!cert) { + printf("Can't load certificate from file '%s'\n", filename); + ERR_print_errors_fp(stderr); + } + + return cert; +} + +/** + * create_auth_data - compose authentication data in capsule + * @auth_context: Pointer to authentication context + * + * Fill up an authentication header (.auth) and signature data (.sig_data) + * in @auth_context, using library functions from openssl. + * All the parameters in @auth_context must be filled in by a caller. + * + * Return: + * * 0 - on success + * * -1 - on failure + */ +static int create_auth_data(struct auth_context *ctx) +{ + EVP_PKEY *key = NULL; + X509 *cert = NULL; + BIO *data_bio = NULL; + const EVP_MD *md; + PKCS7 *p7; + int flags, ret = -1; + + OpenSSL_add_all_digests(); + OpenSSL_add_all_ciphers(); + ERR_load_crypto_strings(); + + key = fileio_read_pkey(ctx->key_file); + if (!key) + goto err; + cert = fileio_read_cert(ctx->cert_file); + if (!cert) + goto err; + + /* + * create a BIO, containing: + * * firmware image + * * monotonic count + * in this order! + * See EDK2's FmpAuthenticatedHandlerRsa2048Sha256() + */ + data_bio = BIO_new(BIO_s_mem()); + BIO_write(data_bio, ctx->image_data, ctx->image_size); + BIO_write(data_bio, &ctx->auth.monotonic_count, + sizeof(ctx->auth.monotonic_count)); + + md = EVP_get_digestbyname("SHA256"); + if (!md) + goto err; + + /* create signature */ + /* TODO: maybe add PKCS7_NOATTR and PKCS7_NOSMIMECAP */ + flags = PKCS7_BINARY | PKCS7_DETACHED; + p7 = PKCS7_sign(NULL, NULL, NULL, data_bio, flags | PKCS7_PARTIAL); + if (!p7) + goto err; + if (!PKCS7_sign_add_signer(p7, cert, key, md, flags)) + goto err; + if (!PKCS7_final(p7, data_bio, flags)) + goto err; + + /* convert pkcs7 into DER */ + ctx->sig_data = NULL; + ctx->sig_size = ASN1_item_i2d((ASN1_VALUE *)p7, &ctx->sig_data, + ASN1_ITEM_rptr(PKCS7)); + if (!ctx->sig_size) + goto err; + + /* fill auth_info */ + ctx->auth.auth_info.hdr.dwLength = sizeof(ctx->auth.auth_info) + + ctx->sig_size; + ctx->auth.auth_info.hdr.wRevision = WIN_CERT_REVISION_2_0; + ctx->auth.auth_info.hdr.wCertificateType = WIN_CERT_TYPE_EFI_GUID; + memcpy(&ctx->auth.auth_info.cert_type, &efi_guid_cert_type_pkcs7, + sizeof(efi_guid_cert_type_pkcs7)); + + ret = 0; +err: + BIO_free_all(data_bio); + EVP_PKEY_free(key); + X509_free(cert); + + return ret; +} + +/** + * dump_signature - dump out a signature + * @path: Path to a capsule file + * @signature: Signature data + * @sig_size: Size of signature data + * + * Signature data pointed to by @signature will be saved into + * a file whose file name is @path with ".p7" suffix. + * + * Return: + * * 0 - on success + * * -1 - on failure + */ +static int dump_signature(const char *path, u8 *signature, size_t sig_size) +{ + char *sig_path; + FILE *f; + size_t size; + int ret = -1; + + sig_path = malloc(strlen(path) + 3 + 1); + if (!sig_path) + return ret; + + sprintf(sig_path, "%s.p7", path); + f = fopen(sig_path, "w"); + if (!f) + goto err; + + size = fwrite(signature, 1, sig_size, f); + if (size == sig_size) + ret = 0; + + fclose(f); +err: + free(sig_path); + return ret; +} + +/** + * free_sig_data - free out signature data + * @ctx: Pointer to authentication context + * + * Free signature data allocated in create_auth_data(). + */ +static void free_sig_data(struct auth_context *ctx) +{ + if (ctx->sig_size) + OPENSSL_free(ctx->sig_data); +} +#else +static int create_auth_data(struct auth_context *ctx) +{ + return 0; +} + +static int dump_signature(const char *path, u8 *signature, size_t sig_size) +{ + return 0; +} + +static void free_sig_data(struct auth_context *ctx) {} +#endif + +/** + * create_fwbin - create an uefi capsule file + * @path: Path to a created capsule file + * @bin: Path to a firmware binary to encapsulate + * @guid: GUID of related FMP driver + * @index: Index number in capsule + * @instance: Instance number in capsule + * @mcount: Monotonic count in authentication information + * @private_file: Path to a private key file + * @cert_file: Path to a certificate file + * + * This function actually does the job of creating an uefi capsule file. + * All the arguments must be supplied. + * If either @private_file ror @cert_file is NULL, the capsule file + * won't be signed. + * + * Return: + * * 0 - on success + * * -1 - on failure + */ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, - unsigned long index, unsigned long instance) + unsigned long index, unsigned long instance, + uint64_t mcount, char *privkey_file, char *cert_file) { struct efi_capsule_header header; struct efi_firmware_management_capsule_header capsule; struct efi_firmware_management_capsule_image_header image; + struct auth_context auth_context; FILE *f, *g; struct stat bin_stat; u8 *data; @@ -76,8 +363,9 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, #ifdef DEBUG printf("For output: %s\n", path); printf("\tbin: %s\n\ttype: %pUl\n", bin, guid); - printf("\tindex: %ld\n\tinstance: %ld\n", index, instance); + printf("\tindex: %lu\n\tinstance: %lu\n", index, instance); #endif + auth_context.sig_size = 0;
g = fopen(bin, "r"); if (!g) { @@ -93,11 +381,34 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, printf("cannot allocate memory: %zx\n", (size_t)bin_stat.st_size); goto err_1; } - f = fopen(path, "w"); - if (!f) { - printf("cannot open %s\n", path); + + size = fread(data, 1, bin_stat.st_size, g); + if (size < bin_stat.st_size) { + printf("read failed (%zx)\n", size); goto err_2; } + + /* first, calculate signature to determine its size */ + if (privkey_file && cert_file) { + auth_context.key_file = privkey_file; + auth_context.cert_file = cert_file; + auth_context.auth.monotonic_count = mcount; + auth_context.image_data = data; + auth_context.image_size = bin_stat.st_size; + + if (create_auth_data(&auth_context)) { + printf("Signing firmware image failed\n"); + goto err_3; + } + + if (dump_sig && + dump_signature(path, auth_context.sig_data, + auth_context.sig_size)) { + printf("Creating signature file failed\n"); + goto err_3; + } + } + header.capsule_guid = efi_guid_fm_capsule; header.header_size = sizeof(header); /* TODO: The current implementation ignores flags */ @@ -106,11 +417,20 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, + sizeof(capsule) + sizeof(u64) + sizeof(image) + bin_stat.st_size; + if (auth_context.sig_size) + header.capsule_image_size += sizeof(auth_context.auth) + + auth_context.sig_size; + + f = fopen(path, "w"); + if (!f) { + printf("cannot open %s\n", path); + goto err_3; + }
size = fwrite(&header, 1, sizeof(header), f); if (size < sizeof(header)) { printf("write failed (%zx)\n", size); - goto err_3; + goto err_4; }
capsule.version = 0x00000001; @@ -119,13 +439,13 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, size = fwrite(&capsule, 1, sizeof(capsule), f); if (size < (sizeof(capsule))) { printf("write failed (%zx)\n", size); - goto err_3; + goto err_4; } offset = sizeof(capsule) + sizeof(u64); size = fwrite(&offset, 1, sizeof(offset), f); if (size < sizeof(offset)) { printf("write failed (%zx)\n", size); - goto err_3; + goto err_4; }
image.version = 0x00000003; @@ -135,34 +455,53 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, image.reserved[1] = 0; image.reserved[2] = 0; image.update_image_size = bin_stat.st_size; + if (auth_context.sig_size) + image.update_image_size += sizeof(auth_context.auth) + + auth_context.sig_size; image.update_vendor_code_size = 0; /* none */ image.update_hardware_instance = instance; image.image_capsule_support = 0; + if (auth_context.sig_size) + image.image_capsule_support |= CAPSULE_SUPPORT_AUTHENTICATION;
size = fwrite(&image, 1, sizeof(image), f); if (size < sizeof(image)) { printf("write failed (%zx)\n", size); - goto err_3; + goto err_4; } - size = fread(data, 1, bin_stat.st_size, g); - if (size < bin_stat.st_size) { - printf("read failed (%zx)\n", size); - goto err_3; + + if (auth_context.sig_size) { + size = fwrite(&auth_context.auth, 1, + sizeof(auth_context.auth), f); + if (size < sizeof(auth_context.auth)) { + printf("write failed (%zx)\n", size); + goto err_4; + } + size = fwrite(auth_context.sig_data, 1, + auth_context.sig_size, f); + if (size < auth_context.sig_size) { + printf("write failed (%zx)\n", size); + goto err_4; + } } + size = fwrite(data, 1, bin_stat.st_size, f); if (size < bin_stat.st_size) { printf("write failed (%zx)\n", size); - goto err_3; + goto err_4; }
fclose(f); fclose(g); free(data); + free_sig_data(&auth_context);
return 0;
-err_3: +err_4: fclose(f); +err_3: + free_sig_data(&auth_context); err_2: free(data); err_1: @@ -171,23 +510,37 @@ err_1: return -1; }
-/* - * Usage: - * $ mkeficapsule -f <firmware binary> <output file> +/** + * main - main entry function of mkeficapsule + * @argc: Number of arguments + * @argv: Array of pointers to arguments + * + * Create an uefi capsule file, optionally signing it. + * Parse all the arguments and pass them on to create_fwbin(). + * + * Return: + * * 0 - on success + * * -1 - on failure */ int main(int argc, char **argv) { char *file; efi_guid_t *guid; unsigned long index, instance; + uint64_t mcount; + char *privkey_file, *cert_file; int c, idx;
file = NULL; guid = NULL; index = 0; instance = 0; + mcount = 0; + privkey_file = NULL; + cert_file = NULL; + dump_sig = 0; for (;;) { - c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx); + c = getopt_long(argc, argv, opts_short, options, &idx); if (c == -1) break;
@@ -214,29 +567,47 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break; +#ifdef CONFIG_TOOLS_LIBCRYPTO + case 'p': + if (privkey_file) { + printf("Private Key already specified\n"); + return -1; + } + privkey_file = optarg; + break; + case 'c': + if (cert_file) { + printf("Certificate file already specified\n"); + return -1; + } + cert_file = optarg; + break; + case 'm': + mcount = strtoul(optarg, NULL, 0); + break; + case 'd': + dump_sig = 1; + break; +#endif /* CONFIG_TOOLS_LIBCRYPTO */ case 'h': print_usage(); return 0; } }
- /* need an output file */ - if (argc != optind + 1) { - print_usage(); - exit(EXIT_FAILURE); - } - - /* need a fit image file or raw image file */ - if (!file) { + /* check necessary parameters */ + if ((argc != optind + 1) || !file || + ((privkey_file && !cert_file) || + (!privkey_file && cert_file))) { print_usage(); - exit(EXIT_SUCCESS); + return -1; }
- if (create_fwbin(argv[optind], file, guid, index, instance) - < 0) { + if (create_fwbin(argv[optind], file, guid, index, instance, + mcount, privkey_file, cert_file) < 0) { printf("Creating firmware capsule failed\n"); - exit(EXIT_FAILURE); + return -1; }
- exit(EXIT_SUCCESS); + return 0; }
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
With this enhancement, mkeficapsule will be able to sign a capsule file when it is created. A signature added will be used later in the verification at FMP's SetImage() call.
To do that, We need specify additional command parameters: -monotonic-cout <count> : monotonic count -private-key <private key file> : private key file -certificate <certificate file> : certificate file Only when all of those parameters are given, a signature will be added to a capsule file.
Users are expected to maintain and increment the monotonic count at every time of the update for each firmware image.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
tools/Kconfig | 8 + tools/Makefile | 8 +- tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 417 insertions(+), 34 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This looks OK but I have some suggestions
- I don't think you should return -1 from main - could you split up your create_fwbin() to return the number of gotos? - could we have a man page for the tool? - should the files be opened in binary mode? - can we just build the tool always?
Regards, Simon
On Thu, Oct 28, 2021 at 09:17:45PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
With this enhancement, mkeficapsule will be able to sign a capsule file when it is created. A signature added will be used later in the verification at FMP's SetImage() call.
To do that, We need specify additional command parameters: -monotonic-cout <count> : monotonic count -private-key <private key file> : private key file -certificate <certificate file> : certificate file Only when all of those parameters are given, a signature will be added to a capsule file.
Users are expected to maintain and increment the monotonic count at every time of the update for each firmware image.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
tools/Kconfig | 8 + tools/Makefile | 8 +- tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 417 insertions(+), 34 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Thank you for your reviewing.
This looks OK but I have some suggestions
- I don't think you should return -1 from main
exit(EXIT_FAILURE)? Yeah, but when I first wrote this tool (without authentication support), 'return -1' was used everywhere. So I didn't want to have mixed styles in this patch. I will make a change with the tweak below.
- could you split up your create_fwbin() to return the number of gotos?
Yeah, lots of gotos are messy.
- could we have a man page for the tool?
Patch#3
- should the files be opened in binary mode?
Well, the man page of fopen() says, This is strictly for compatibility with C89 and has no effect; the 'b' is ignored on all POSIX conforming sys- tems, including Linux.
U-Boot now requires C11, and so no need?
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
# I saw some discussion in another topic thread, and some distro guy said # that they used sandbox_defconfig for tool packaging.
-Takahiro Akashi
Regards, Simon
Hi Takahiro,
On Thu, 28 Oct 2021 at 22:56, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:45PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
With this enhancement, mkeficapsule will be able to sign a capsule file when it is created. A signature added will be used later in the verification at FMP's SetImage() call.
To do that, We need specify additional command parameters: -monotonic-cout <count> : monotonic count -private-key <private key file> : private key file -certificate <certificate file> : certificate file Only when all of those parameters are given, a signature will be added to a capsule file.
Users are expected to maintain and increment the monotonic count at every time of the update for each firmware image.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
tools/Kconfig | 8 + tools/Makefile | 8 +- tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 417 insertions(+), 34 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Thank you for your reviewing.
This looks OK but I have some suggestions
- I don't think you should return -1 from main
exit(EXIT_FAILURE)? Yeah, but when I first wrote this tool (without authentication support), 'return -1' was used everywhere. So I didn't want to have mixed styles in this patch. I will make a change with the tweak below.
OK. I just mean that I think the return code is supposed to be 1 or 2 or maybe 3 on error, not 255.
- could you split up your create_fwbin() to return the number of gotos?
Yeah, lots of gotos are messy.
- could we have a man page for the tool?
Patch#3
OK
- should the files be opened in binary mode?
Well, the man page of fopen() says, This is strictly for compatibility with C89 and has no effect; the 'b' is ignored on all POSIX conforming sys- tems, including Linux.
U-Boot now requires C11, and so no need?
Ah OK. I suppose no one builds this on Windows.
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
# I saw some discussion in another topic thread, and some distro guy said # that they used sandbox_defconfig for tool packaging.
What about tools-only ?
So long as the options are enabled it is fine to have options for the tools. But I think we should try to build all the tools.
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: In file included from include/linux/string.h:21: ./arch/arm/include/asm/string.h:20:15: error: conflicting types for 'memcpy' extern void * memcpy(void *, const void *, __kernel_size_t); ^ /usr/include/string.h:65:7: note: previous declaration is here void *memcpy(void *__restrict, const void *__restrict, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: In file included from include/linux/string.h:21: ./arch/arm/include/asm/string.h:27:15: error: conflicting types for 'memmove' extern void * memmove(void *, const void *, __kernel_size_t); ^ /usr/include/string.h:68:7: note: previous declaration is here void *memmove(void *, const void *, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: In file included from include/linux/string.h:21: ./arch/arm/include/asm/string.h:30:15: error: conflicting types for 'memchr' extern void * memchr(const void *, int, __kernel_size_t); ^ /usr/include/string.h:63:7: note: previous declaration is here void *memchr(const void *, int, size_t); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: In file included from include/linux/string.h:21: ./arch/arm/include/asm/string.h:36:15: error: conflicting types for 'memset' extern void * memset(void *, int, __kernel_size_t); ^ /usr/include/string.h:71:7: note: previous declaration is here void *memset(void *, int, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:27:15: error: conflicting types for 'strncpy' extern char * strncpy(char *,const char *, __kernel_size_t); ^ /usr/include/string.h:84:7: note: previous declaration is here char *strncpy(char *__restrict, const char *__restrict, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:36:15: error: conflicting types for 'strncat' extern char * strncat(char *, const char *, __kernel_size_t); ^ /usr/include/string.h:81:7: note: previous declaration is here char *strncat(char *__restrict, const char *__restrict, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:45:12: error: conflicting types for 'strncmp' extern int strncmp(const char *,const char *,__kernel_size_t); ^ /usr/include/string.h:83:6: note: previous declaration is here int strncmp(const char *, const char *, size_t); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:51:12: error: conflicting types for 'strncasecmp' extern int strncasecmp(const char *s1, const char *s2, __kernel_size_t len); ^ /usr/include/strings.h:75:6: note: previous declaration is here int strncasecmp(const char *, const char *, size_t); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:77:24: error: conflicting types for 'strlen' extern __kernel_size_t strlen(const char *); ^ /usr/include/string.h:80:9: note: previous declaration is here size_t strlen(const char *); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:80:24: error: conflicting types for 'strnlen' extern __kernel_size_t strnlen(const char *,__kernel_size_t); ^ /usr/include/string.h:115:9: note: previous declaration is here size_t strnlen(const char *, size_t); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:111:15: error: conflicting types for 'memset' extern void * memset(void *,int,__kernel_size_t); ^ /usr/include/string.h:71:7: note: previous declaration is here void *memset(void *, int, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:114:15: error: conflicting types for 'memcpy' extern void * memcpy(void *,const void *,__kernel_size_t); ^ /usr/include/string.h:65:7: note: previous declaration is here void *memcpy(void *__restrict, const void *__restrict, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:117:15: error: conflicting types for 'memmove' extern void * memmove(void *,const void *,__kernel_size_t); ^ /usr/include/string.h:68:7: note: previous declaration is here void *memmove(void *, const void *, size_t) ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:123:12: error: conflicting types for 'memcmp' extern int memcmp(const void *,const void *,__kernel_size_t); ^ /usr/include/string.h:64:6: note: previous declaration is here int memcmp(const void *, const void *, size_t); ^ In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:126:15: error: conflicting types for 'memchr' extern void * memchr(const void *,int,__kernel_size_t); ^ /usr/include/string.h:63:7: note: previous declaration is here void *memchr(const void *, int, size_t); ^ tools/mkeficapsule.c:25:9: warning: 'aligned_u64' macro redefined [-Wmacro-redefined] #define aligned_u64 __aligned_u64 ^ include/linux/types.h:118:9: note: previous definition is here #define aligned_u64 __u64 __aligned(8) ^ In file included from tools/mkeficapsule.c:32: In file included from include/efi_api.h:20: In file included from include/charset.h:11: In file included from include/linux/kernel.h:5: In file included from include/linux/printk.h:4: include/log.h:382:24: error: expression is not an integer constant expression LOGRECF_FORCE_DEBUG = BIT(0), ^~~~~~ include/log.h:384:18: error: expression is not an integer constant expression LOGRECF_CONT = BIT(1), ^~~~~~ include/log.h:421:18: error: expression is not an integer constant expression LOGDF_ENABLE = BIT(0), /* Device is enabled */ ^~~~~~ fatal error: too many errors emitted, stopping now [-ferror-limit=] 1 warning and 20 errors generated.
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^ This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Thu, 4 Nov 2021 09:11:59 -0600
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
> - can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Last time that came it was pointed out that Azure pipelines only support Windows, macOS and Linux. I would expect that a macOS build would catch the issues I'm seeing on OpenBSD.
Hi Mark,
On Thu, 4 Nov 2021 at 10:51, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 4 Nov 2021 09:11:59 -0600
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,e
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
> > - can we just build the tool always? > > This is one of my questions. > Why do you want to do so while there are bunch of tools that are > not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Last time that came it was pointed out that Azure pipelines only support Windows, macOS and Linux. I would expect that a macOS build would catch the issues I'm seeing on OpenBSD.
So should we have something in doc/build/openbsd.rst ? I have installed openbsd but have no idea how to build the tools or what prereqs are needed, nor how to install them...
Regards, SImon
From: Simon Glass sjg@chromium.org Date: Thu, 4 Nov 2021 20:02:38 -0600
Hi Mark,
On Thu, 4 Nov 2021 at 10:51, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 4 Nov 2021 09:11:59 -0600
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,e
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
> From: Simon Glass sjg@chromium.org > Date: Tue, 2 Nov 2021 08:56:50 -0600 > > Hi Takahiro, > > > > - can we just build the tool always? > > > > This is one of my questions. > > Why do you want to do so while there are bunch of tools that are > > not always built. > > Because I think all tools should be built always. It is fine if that > happens due to CONFIG options but we should try to avoid making it > complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Last time that came it was pointed out that Azure pipelines only support Windows, macOS and Linux. I would expect that a macOS build would catch the issues I'm seeing on OpenBSD.
So should we have something in doc/build/openbsd.rst ? I have installed openbsd but have no idea how to build the tools or what prereqs are needed, nor how to install them...
I suppose I could write something like that. Seems that the build requirements for Linux are actually documented in doc/build/gcc.rst? Could simply add an OpenBSD paragraph to that.
Hi, Simon,
On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
> - can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
As far as I know, you initially introduced efi.h and efi_api.h. What is your intent to have the two?
I think that efi_api.h contains definitions and interfaces defined in UEFI specification for building EFI application/modules, hence I believe that it should be target-independent. Right?
But it *includes* efi.h which also contains some definitions defined in UEFI specification, while efi.h is only for U-Boot as UEFI application.
I suspect that is the root cause. Or should we thoroughly use linux headers like "efi/efi.h" in this tool?
-Takahiro Akashi
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi, Simon,
On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
> > - can we just build the tool always? > > This is one of my questions. > Why do you want to do so while there are bunch of tools that are > not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
As far as I know, you initially introduced efi.h and efi_api.h. What is your intent to have the two?
I think that efi_api.h contains definitions and interfaces defined in UEFI specification for building EFI application/modules, hence I believe that it should be target-independent. Right?
But it *includes* efi.h which also contains some definitions defined in UEFI specification, while efi.h is only for U-Boot as UEFI application.
I suspect that is the root cause.
Yes I think you are right.
Or should we thoroughly use linux headers like "efi/efi.h" in this tool?
Well either way, we need host builds to not include U-Boot headers.
- Simon
-Takahiro Akashi
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi, Simon,
On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
> From: Simon Glass sjg@chromium.org > Date: Tue, 2 Nov 2021 08:56:50 -0600 > > Hi Takahiro, > > > > - can we just build the tool always? > > > > This is one of my questions. > > Why do you want to do so while there are bunch of tools that are > > not always built. > > Because I think all tools should be built always. It is fine if that > happens due to CONFIG options but we should try to avoid making it > complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
As far as I know, you initially introduced efi.h and efi_api.h. What is your intent to have the two?
I think that efi_api.h contains definitions and interfaces defined in UEFI specification for building EFI application/modules, hence I believe that it should be target-independent. Right?
But it *includes* efi.h which also contains some definitions defined in UEFI specification, while efi.h is only for U-Boot as UEFI application.
I suspect that is the root cause.
Yes I think you are right.
Or should we thoroughly use linux headers like "efi/efi.h" in this tool?
Well either way, we need host builds to not include U-Boot headers.
Yeah, but there are still lots of host tools which include U-Boot headers. In addition, I'm not quite sure whether *generic* efi headers, like efi/efi.h, are available across different host OSs.
-Takahiro Akashi
- Simon
-Takahiro Akashi
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
On Fri, Nov 05, 2021 at 11:35:00AM +0900, AKASHI Takahiro wrote:
On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi, Simon,
On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote: > > > From: Simon Glass sjg@chromium.org > > Date: Tue, 2 Nov 2021 08:56:50 -0600 > > > > Hi Takahiro, > > > > > > - can we just build the tool always? > > > > > > This is one of my questions. > > > Why do you want to do so while there are bunch of tools that are > > > not always built. > > > > Because I think all tools should be built always. It is fine if that > > happens due to CONFIG options but we should try to avoid making it > > complicated. > > Well, unless this patchset fixes things, we can't, because > mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I > can't figure out how this is even supposed to compile as a host tool: > > > In file included from tools/mkeficapsule.c:8: > In file included from include/malloc.h:369: > include/linux/string.h:15:24: error: conflicting types for 'strspn' > extern __kernel_size_t strspn(const char *,const char *); > ^ > /usr/include/string.h:88:9: note: previous declaration is here > size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
As far as I know, you initially introduced efi.h and efi_api.h. What is your intent to have the two?
I think that efi_api.h contains definitions and interfaces defined in UEFI specification for building EFI application/modules, hence I believe that it should be target-independent. Right?
But it *includes* efi.h which also contains some definitions defined in UEFI specification, while efi.h is only for U-Boot as UEFI application.
I suspect that is the root cause.
Yes I think you are right.
Or should we thoroughly use linux headers like "efi/efi.h" in this tool?
Well either way, we need host builds to not include U-Boot headers.
Yeah, but there are still lots of host tools which include U-Boot headers. In addition, I'm not quite sure whether *generic* efi headers, like efi/efi.h, are available across different host OSs.
I looked through linux's efi headers under /usr/include/efi, but they don't provide enough set of definitions to make mkeficapsule buildable. Particularly, capsule-related structure definitions are missing.
So modifying U-Boot headers and removing target-dependent coding would be more practical. (I don't know yet whether it is feasible or not.)
Or even adding host-tools-local headers would be more optimal.
-Takahiro Akashi
-Takahiro Akashi
- Simon
-Takahiro Akashi
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
Heinrich,
On Fri, Nov 05, 2021 at 06:35:08PM +0900, AKASHI Takahiro wrote:
On Fri, Nov 05, 2021 at 11:35:00AM +0900, AKASHI Takahiro wrote:
On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi, Simon,
On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote:
> From: Simon Glass sjg@chromium.org > Date: Wed, 3 Nov 2021 20:51:25 -0600 > > Hi Mark, > > On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote: > > > > > From: Simon Glass sjg@chromium.org > > > Date: Tue, 2 Nov 2021 08:56:50 -0600 > > > > > > Hi Takahiro, > > > > > > > > - can we just build the tool always? > > > > > > > > This is one of my questions. > > > > Why do you want to do so while there are bunch of tools that are > > > > not always built. > > > > > > Because I think all tools should be built always. It is fine if that > > > happens due to CONFIG options but we should try to avoid making it > > > complicated. > > > > Well, unless this patchset fixes things, we can't, because > > mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I > > can't figure out how this is even supposed to compile as a host tool: > > > > > > In file included from tools/mkeficapsule.c:8: > > In file included from include/malloc.h:369: > > include/linux/string.h:15:24: error: conflicting types for 'strspn' > > extern __kernel_size_t strspn(const char *,const char *); > > ^ > > /usr/include/string.h:88:9: note: previous declaration is here > > size_t strspn(const char *, const char *); > > My guess is that linux/string.h should not be included, or perhaps > __kernel_size_t should be defined to size_t. > > I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^
This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
As far as I know, you initially introduced efi.h and efi_api.h. What is your intent to have the two?
I think that efi_api.h contains definitions and interfaces defined in UEFI specification for building EFI application/modules, hence I believe that it should be target-independent. Right?
But it *includes* efi.h which also contains some definitions defined in UEFI specification, while efi.h is only for U-Boot as UEFI application.
I suspect that is the root cause.
Yes I think you are right.
Or should we thoroughly use linux headers like "efi/efi.h" in this tool?
Well either way, we need host builds to not include U-Boot headers.
Yeah, but there are still lots of host tools which include U-Boot headers. In addition, I'm not quite sure whether *generic* efi headers, like efi/efi.h, are available across different host OSs.
I looked through linux's efi headers under /usr/include/efi, but they don't provide enough set of definitions to make mkeficapsule buildable. Particularly, capsule-related structure definitions are missing.
So modifying U-Boot headers and removing target-dependent coding would be more practical. (I don't know yet whether it is feasible or not.)
What's your thought here?
Or even adding host-tools-local headers would be more optimal.
I prefer this approach, though.
-Takahiro Akashi
-Takahiro Akashi
-Takahiro Akashi
- Simon
-Takahiro Akashi
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
Heinrich,
On Mon, Nov 08, 2021 at 01:55:24PM +0900, AKASHI Takahiro wrote:
Heinrich,
On Fri, Nov 05, 2021 at 06:35:08PM +0900, AKASHI Takahiro wrote:
On Fri, Nov 05, 2021 at 11:35:00AM +0900, AKASHI Takahiro wrote:
On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi, Simon,
On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
Hi Mark,
On Thu, 4 Nov 2021 at 08:31, Mark Kettenis mark.kettenis@xs4all.nl wrote: > > > From: Simon Glass sjg@chromium.org > > Date: Wed, 3 Nov 2021 20:51:25 -0600 > > > > Hi Mark, > > > > On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote: > > > > > > > From: Simon Glass sjg@chromium.org > > > > Date: Tue, 2 Nov 2021 08:56:50 -0600 > > > > > > > > Hi Takahiro, > > > > > > > > > > - can we just build the tool always? > > > > > > > > > > This is one of my questions. > > > > > Why do you want to do so while there are bunch of tools that are > > > > > not always built. > > > > > > > > Because I think all tools should be built always. It is fine if that > > > > happens due to CONFIG options but we should try to avoid making it > > > > complicated. > > > > > > Well, unless this patchset fixes things, we can't, because > > > mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I > > > can't figure out how this is even supposed to compile as a host tool: > > > > > > > > > In file included from tools/mkeficapsule.c:8: > > > In file included from include/malloc.h:369: > > > include/linux/string.h:15:24: error: conflicting types for 'strspn' > > > extern __kernel_size_t strspn(const char *,const char *); > > > ^ > > > /usr/include/string.h:88:9: note: previous declaration is here > > > size_t strspn(const char *, const char *); > > > > My guess is that linux/string.h should not be included, or perhaps > > __kernel_size_t should be defined to size_t. > > > > I doubt it would take an age to figure out, with a bit of fiddling. > > Well, I think the problem is quite fundamental. Indeed I agree that > linux/string.h shouldn't be included. It gets pulled in because the > tools include <malloc.h>. Modern software really shouldn't include > that header anymore, and we removed it in OpenBSD some time ago. But > even with that fixed, things break since the same header gets pulled > in from <efi.h>. > > Redefining __kernel_size_t doesn't provide a way out: > > tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') > typedef size_t __kernel_size_t; > ^ > ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here > typedef unsigned int __kernel_size_t; > ^ > > This is on an amd64 host, so "unsigned int" clearly is the wrong type > for size_t. > > The fundamental problem seems to be that <efi.h> isn't safe to include > in a "host" tool because it includes "target" headers that > accidentally resolve to "system" headers on Linux systems. > > Maybe Takahiro or Heinrich have an idea how to fix that? But in the > meantime it would be good if building this tool would remain optional.
Yes let's ask them to fix that as I agree this sounds wrong. We have several efi headers so perhaps just need to have the right stuff in each.
As far as I know, you initially introduced efi.h and efi_api.h. What is your intent to have the two?
I think that efi_api.h contains definitions and interfaces defined in UEFI specification for building EFI application/modules, hence I believe that it should be target-independent. Right?
But it *includes* efi.h which also contains some definitions defined in UEFI specification, while efi.h is only for U-Boot as UEFI application.
I suspect that is the root cause.
Yes I think you are right.
Or should we thoroughly use linux headers like "efi/efi.h" in this tool?
Well either way, we need host builds to not include U-Boot headers.
Yeah, but there are still lots of host tools which include U-Boot headers. In addition, I'm not quite sure whether *generic* efi headers, like efi/efi.h, are available across different host OSs.
I looked through linux's efi headers under /usr/include/efi, but they don't provide enough set of definitions to make mkeficapsule buildable. Particularly, capsule-related structure definitions are missing.
So modifying U-Boot headers and removing target-dependent coding would be more practical. (I don't know yet whether it is feasible or not.)
What's your thought here?
Or even adding host-tools-local headers would be more optimal.
I prefer this approach, though.
I need your feedback on fixing this issue.
-Takahiro Akashi
-Takahiro Akashi
-Takahiro Akashi
-Takahiro Akashi
- Simon
-Takahiro Akashi
It is OK to have it optional with a CONFIG, but it should be enabled by default, otherwise no one will know it is there.
Can we get the OpenBSD environment into CI or is that just too hard?
Regards, Simon
Hi Mark,
On Thu, Nov 04, 2021 at 03:31:40PM +0100, Mark Kettenis wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 3 Nov 2021 20:51:25 -0600
Hi Mark,
On Tue, 2 Nov 2021 at 09:13, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 2 Nov 2021 08:56:50 -0600
Hi Takahiro,
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
Well, unless this patchset fixes things, we can't, because mkeficapsule doesn't build on OpenBSD. I tried looking into it, but I can't figure out how this is even supposed to compile as a host tool:
In file included from tools/mkeficapsule.c:8: In file included from include/malloc.h:369: include/linux/string.h:15:24: error: conflicting types for 'strspn' extern __kernel_size_t strspn(const char *,const char *); ^ /usr/include/string.h:88:9: note: previous declaration is here size_t strspn(const char *, const char *);
My guess is that linux/string.h should not be included, or perhaps __kernel_size_t should be defined to size_t.
I doubt it would take an age to figure out, with a bit of fiddling.
Well, I think the problem is quite fundamental. Indeed I agree that linux/string.h shouldn't be included. It gets pulled in because the tools include <malloc.h>. Modern software really shouldn't include that header anymore, and we removed it in OpenBSD some time ago. But even with that fixed, things break since the same header gets pulled in from <efi.h>.
Redefining __kernel_size_t doesn't provide a way out:
tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int') typedef size_t __kernel_size_t; ^ ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here typedef unsigned int __kernel_size_t; ^ This is on an amd64 host, so "unsigned int" clearly is the wrong type for size_t.
The fundamental problem seems to be that <efi.h> isn't safe to include in a "host" tool because it includes "target" headers that accidentally resolve to "system" headers on Linux systems.
Maybe Takahiro or Heinrich have an idea how to fix that? But in the meantime it would be good if building this tool would remain optional.
Please let me confirm one thing; you can't build this tool even without my patch applied. Right? If so, we may want to discuss and figure out a solution after the patch is merged as long as the tool won't be built neither by Kconfig default nor by tools-only_defconfig.
-Takahiro Akashi
On Tue, Nov 02, 2021 at 08:56:50AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 22:56, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:45PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
With this enhancement, mkeficapsule will be able to sign a capsule file when it is created. A signature added will be used later in the verification at FMP's SetImage() call.
To do that, We need specify additional command parameters: -monotonic-cout <count> : monotonic count -private-key <private key file> : private key file -certificate <certificate file> : certificate file Only when all of those parameters are given, a signature will be added to a capsule file.
Users are expected to maintain and increment the monotonic count at every time of the update for each firmware image.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
tools/Kconfig | 8 + tools/Makefile | 8 +- tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 417 insertions(+), 34 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Thank you for your reviewing.
This looks OK but I have some suggestions
- I don't think you should return -1 from main
exit(EXIT_FAILURE)? Yeah, but when I first wrote this tool (without authentication support), 'return -1' was used everywhere. So I didn't want to have mixed styles in this patch. I will make a change with the tweak below.
OK. I just mean that I think the return code is supposed to be 1 or 2 or maybe 3 on error, not 255.
- could you split up your create_fwbin() to return the number of gotos?
Yeah, lots of gotos are messy.
- could we have a man page for the tool?
Patch#3
OK
- should the files be opened in binary mode?
Well, the man page of fopen() says, This is strictly for compatibility with C89 and has no effect; the 'b' is ignored on all POSIX conforming sys- tems, including Linux.
U-Boot now requires C11, and so no need?
Ah OK. I suppose no one builds this on Windows.
- can we just build the tool always?
This is one of my questions. Why do you want to do so while there are bunch of tools that are not always built.
Because I think all tools should be built always. It is fine if that happens due to CONFIG options but we should try to avoid making it complicated.
# I saw some discussion in another topic thread, and some distro guy said # that they used sandbox_defconfig for tool packaging.
What about tools-only ?
So long as the options are enabled it is fine to have options for the tools. But I think we should try to build all the tools.
I forgot to add CMD_MKEFITOOL in tools-only_defconfig in v6. If I need to send v7, I will include it, otherwise send it in a separate patch.
-Takahiro Akashi
Regards, Simon
Add a man page for mkeficapsule command.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- MAINTAINERS | 1 + doc/mkeficapsule.1 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 doc/mkeficapsule.1
diff --git a/MAINTAINERS b/MAINTAINERS index 5069f1880652..96233f0aad51 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -707,6 +707,7 @@ S: Maintained T: git https://source.denx.de/u-boot/custodians/u-boot-efi.git F: doc/api/efi.rst F: doc/develop/uefi/* +F: doc/mkeficapsule.1 F: doc/usage/bootefi.rst F: drivers/rtc/emul_rtc.c F: include/capitalization.h diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 new file mode 100644 index 000000000000..837e09ab451e --- /dev/null +++ b/doc/mkeficapsule.1 @@ -0,0 +1,95 @@ +.TH MAEFICAPSULE 1 "May 2021" + +.SH NAME +mkeficapsule - Generate EFI capsule file for U-Boot + +.SH SYNOPSIS +.B mkeficapsule +.RB [\fIoptions\fP] " \fIcapsule-file\fP" + +.SH "DESCRIPTION" +The +\fBmkeficapsule\fP +command is used to create an EFI capsule file for use with the U-Boot +EFI capsule update. +A capsule file may contain various type of firmware blobs which +are to be applied to the system and must be placed in the specific +directory on the UEFI system partition. An update will be automatically +executed at next reboot. + +Optionally, a capsule file can be signed with a given private key. +In this case, the update will be authenticated by verifying the signature +before applying. + +\fBmkeficapsule\fP supports two different format of image files: +.TP +.I raw image +format is a single binary blob of any type of firmware. + +.TP +.I FIT (Flattened Image Tree) image +format +is the same as used in the new \fIuImage\fP format and allows for +multiple binary blobs in a single capsule file. +This type of image file can be generated by \fBmkimage\fP. + +.SH "OPTIONS" +One of \fB--fit\fP or \fB--raw\fP option must be specified. + +.TP +.BI "-f, --fit \fIfit-image-file\fP" +Specify a FIT image file + +.TP +.BI "-r, --raw \fIraw-image-file\fP" +Specify a raw image file + +.TP +.BI "-i, --index \fIindex\fP" +Specify an image index + +.TP +.BI "-I, --instance \fIinstance\fP" +Specify a hardware instance + +.TP +.BI "-h, --help" +Print a help message + +.TP 0 +.B With signing: + +\fB--private-key\fP, \fB--certificate\fP and \fB--monotonic-count\fP are +all mandatory. + +.TP +.BI "-p, --private-key \fIprivate-key-file\fP" +Specify signer's private key file in PEM + +.TP +.BI "-c, --certificate \fIcertificate-file\fP" +Specify signer's certificate file in EFI certificate list format + +.TP +.BI "-m, --monotonic-count \fIcount\fP" +Specify a monotonic count which is set to be monotonically incremented +at every firmware update. + +.TP +.BI "-d, --dump_sig" +Dump signature data into *.p7 file + +.PP +.SH FILES +.TP +.BI "\fI/EFI/UpdateCapsule\fP" +The directory in which all capsule files be placed + +.SH SEE ALSO +.B mkimage + +.SH AUTHORS +Written by AKASHI Takahiro takahiro.akashi@linaro.org + +.SH HOMEPAGE +http://www.denx.de/wiki/U-Boot/WebHome
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a man page for mkeficapsule command.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
MAINTAINERS | 1 + doc/mkeficapsule.1 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 doc/mkeficapsule.1
Reviewed-by: Simon Glass sjg@chromium.org
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory. - -The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory:: + + \EFI\UpdateCapsule + +The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option. + +Please note that capsules will be applied in the alphabetic order of +capsule file names. + +Creating a capsule file +*********************** + +A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable:: + + CONFIG_TOOLS_MKEFICAPSULE=y + CONFIG_TOOLS_LIBCRYPTO=y + +Run the following command:: + + $ mkeficapsule \ + --index 1 --instance 0 \ + [--fit <FIT image> | --raw <raw image>] \ + <capsule_file_name>
Performing the update *********************
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule. - -If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04
-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update - -**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication ******************************* @@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y - CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- 1. Install utility commands on your host - * OPENSSL +1. Install utility commands on your host + * openssl * efitools
- 2. Create signing keys and certificate files on your host +2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
- $ openssl x509 -in CRT.crt -out CRT.cer -outform DER - $ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem - - $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt - $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem - -The capsule file can be generated by using the GenerateCapsule.py -script in EDKII:: - - $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \ - <capsule_file_name> --monotonic-count <val> --fw-version \ - <val> --lsv <val> --guid \ - e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \ - --update-image-index <val> --signer-private-cert \ - /path/to/CRT.pem --trusted-public-cert \ - /path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \ - <u-boot.bin> - -Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory - -Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000. + $ mkeficapsule --monotonic-count 1 \ + --private-key CRT.key \ + --certificate CRT.crt \ + --index 1 --instance 0 \ + [--fit <FIT image> | --raw <raw image>] \ + <capsule_file_name>
-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y - CONFIG_FLASH_CFI_MTD=y - CONFIG_CMD_MTDPARTS=y - CONFIG_CMD_DFU=y - CONFIG_DFU_MTD=y - CONFIG_PCI_INIT_R=y - CONFIG_EFI_CAPSULE_ON_DISK=y - CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y - CONFIG_EFI_CAPSULE_FIRMWARE=y - CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y + { + signature { + capsule-key = [ <binary of signature list> ]; + } + ... + }
-In addition, the following config needs to be disabled(QEMU ARM specific):: + You can do this manually with::
- CONFIG_TFABOOT + $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts + $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule:: + where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name> + &{/} { + signature { + capsule-key = /incbin/("CRT.esl"); + }; + };
Executing the boot manager ~~~~~~~~~~~~~~~~~~~~~~~~~~
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory.
-The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory::
- \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
+The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option.
+Please note that capsules will be applied in the alphabetic order of +capsule file names.
+Creating a capsule file +***********************
+A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable::
- CONFIG_TOOLS_MKEFICAPSULE=y
- CONFIG_TOOLS_LIBCRYPTO=y
+Run the following command::
- $ mkeficapsule \
--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>Performing the update
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
-If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update
-**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication
@@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y
- CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- Install utility commands on your host
* OPENSSL+1. Install utility commands on your host
* openssl * efitools
- Create signing keys and certificate files on your host
+2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
$ openssl x509 -in CRT.crt -out CRT.cer -outform DER$ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem$ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt$ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem-The capsule file can be generated by using the GenerateCapsule.py -script in EDKII::
- $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
<capsule_file_name> --monotonic-count <val> --fw-version \<val> --lsv <val> --guid \e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \--update-image-index <val> --signer-private-cert \/path/to/CRT.pem --trusted-public-cert \/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \<u-boot.bin>-Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory
-Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000.
- $ mkeficapsule --monotonic-count 1 \
--private-key CRT.key \--certificate CRT.crt \--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y
- CONFIG_FLASH_CFI_MTD=y
- CONFIG_CMD_MTDPARTS=y
- CONFIG_CMD_DFU=y
- CONFIG_DFU_MTD=y
- CONFIG_PCI_INIT_R=y
- CONFIG_EFI_CAPSULE_ON_DISK=y
- CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
- CONFIG_EFI_CAPSULE_FIRMWARE=y
- CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
{signature {capsule-key = [ <binary of signature list> ];}
Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
...}-In addition, the following config needs to be disabled(QEMU ARM specific)::
- You can do this manually with::
- CONFIG_TFABOOT
- $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
- $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule::
- where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
&{/} {signature {capsule-key = /incbin/("CRT.esl");};};
Ick. I think your tool should just support adding the signature.
Executing the boot manager
-- 2.33.0
Regards, Simon
Regards, SImon
On Thu, Oct 28, 2021 at 09:17:48PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory.
-The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory::
- \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
UEFI specification. In this document, all the file paths are presented with backslashes. (See section 8.5.5 in version 2.9)
Anyhow U-Boot UEFI internally converts the path with slashes.
+The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option.
+Please note that capsules will be applied in the alphabetic order of +capsule file names.
+Creating a capsule file +***********************
+A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable::
- CONFIG_TOOLS_MKEFICAPSULE=y
- CONFIG_TOOLS_LIBCRYPTO=y
+Run the following command::
- $ mkeficapsule \
--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>Performing the update
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
-If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update
-**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication
@@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y
- CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- Install utility commands on your host
* OPENSSL+1. Install utility commands on your host
* openssl * efitools
- Create signing keys and certificate files on your host
+2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
$ openssl x509 -in CRT.crt -out CRT.cer -outform DER$ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem$ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt$ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem-The capsule file can be generated by using the GenerateCapsule.py -script in EDKII::
- $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
<capsule_file_name> --monotonic-count <val> --fw-version \<val> --lsv <val> --guid \e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \--update-image-index <val> --signer-private-cert \/path/to/CRT.pem --trusted-public-cert \/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \<u-boot.bin>-Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory
-Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000.
- $ mkeficapsule --monotonic-count 1 \
--private-key CRT.key \--certificate CRT.crt \--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y
- CONFIG_FLASH_CFI_MTD=y
- CONFIG_CMD_MTDPARTS=y
- CONFIG_CMD_DFU=y
- CONFIG_DFU_MTD=y
- CONFIG_PCI_INIT_R=y
- CONFIG_EFI_CAPSULE_ON_DISK=y
- CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
- CONFIG_EFI_CAPSULE_FIRMWARE=y
- CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
{signature {capsule-key = [ <binary of signature list> ];}Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
As I said in the previous version, I don't know yet if binman is the best place. Can you give me a pointer where a similar feature is implemented in binman, please?
...}-In addition, the following config needs to be disabled(QEMU ARM specific)::
- You can do this manually with::
- CONFIG_TFABOOT
- $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
- $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule::
- where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
&{/} {signature {capsule-key = /incbin/("CRT.esl");};};Ick. I think your tool should just support adding the signature.
# I may misunderstand your point.
The whole purpose of this tool is to create a capsule file. Adding the signature to that file is simply an optional behavior. I don't see any reason that we should have those features in separate tools.
On the other hand, step.4 mentioned above is to add public keys (x509 certificate list or signature list in UEFI terminology) to a device tree. This is a separate step. Clear?
-Takahiro Akashi
Executing the boot manager
-- 2.33.0Regards, Simon
Regards, SImon
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:20, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:48PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory.
-The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory::
- \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
UEFI specification. In this document, all the file paths are presented with backslashes. (See section 8.5.5 in version 2.9)
Anyhow U-Boot UEFI internally converts the path with slashes.
So do we need to use backslashes in U-Boot and in the docs? Can we use a forward slash instead? I had hoped those days were behind us. The backslash is used for C escapes, after all.
+The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option.
+Please note that capsules will be applied in the alphabetic order of +capsule file names.
+Creating a capsule file +***********************
+A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable::
- CONFIG_TOOLS_MKEFICAPSULE=y
- CONFIG_TOOLS_LIBCRYPTO=y
+Run the following command::
- $ mkeficapsule \
--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>Performing the update
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
-If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update
-**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication
@@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y
- CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- Install utility commands on your host
* OPENSSL+1. Install utility commands on your host
* openssl * efitools
- Create signing keys and certificate files on your host
+2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
$ openssl x509 -in CRT.crt -out CRT.cer -outform DER$ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem$ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt$ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem-The capsule file can be generated by using the GenerateCapsule.py -script in EDKII::
- $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
<capsule_file_name> --monotonic-count <val> --fw-version \<val> --lsv <val> --guid \e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \--update-image-index <val> --signer-private-cert \/path/to/CRT.pem --trusted-public-cert \/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \<u-boot.bin>-Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory
-Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000.
- $ mkeficapsule --monotonic-count 1 \
--private-key CRT.key \--certificate CRT.crt \--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y
- CONFIG_FLASH_CFI_MTD=y
- CONFIG_CMD_MTDPARTS=y
- CONFIG_CMD_DFU=y
- CONFIG_DFU_MTD=y
- CONFIG_PCI_INIT_R=y
- CONFIG_EFI_CAPSULE_ON_DISK=y
- CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
- CONFIG_EFI_CAPSULE_FIRMWARE=y
- CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
{signature {capsule-key = [ <binary of signature list> ];}
Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
As I said in the previous version, I don't know yet if binman is the best place. Can you give me a pointer where a similar feature is implemented in binman, please?
See tools/binman/etype/vblock.py
...}-In addition, the following config needs to be disabled(QEMU ARM specific)::
- You can do this manually with::
- CONFIG_TFABOOT
- $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
- $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule::
- where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
&{/} {signature {capsule-key = /incbin/("CRT.esl");};};Ick. I think your tool should just support adding the signature.
# I may misunderstand your point.
The whole purpose of this tool is to create a capsule file. Adding the signature to that file is simply an optional behavior. I don't see any reason that we should have those features in separate tools.
On the other hand, step.4 mentioned above is to add public keys (x509 certificate list or signature list in UEFI terminology) to a device tree. This is a separate step. Clear?
It just seems a pain to create a DT overlay to add the signature.
I hope Binman can help here if you don't want to put it in your tool. I can write something if it would help.
Regards, Simon
On Tue, Nov 02, 2021 at 08:57:48AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:20, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:48PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory.
-The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory::
- \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
UEFI specification. In this document, all the file paths are presented with backslashes. (See section 8.5.5 in version 2.9)
Anyhow U-Boot UEFI internally converts the path with slashes.
So do we need to use backslashes in U-Boot and in the docs? Can we use a forward slash instead? I had hoped those days were behind us. The backslash is used for C escapes, after all.
I'd defer to the maintainer, Heinrich here.
+The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option.
+Please note that capsules will be applied in the alphabetic order of +capsule file names.
+Creating a capsule file +***********************
+A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable::
- CONFIG_TOOLS_MKEFICAPSULE=y
- CONFIG_TOOLS_LIBCRYPTO=y
+Run the following command::
- $ mkeficapsule \
--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>Performing the update
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
-If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update
-**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication
@@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y
- CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- Install utility commands on your host
* OPENSSL+1. Install utility commands on your host
* openssl * efitools
- Create signing keys and certificate files on your host
+2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
$ openssl x509 -in CRT.crt -out CRT.cer -outform DER$ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem$ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt$ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem-The capsule file can be generated by using the GenerateCapsule.py -script in EDKII::
- $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
<capsule_file_name> --monotonic-count <val> --fw-version \<val> --lsv <val> --guid \e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \--update-image-index <val> --signer-private-cert \/path/to/CRT.pem --trusted-public-cert \/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \<u-boot.bin>-Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory
-Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000.
- $ mkeficapsule --monotonic-count 1 \
--private-key CRT.key \--certificate CRT.crt \--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y
- CONFIG_FLASH_CFI_MTD=y
- CONFIG_CMD_MTDPARTS=y
- CONFIG_CMD_DFU=y
- CONFIG_DFU_MTD=y
- CONFIG_PCI_INIT_R=y
- CONFIG_EFI_CAPSULE_ON_DISK=y
- CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
- CONFIG_EFI_CAPSULE_FIRMWARE=y
- CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
{signature {capsule-key = [ <binary of signature list> ];}Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
As I said in the previous version, I don't know yet if binman is the best place. Can you give me a pointer where a similar feature is implemented in binman, please?
See tools/binman/etype/vblock.py
I'll check it out.
...}-In addition, the following config needs to be disabled(QEMU ARM specific)::
- You can do this manually with::
- CONFIG_TFABOOT
- $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
- $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule::
- where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
&{/} {signature {capsule-key = /incbin/("CRT.esl");};};Ick. I think your tool should just support adding the signature.
# I may misunderstand your point.
The whole purpose of this tool is to create a capsule file. Adding the signature to that file is simply an optional behavior. I don't see any reason that we should have those features in separate tools.
On the other hand, step.4 mentioned above is to add public keys (x509 certificate list or signature list in UEFI terminology) to a device tree. This is a separate step. Clear?
It just seems a pain to create a DT overlay to add the signature.
Pain in what sense? I have provided fdtsig.sh to avoid bothering users.
I hope Binman can help here if you don't want to put it in your tool. I can write something if it would help.
I think it's up to you to add the feature to binman, but also believe we should not and don't have to impose always using binman for this purpose on everyone, thinking of various situations that users may have.
Thanks, -Takahiro Akashi
Regards, Simon
Hi Takahiro,
On Wed, 3 Nov 2021 at 19:49, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:57:48AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:20, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:48PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory.
-The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory::
- \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
UEFI specification. In this document, all the file paths are presented with backslashes. (See section 8.5.5 in version 2.9)
Anyhow U-Boot UEFI internally converts the path with slashes.
So do we need to use backslashes in U-Boot and in the docs? Can we use a forward slash instead? I had hoped those days were behind us. The backslash is used for C escapes, after all.
I'd defer to the maintainer, Heinrich here.
+The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option.
+Please note that capsules will be applied in the alphabetic order of +capsule file names.
+Creating a capsule file +***********************
+A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable::
- CONFIG_TOOLS_MKEFICAPSULE=y
- CONFIG_TOOLS_LIBCRYPTO=y
+Run the following command::
- $ mkeficapsule \
--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>Performing the update
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
-If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update
-**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication
@@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y
- CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- Install utility commands on your host
* OPENSSL+1. Install utility commands on your host
* openssl * efitools
- Create signing keys and certificate files on your host
+2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
$ openssl x509 -in CRT.crt -out CRT.cer -outform DER$ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem$ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt$ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem-The capsule file can be generated by using the GenerateCapsule.py -script in EDKII::
- $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
<capsule_file_name> --monotonic-count <val> --fw-version \<val> --lsv <val> --guid \e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \--update-image-index <val> --signer-private-cert \/path/to/CRT.pem --trusted-public-cert \/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \<u-boot.bin>-Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory
-Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000.
- $ mkeficapsule --monotonic-count 1 \
--private-key CRT.key \--certificate CRT.crt \--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y
- CONFIG_FLASH_CFI_MTD=y
- CONFIG_CMD_MTDPARTS=y
- CONFIG_CMD_DFU=y
- CONFIG_DFU_MTD=y
- CONFIG_PCI_INIT_R=y
- CONFIG_EFI_CAPSULE_ON_DISK=y
- CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
- CONFIG_EFI_CAPSULE_FIRMWARE=y
- CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
{signature {capsule-key = [ <binary of signature list> ];}Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
As I said in the previous version, I don't know yet if binman is the best place. Can you give me a pointer where a similar feature is implemented in binman, please?
See tools/binman/etype/vblock.py
I'll check it out.
...}-In addition, the following config needs to be disabled(QEMU ARM specific)::
- You can do this manually with::
- CONFIG_TFABOOT
- $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
- $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule::
- where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
&{/} {signature {capsule-key = /incbin/("CRT.esl");};};Ick. I think your tool should just support adding the signature.
# I may misunderstand your point.
The whole purpose of this tool is to create a capsule file. Adding the signature to that file is simply an optional behavior. I don't see any reason that we should have those features in separate tools.
On the other hand, step.4 mentioned above is to add public keys (x509 certificate list or signature list in UEFI terminology) to a device tree. This is a separate step. Clear?
It just seems a pain to create a DT overlay to add the signature.
Pain in what sense? I have provided fdtsig.sh to avoid bothering users.
I hope Binman can help here if you don't want to put it in your tool. I can write something if it would help.
I think it's up to you to add the feature to binman, but also believe we should not and don't have to impose always using binman for this purpose on everyone, thinking of various situations that users may have.
I am reading TLOTR at present so this came to mind...
https://www.youtube.com/watch?v=wcASKPX1Ot8
What I would like is a simple way to build an image automatically, with as few steps as possible and have it work. For U-Boot we are trying to use binman internally, but binman can also be used for the 'final step'.
See this talk. There is also a video somewhere.
https://2019.osfc.io/talks/binman-a-data-controlled-firmware-packer-for-u-bo...
Let's try to collaborate on the firmware packaging. It is the wild west at present and the series you have here shows some of the challenges.
Regards, Simon
On Thu, Nov 04, 2021 at 09:11:51AM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 19:49, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:57:48AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:20, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:48PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Now we can use mkeficapsule command instead of EDK-II's script to create a signed capsule file. So update the instruction for capsule authentication.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index f17138f5c765..864d61734bee 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which enables updating the U-Boot image using the UEFI firmware management protocol (FMP). The capsules are not passed to the firmware through the UpdateCapsule runtime service. Instead, capsule-on-disk -functionality is used for fetching the capsule from the EFI System -Partition (ESP) by placing the capsule file under the -\EFI\UpdateCapsule directory.
-The directory \EFI\UpdateCapsule is checked for capsules only within the -EFI system partition on the device specified in the active boot option -determined by reference to BootNext variable or BootOrder variable processing. -The active Boot Variable is the variable with highest priority BootNext or -within BootOrder that refers to a device found to be present. Boot variables -in BootOrder but referring to devices not present are ignored when determining -active boot variable. -Before starting a capsule update make sure your capsules are installed in the -correct ESP partition or set BootNext. +functionality is used for fetching capsules from the EFI System +Partition (ESP) by placing capsule files under the directory::
- \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
UEFI specification. In this document, all the file paths are presented with backslashes. (See section 8.5.5 in version 2.9)
Anyhow U-Boot UEFI internally converts the path with slashes.
So do we need to use backslashes in U-Boot and in the docs? Can we use a forward slash instead? I had hoped those days were behind us. The backslash is used for C escapes, after all.
I'd defer to the maintainer, Heinrich here.
+The directory is checked for capsules only within the +EFI system partition on the device specified in the active boot option, +which is determined by BootXXXX variable in BootNext, or if not, the highest +priority one within BootOrder. Any BootXXXX variables referring to devices +not present are ignored when determining the active boot option.
+Please note that capsules will be applied in the alphabetic order of +capsule file names.
+Creating a capsule file +***********************
+A capsule file can be created by using tools/mkeficapsule. +To build this tool, enable::
- CONFIG_TOOLS_MKEFICAPSULE=y
- CONFIG_TOOLS_LIBCRYPTO=y
+Run the following command::
- $ mkeficapsule \
--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>Performing the update
-Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
-If that option is disabled, you'll need to set the OsIndications variable with:: +Put capsule files under the directory mentioned above. +Then, following the UEFI specification, you'll need to set +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED +bit in OsIndications variable with::
=> setenv -e -nv -bs -rt -v OsIndications =0x04-Finally, the capsule update can be initiated either by rebooting the board, -which is the preferred method, or by issuing the following command:: +Since U-boot doesn't currently support SetVariable at runtime, its value +won't be taken over across the reboot. If this is the case, you can skip +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) +set.
- => efidebug capsule disk-update
-**The efidebug command is should only be used during debugging/development.** +Finally, the capsule update can be initiated by rebooting the board.
Enabling Capsule Authentication
@@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature is computed and prepended to the capsule payload at the time of capsule generation. This signature is then verified by using the public key stored as part of the X509 certificate. This certificate is -in the form of an efi signature list (esl) file, which is embedded as -part of U-Boot. +in the form of an efi signature list (esl) file, which is embedded in +a device tree.
The capsule authentication feature can be enabled through the following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y
- CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated -and used by the steps highlighted below:: +and used by the steps highlighted below.
- Install utility commands on your host
* OPENSSL+1. Install utility commands on your host
* openssl * efitools
- Create signing keys and certificate files on your host
+2. Create signing keys and certificate files on your host::
$ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ -keyout CRT.key -out CRT.crt -nodes -days 365 $ cert-to-efi-sig-list CRT.crt CRT.esl
$ openssl x509 -in CRT.crt -out CRT.cer -outform DER$ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem$ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt$ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem-The capsule file can be generated by using the GenerateCapsule.py -script in EDKII::
- $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
<capsule_file_name> --monotonic-count <val> --fw-version \<val> --lsv <val> --guid \e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \--update-image-index <val> --signer-private-cert \/path/to/CRT.pem --trusted-public-cert \/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \<u-boot.bin>-Place the capsule generated in the above step on the EFI System -Partition under the EFI/UpdateCapsule directory
-Testing on QEMU -*************** +3. Run the following command to create and sign the capsule file::
-Currently, support has been added on the QEMU ARM64 virt platform for -updating the U-Boot binary as a raw image when the platform is booted -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this -configuration, the QEMU platform needs to be booted with -'secure=off'. The U-Boot binary placed on the first bank of the NOR -flash at offset 0x0. The U-Boot environment is placed on the second -NOR flash bank at offset 0x4000000.
- $ mkeficapsule --monotonic-count 1 \
--private-key CRT.key \--certificate CRT.crt \--index 1 --instance 0 \[--fit <FIT image> | --raw <raw image>] \<capsule_file_name>-The capsule update feature is enabled with the following configuration -settings:: +4. Insert the signature list into a device tree in the following format::
- CONFIG_MTD=y
- CONFIG_FLASH_CFI_MTD=y
- CONFIG_CMD_MTDPARTS=y
- CONFIG_CMD_DFU=y
- CONFIG_DFU_MTD=y
- CONFIG_PCI_INIT_R=y
- CONFIG_EFI_CAPSULE_ON_DISK=y
- CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
- CONFIG_EFI_CAPSULE_FIRMWARE=y
- CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
{signature {capsule-key = [ <binary of signature list> ];}Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
As I said in the previous version, I don't know yet if binman is the best place. Can you give me a pointer where a similar feature is implemented in binman, please?
See tools/binman/etype/vblock.py
I'll check it out.
...}-In addition, the following config needs to be disabled(QEMU ARM specific)::
- You can do this manually with::
- CONFIG_TFABOOT
- $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
- $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-The capsule file can be generated by using the tools/mkeficapsule::
- where signature.dts looks like::
- $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
&{/} {signature {capsule-key = /incbin/("CRT.esl");};};Ick. I think your tool should just support adding the signature.
# I may misunderstand your point.
The whole purpose of this tool is to create a capsule file. Adding the signature to that file is simply an optional behavior. I don't see any reason that we should have those features in separate tools.
On the other hand, step.4 mentioned above is to add public keys (x509 certificate list or signature list in UEFI terminology) to a device tree. This is a separate step. Clear?
It just seems a pain to create a DT overlay to add the signature.
Pain in what sense? I have provided fdtsig.sh to avoid bothering users.
I hope Binman can help here if you don't want to put it in your tool. I can write something if it would help.
I think it's up to you to add the feature to binman, but also believe we should not and don't have to impose always using binman for this purpose on everyone, thinking of various situations that users may have.
I am reading TLOTR at present so this came to mind...
OK, you don't rob me of my mkeficapsule, but help me :)
-Takahiro Akashi
What I would like is a simple way to build an image automatically, with as few steps as possible and have it work. For U-Boot we are trying to use binman internally, but binman can also be used for the 'final step'.
See this talk. There is also a video somewhere.
https://2019.osfc.io/talks/binman-a-data-controlled-firmware-packer-for-u-bo...
Let's try to collaborate on the firmware packaging. It is the wild west at present and the series you have here shows some of the challenges.
Regards, Simon
Hi Takahiro,
On Thu, 4 Nov 2021 at 21:15, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Nov 04, 2021 at 09:11:51AM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 19:49, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:57:48AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:20, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:48PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > Now we can use mkeficapsule command instead of EDK-II's script > to create a signed capsule file. So update the instruction for > capsule authentication. > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > --- > doc/develop/uefi/uefi.rst | 143 ++++++++++++++++++-------------------- > 1 file changed, 67 insertions(+), 76 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
thoughts below
> > diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst > index f17138f5c765..864d61734bee 100644 > --- a/doc/develop/uefi/uefi.rst > +++ b/doc/develop/uefi/uefi.rst > @@ -284,37 +284,52 @@ Support has been added for the UEFI capsule update feature which > enables updating the U-Boot image using the UEFI firmware management > protocol (FMP). The capsules are not passed to the firmware through > the UpdateCapsule runtime service. Instead, capsule-on-disk > -functionality is used for fetching the capsule from the EFI System > -Partition (ESP) by placing the capsule file under the > -\EFI\UpdateCapsule directory. > - > -The directory \EFI\UpdateCapsule is checked for capsules only within the > -EFI system partition on the device specified in the active boot option > -determined by reference to BootNext variable or BootOrder variable processing. > -The active Boot Variable is the variable with highest priority BootNext or > -within BootOrder that refers to a device found to be present. Boot variables > -in BootOrder but referring to devices not present are ignored when determining > -active boot variable. > -Before starting a capsule update make sure your capsules are installed in the > -correct ESP partition or set BootNext. > +functionality is used for fetching capsules from the EFI System > +Partition (ESP) by placing capsule files under the directory:: > + > + \EFI\UpdateCapsule
Can we use forward slashes please?
What is a backslash, even? DOS? Windows?
UEFI specification. In this document, all the file paths are presented with backslashes. (See section 8.5.5 in version 2.9)
Anyhow U-Boot UEFI internally converts the path with slashes.
So do we need to use backslashes in U-Boot and in the docs? Can we use a forward slash instead? I had hoped those days were behind us. The backslash is used for C escapes, after all.
I'd defer to the maintainer, Heinrich here.
> + > +The directory is checked for capsules only within the > +EFI system partition on the device specified in the active boot option, > +which is determined by BootXXXX variable in BootNext, or if not, the highest > +priority one within BootOrder. Any BootXXXX variables referring to devices > +not present are ignored when determining the active boot option. > + > +Please note that capsules will be applied in the alphabetic order of > +capsule file names. > + > +Creating a capsule file > +*********************** > + > +A capsule file can be created by using tools/mkeficapsule. > +To build this tool, enable:: > + > + CONFIG_TOOLS_MKEFICAPSULE=y > + CONFIG_TOOLS_LIBCRYPTO=y > + > +Run the following command:: > + > + $ mkeficapsule \ > + --index 1 --instance 0 \ > + [--fit <FIT image> | --raw <raw image>] \ > + <capsule_file_name> > > Performing the update > ********************* > > -Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig > -option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable > -check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule. > - > -If that option is disabled, you'll need to set the OsIndications variable with:: > +Put capsule files under the directory mentioned above. > +Then, following the UEFI specification, you'll need to set > +the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED > +bit in OsIndications variable with:: > > => setenv -e -nv -bs -rt -v OsIndications =0x04 > > -Finally, the capsule update can be initiated either by rebooting the board, > -which is the preferred method, or by issuing the following command:: > +Since U-boot doesn't currently support SetVariable at runtime, its value > +won't be taken over across the reboot. If this is the case, you can skip > +this feature check with the Kconfig option (CONFIG_EFI_IGNORE_OSINDICATIONS) > +set. > > - => efidebug capsule disk-update > - > -**The efidebug command is should only be used during debugging/development.** > +Finally, the capsule update can be initiated by rebooting the board. > > Enabling Capsule Authentication > ******************************* > @@ -324,82 +339,58 @@ be updated by verifying the capsule signature. The capsule signature > is computed and prepended to the capsule payload at the time of > capsule generation. This signature is then verified by using the > public key stored as part of the X509 certificate. This certificate is > -in the form of an efi signature list (esl) file, which is embedded as > -part of U-Boot. > +in the form of an efi signature list (esl) file, which is embedded in > +a device tree. > > The capsule authentication feature can be enabled through the > following config, in addition to the configs listed above for capsule > update:: > > CONFIG_EFI_CAPSULE_AUTHENTICATE=y > - CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert> > > The public and private keys used for the signing process are generated > -and used by the steps highlighted below:: > +and used by the steps highlighted below. > > - 1. Install utility commands on your host > - * OPENSSL > +1. Install utility commands on your host > + * openssl > * efitools > > - 2. Create signing keys and certificate files on your host > +2. Create signing keys and certificate files on your host:: > > $ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \ > -keyout CRT.key -out CRT.crt -nodes -days 365 > $ cert-to-efi-sig-list CRT.crt CRT.esl > > - $ openssl x509 -in CRT.crt -out CRT.cer -outform DER > - $ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem > - > - $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt > - $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem > - > -The capsule file can be generated by using the GenerateCapsule.py > -script in EDKII:: > - > - $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \ > - <capsule_file_name> --monotonic-count <val> --fw-version \ > - <val> --lsv <val> --guid \ > - e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \ > - --update-image-index <val> --signer-private-cert \ > - /path/to/CRT.pem --trusted-public-cert \ > - /path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \ > - <u-boot.bin> > - > -Place the capsule generated in the above step on the EFI System > -Partition under the EFI/UpdateCapsule directory > - > -Testing on QEMU > -*************** > +3. Run the following command to create and sign the capsule file:: > > -Currently, support has been added on the QEMU ARM64 virt platform for > -updating the U-Boot binary as a raw image when the platform is booted > -in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this > -configuration, the QEMU platform needs to be booted with > -'secure=off'. The U-Boot binary placed on the first bank of the NOR > -flash at offset 0x0. The U-Boot environment is placed on the second > -NOR flash bank at offset 0x4000000. > + $ mkeficapsule --monotonic-count 1 \ > + --private-key CRT.key \ > + --certificate CRT.crt \ > + --index 1 --instance 0 \ > + [--fit <FIT image> | --raw <raw image>] \ > + <capsule_file_name> > > -The capsule update feature is enabled with the following configuration > -settings:: > +4. Insert the signature list into a device tree in the following format:: > > - CONFIG_MTD=y > - CONFIG_FLASH_CFI_MTD=y > - CONFIG_CMD_MTDPARTS=y > - CONFIG_CMD_DFU=y > - CONFIG_DFU_MTD=y > - CONFIG_PCI_INIT_R=y > - CONFIG_EFI_CAPSULE_ON_DISK=y > - CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y > - CONFIG_EFI_CAPSULE_FIRMWARE=y > - CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y > + { > + signature { > + capsule-key = [ <binary of signature list> ]; > + }
Can you add this feature to binman? A new entry type that takes the capsule key could do it. We need some way of handling this a bit better.
As I said in the previous version, I don't know yet if binman is the best place. Can you give me a pointer where a similar feature is implemented in binman, please?
See tools/binman/etype/vblock.py
I'll check it out.
> + ... > + } > > -In addition, the following config needs to be disabled(QEMU ARM specific):: > + You can do this manually with:: > > - CONFIG_TFABOOT > + $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > + $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo > > -The capsule file can be generated by using the tools/mkeficapsule:: > + where signature.dts looks like:: > > - $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name> > + &{/} { > + signature { > + capsule-key = /incbin/("CRT.esl"); > + }; > + };
Ick. I think your tool should just support adding the signature.
# I may misunderstand your point.
The whole purpose of this tool is to create a capsule file. Adding the signature to that file is simply an optional behavior. I don't see any reason that we should have those features in separate tools.
On the other hand, step.4 mentioned above is to add public keys (x509 certificate list or signature list in UEFI terminology) to a device tree. This is a separate step. Clear?
It just seems a pain to create a DT overlay to add the signature.
Pain in what sense? I have provided fdtsig.sh to avoid bothering users.
I hope Binman can help here if you don't want to put it in your tool. I can write something if it would help.
I think it's up to you to add the feature to binman, but also believe we should not and don't have to impose always using binman for this purpose on everyone, thinking of various situations that users may have.
I am reading TLOTR at present so this came to mind...
OK, you don't rob me of my mkeficapsule, but help me :)
:-) Yes I think binman can call that tool.
- Simon
-Takahiro Akashi
What I would like is a simple way to build an image automatically, with as few steps as possible and have it work. For U-Boot we are trying to use binman internally, but binman can also be used for the 'final step'.
See this talk. There is also a video somewhere.
https://2019.osfc.io/talks/binman-a-data-controlled-firmware-packer-for-u-bo...
Let's try to collaborate on the firmware packaging. It is the wild west at present and the series you have here shows some of the challenges.
Regards, Simon
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- .../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule' + +# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
- @pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config): - """Set up a file system to be used in UEFI capsule test. + """Set up a file system to be used in UEFI capsule and + authentication test.
Args: request: Pytest request object. @@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
+ capsule_auth_enabled = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') + if capsule_auth_enabled: + # Create private key (SIGNER.key) and certificate (SIGNER.crt) + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365' + % data_dir, shell=True) + check_call('cd %s; %scert-to-efi-sig-list SIGNER.crt SIGNER.esl' + % (data_dir, EFITOOLS_PATH), shell=True) + + # Update dtb adding capsule certificate + check_call('cd %s; cp %s/test/py/tests/test_efi_capsule/signature.dts .' + % (data_dir, u_boot_config.source_dir), shell=True) + check_call('cd %s; dtc -@ -I dts -O dtb -o signature.dtbo signature.dts; fdtoverlay -i %s/arch/sandbox/dts/test.dtb -o test_sig.dtb signature.dtbo' + % (data_dir, u_boot_config.build_dir), shell=True) + + # Create *malicious* private key (SIGNER2.key) and certificate + # (SIGNER2.crt) + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER2.key -out SIGNER2.crt -nodes -days 365' + % data_dir, shell=True) + # Create capsule files # two regions: one for u-boot.bin and the other for u-boot.env check_call('cd %s; echo -n u-boot:Old > u-boot.bin.old; echo -n u-boot:New > u-boot.bin.new; echo -n u-boot-env:Old -> u-boot.env.old; echo -n u-boot-env:New > u-boot.env.new' % data_dir, @@ -56,6 +76,15 @@ def efi_capsule_data(request, u_boot_config): check_call('cd %s; %s/tools/mkeficapsule --raw u-boot.bin.new --index 1 Test02' % (data_dir, u_boot_config.build_dir), shell=True) + if capsule_auth_enabled: + # firmware signed with proper key + check_call('cd %s; %s/tools/mkeficapsule --index 1 --monotonic-count 1 --private-key SIGNER.key --certificate SIGNER.crt --raw u-boot.bin.new Test11' % + (data_dir, u_boot_config.build_dir), + shell=True) + # firmware signed with *mal* key + check_call('cd %s; %s/tools/mkeficapsule --index 1 --monotonic-count 1 --private-key SIGNER2.key --certificate SIGNER2.crt --raw u-boot.bin.new Test12' % + (data_dir, u_boot_config.build_dir), + shell=True)
# Create a disk image with EFI system partition check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat %s %s' % diff --git a/test/py/tests/test_efi_capsule/signature.dts b/test/py/tests/test_efi_capsule/signature.dts new file mode 100644 index 000000000000..078cfc76c93c --- /dev/null +++ b/test/py/tests/test_efi_capsule/signature.dts @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; +/plugin/; + +&{/} { + signature { + capsule-key = /incbin/("SIGNER.esl"); + }; +}; diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py new file mode 100644 index 000000000000..e8bfd49e6363 --- /dev/null +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py @@ -0,0 +1,233 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2021, Linaro Limited +# Author: AKASHI Takahiro takahiro.akashi@linaro.org +# +# U-Boot UEFI: Firmware Update (Signed capsule) Test + +""" +This test verifies capsule-on-disk firmware update +with signed capsule files +""" + +import pytest +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('efi_capsule_firmware_raw') +@pytest.mark.buildconfigspec('efi_capsule_authenticate') +@pytest.mark.buildconfigspec('dfu') +@pytest.mark.buildconfigspec('dfu_sf') +@pytest.mark.buildconfigspec('cmd_efidebug') +@pytest.mark.buildconfigspec('cmd_fat') +@pytest.mark.buildconfigspec('cmd_memory') +@pytest.mark.buildconfigspec('cmd_nvedit_efi') +@pytest.mark.buildconfigspec('cmd_sf') +@pytest.mark.slow +class TestEfiCapsuleFirmwareSigned(object): + def test_efi_capsule_auth1( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 1 - Update U-Boot on SPI Flash, raw image format + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is properly signed, the authentication + should pass and the firmware be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 1-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test11' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test11 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test11' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR + '/test_sig.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 1-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test11' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000') + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test11' not in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:New' in ''.join(output) + + def test_efi_capsule_auth2( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 2 - Update U-Boot on SPI Flash, raw image format + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is signed but with an invalid key, + the authentication should fail and the firmware + not be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 2-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test12' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test12 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test12' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR + '/test_sig.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 2-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test12' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000') + + # deleted any way + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test12' not in ''.join(output) + + # TODO: check CapsuleStatus in CapsuleXXXX + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:Old' in ''.join(output) + + def test_efi_capsule_auth3( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 3 - Update U-Boot on SPI Flash, raw image format + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is not signed, the authentication + should fail and the firmware not be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 3-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test02' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test02 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test02' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR + '/test_sig.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 3-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test02' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000') + + # deleted any way + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test02' not in ''.join(output) + + # TODO: check CapsuleStatus in CapsuleXXXX + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:Old' in ''.join(output)
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
.../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule'
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
@pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config):
- """Set up a file system to be used in UEFI capsule test.
"""Set up a file system to be used in UEFI capsule and
authentication test.Args: request: Pytest request object.
@@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
capsule_auth_enabled = u_boot_config.buildconfig.get('config_efi_capsule_authenticate')if capsule_auth_enabled:# Create private key (SIGNER.key) and certificate (SIGNER.crt)check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365'% data_dir, shell=True)
run_and_log()?
Can you split up the args or something to reduce the line length?
check_call('cd %s; %scert-to-efi-sig-list SIGNER.crt SIGNER.esl'% (data_dir, EFITOOLS_PATH), shell=True)
[..]
Regards, Simon
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
.../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule'
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
@pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config):
- """Set up a file system to be used in UEFI capsule test.
"""Set up a file system to be used in UEFI capsule and
authentication test.Args: request: Pytest request object.
@@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
capsule_auth_enabled = u_boot_config.buildconfig.get('config_efi_capsule_authenticate')if capsule_auth_enabled:# Create private key (SIGNER.key) and certificate (SIGNER.crt)check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365'% data_dir, shell=True)run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
Can you split up the args or something to reduce the line length?
I will try to fix them if possible.
-Takahiro Akashi
check_call('cd %s; %scert-to-efi-sig-list SIGNER.crt SIGNER.esl'% (data_dir, EFITOOLS_PATH), shell=True)[..]
Regards, Simon
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
.../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule'
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
@pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config):
- """Set up a file system to be used in UEFI capsule test.
"""Set up a file system to be used in UEFI capsule and
authentication test.Args: request: Pytest request object.
@@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
capsule_auth_enabled = u_boot_config.buildconfig.get('config_efi_capsule_authenticate')if capsule_auth_enabled:# Create private key (SIGNER.key) and certificate (SIGNER.crt)check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365'% data_dir, shell=True)run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
Regards, Simon
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
.../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule'
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
@pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config):
- """Set up a file system to be used in UEFI capsule test.
"""Set up a file system to be used in UEFI capsule and
authentication test.Args: request: Pytest request object.
@@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
capsule_auth_enabled = u_boot_config.buildconfig.get('config_efi_capsule_authenticate')if capsule_auth_enabled:# Create private key (SIGNER.key) and certificate (SIGNER.crt)check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365'% data_dir, shell=True)run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
-Takahiro Akashi
Regards, Simon
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
.../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule'
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
@pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config):
- """Set up a file system to be used in UEFI capsule test.
"""Set up a file system to be used in UEFI capsule and
authentication test.Args: request: Pytest request object.
@@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
capsule_auth_enabled = u_boot_config.buildconfig.get('config_efi_capsule_authenticate')if capsule_auth_enabled:# Create private key (SIGNER.key) and certificate (SIGNER.crt)check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365'% data_dir, shell=True)run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
The test log is supposed to show everything that happened. It does that with other tests and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
Regards, Simon
On Wed, Nov 03, 2021 at 08:49:04PM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Add a couple of test cases against capsule image authentication for capsule-on-disk, where only a signed capsule file with the verified signature will be applied to the system.
Due to the difficulty of embedding a public key (esl file) in U-Boot binary during pytest setup time, all the keys/certificates are pre-created.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
.../py/tests/test_efi_capsule/capsule_defs.py | 5 + test/py/tests/test_efi_capsule/conftest.py | 35 ++- test/py/tests/test_efi_capsule/signature.dts | 10 + .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/py/tests/test_efi_capsule/signature.dts create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py
diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py index 4fd6353c2040..aa9bf5eee3aa 100644 --- a/test/py/tests/test_efi_capsule/capsule_defs.py +++ b/test/py/tests/test_efi_capsule/capsule_defs.py @@ -3,3 +3,8 @@ # Directories CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule'
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and +# you need build a newer version on your own. +# The path must terminate with '/'. +EFITOOLS_PATH = '' diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 6ad5608cd71c..b0e84dec4931 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -10,13 +10,13 @@ import pytest from capsule_defs import *
# -# Fixture for UEFI secure boot test +# Fixture for UEFI capsule test #
@pytest.fixture(scope='session') def efi_capsule_data(request, u_boot_config):
- """Set up a file system to be used in UEFI capsule test.
"""Set up a file system to be used in UEFI capsule and
authentication test.Args: request: Pytest request object.
@@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): check_call('mkdir -p %s' % data_dir, shell=True) check_call('mkdir -p %s' % install_dir, shell=True)
capsule_auth_enabled = u_boot_config.buildconfig.get('config_efi_capsule_authenticate')if capsule_auth_enabled:# Create private key (SIGNER.key) and certificate (SIGNER.crt)check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365'% data_dir, shell=True)run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
Yes.
The test log is supposed to show everything that happened. It does that with other tests
It does? (I don't think so.)
and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
What is missing in general is that confest.py doesn't generate line-by-line trace logs if needed. It's not my test specific.
-Takahiro Akashi
Regards, Simon
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:21, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Wed, Nov 03, 2021 at 08:49:04PM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > Add a couple of test cases against capsule image authentication > for capsule-on-disk, where only a signed capsule file with the verified > signature will be applied to the system. > > Due to the difficulty of embedding a public key (esl file) in U-Boot > binary during pytest setup time, all the keys/certificates are pre-created. > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > --- > .../py/tests/test_efi_capsule/capsule_defs.py | 5 + > test/py/tests/test_efi_capsule/conftest.py | 35 ++- > test/py/tests/test_efi_capsule/signature.dts | 10 + > .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ > 4 files changed, 280 insertions(+), 3 deletions(-) > create mode 100644 test/py/tests/test_efi_capsule/signature.dts > create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py > > diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py > index 4fd6353c2040..aa9bf5eee3aa 100644 > --- a/test/py/tests/test_efi_capsule/capsule_defs.py > +++ b/test/py/tests/test_efi_capsule/capsule_defs.py > @@ -3,3 +3,8 @@ > # Directories > CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' > CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule' > + > +# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and > +# you need build a newer version on your own. > +# The path must terminate with '/'. > +EFITOOLS_PATH = '' > diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py > index 6ad5608cd71c..b0e84dec4931 100644 > --- a/test/py/tests/test_efi_capsule/conftest.py > +++ b/test/py/tests/test_efi_capsule/conftest.py > @@ -10,13 +10,13 @@ import pytest > from capsule_defs import * > > # > -# Fixture for UEFI secure boot test > +# Fixture for UEFI capsule test > # > > - > @pytest.fixture(scope='session') > def efi_capsule_data(request, u_boot_config): > - """Set up a file system to be used in UEFI capsule test. > + """Set up a file system to be used in UEFI capsule and > + authentication test. > > Args: > request: Pytest request object. > @@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): > check_call('mkdir -p %s' % data_dir, shell=True) > check_call('mkdir -p %s' % install_dir, shell=True) > > + capsule_auth_enabled = u_boot_config.buildconfig.get( > + 'config_efi_capsule_authenticate') > + if capsule_auth_enabled: > + # Create private key (SIGNER.key) and certificate (SIGNER.crt) > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365' > + % data_dir, shell=True)
run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
Yes.
The test log is supposed to show everything that happened. It does that with other tests
It does? (I don't think so.)
and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
What is missing in general is that confest.py doesn't generate line-by-line trace logs if needed. It's not my test specific.
Can you try checking test-log.html ?
Here is an example with a vboot test. See the lines with 'openssl' and 'dtc' ? That is what I am talking about.
Do you see this output with the command you are using?
[-] Section: test_vboot[sha1-basic-sha1--None-False-True] TIME: NOW: 2021/11/04 19:52:55.916263
TIME: SINCE-PREV: 0:00:00.429408
TIME: SINCE-START: 0:00:00.429408
[-] Section: test_vboot[sha1-basic-sha1--None-False-True]/Starting U-Boot TIME: NOW: 2021/11/04 19:52:55.916582
TIME: SINCE-PREV: 0:00:00.000319
TIME: SINCE-START: 0:00:00.429727
[-] Stream: console Creating new bloblist size 400 at c000 sandbox_serial serial: pinctrl_select_state_full: uclass_get_device_by_phandle_id: err=-19
U-Boot 2021.10-00200-g458c5ec2f57-dirty (Nov 04 2021 - 19:52:48 -0600)
Model: sandbox DRAM: 128 MiB Core: 246 devices, 88 uclasses, devicetree: board WDT: Not starting gpio-wdt WDT: Not starting wdt@0 MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) Loading Environment from nowhere... OK In: cros-ec-keyb Out: vidconsole Err: vidconsole Model: sandbox SCSI: Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1 Hit any key to stop autoboot: 2 %08%08%08 0 => TIME: NOW: 2021/11/04 19:52:56.023596
TIME: SINCE-PREV: 0:00:00.107014
TIME: SINCE-START: 0:00:00.536741
TIME: SINCE-SECTION: 0:00:00.107114
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/dev.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...................+++++ ...............+++++
TIME: NOW: 2021/11/04 19:52:56.067325
TIME: SINCE-PREV: 0:00:00.043729
TIME: SINCE-START: 0:00:00.580470
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/dev.key -out /tmp/b/sandbox/sha1-basic/dev.crt
TIME: NOW: 2021/11/04 19:52:56.077671
TIME: SINCE-PREV: 0:00:00.010346
TIME: SINCE-START: 0:00:00.590816
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/prod.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...........................+++++ ............+++++
TIME: NOW: 2021/11/04 19:52:56.127578
TIME: SINCE-PREV: 0:00:00.049907
TIME: SINCE-START: 0:00:00.640723
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/prod.key -out /tmp/b/sandbox/sha1-basic/prod.crt
TIME: NOW: 2021/11/04 19:52:56.136682
TIME: SINCE-PREV: 0:00:00.009104
TIME: SINCE-START: 0:00:00.649827
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-kernel.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-kernel.dtb
TIME: NOW: 2021/11/04 19:52:56.142636
TIME: SINCE-PREV: 0:00:00.005954
TIME: SINCE-START: 0:00:00.655781
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-u-boot.dtb /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts:7.10-9.4: Warning (unit_address_vs_reg): /reset@0: node has a unit name, but no reg or ranges property
TIME: NOW: 2021/11/04 19:52:56.147797
Regards, Simon
On Thu, Nov 04, 2021 at 08:02:37PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:21, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Wed, Nov 03, 2021 at 08:49:04PM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote: > Hi Takahiro, > > On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro > takahiro.akashi@linaro.org wrote: > > > > Add a couple of test cases against capsule image authentication > > for capsule-on-disk, where only a signed capsule file with the verified > > signature will be applied to the system. > > > > Due to the difficulty of embedding a public key (esl file) in U-Boot > > binary during pytest setup time, all the keys/certificates are pre-created. > > > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > > --- > > .../py/tests/test_efi_capsule/capsule_defs.py | 5 + > > test/py/tests/test_efi_capsule/conftest.py | 35 ++- > > test/py/tests/test_efi_capsule/signature.dts | 10 + > > .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ > > 4 files changed, 280 insertions(+), 3 deletions(-) > > create mode 100644 test/py/tests/test_efi_capsule/signature.dts > > create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py > > > > diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py > > index 4fd6353c2040..aa9bf5eee3aa 100644 > > --- a/test/py/tests/test_efi_capsule/capsule_defs.py > > +++ b/test/py/tests/test_efi_capsule/capsule_defs.py > > @@ -3,3 +3,8 @@ > > # Directories > > CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' > > CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule' > > + > > +# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and > > +# you need build a newer version on your own. > > +# The path must terminate with '/'. > > +EFITOOLS_PATH = '' > > diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py > > index 6ad5608cd71c..b0e84dec4931 100644 > > --- a/test/py/tests/test_efi_capsule/conftest.py > > +++ b/test/py/tests/test_efi_capsule/conftest.py > > @@ -10,13 +10,13 @@ import pytest > > from capsule_defs import * > > > > # > > -# Fixture for UEFI secure boot test > > +# Fixture for UEFI capsule test > > # > > > > - > > @pytest.fixture(scope='session') > > def efi_capsule_data(request, u_boot_config): > > - """Set up a file system to be used in UEFI capsule test. > > + """Set up a file system to be used in UEFI capsule and > > + authentication test. > > > > Args: > > request: Pytest request object. > > @@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): > > check_call('mkdir -p %s' % data_dir, shell=True) > > check_call('mkdir -p %s' % install_dir, shell=True) > > > > + capsule_auth_enabled = u_boot_config.buildconfig.get( > > + 'config_efi_capsule_authenticate') > > + if capsule_auth_enabled: > > + # Create private key (SIGNER.key) and certificate (SIGNER.crt) > > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365' > > + % data_dir, shell=True) > > run_and_log()?
I have always used this style of coding in this file as well as other my pytests in test/py/tests (filesystem and secure boot).
So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
Yes.
The test log is supposed to show everything that happened. It does that with other tests
It does? (I don't think so.)
and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
What is missing in general is that confest.py doesn't generate line-by-line trace logs if needed. It's not my test specific.
Can you try checking test-log.html ?
Here is an example with a vboot test. See the lines with 'openssl' and 'dtc' ? That is what I am talking about.
Do you see this output with the command you are using?
No. In your case, openssl and dtc are called in a test function, while my tool is invoked as part of fixture in confest.py.
What I requested is that command executions in fixtures be logged as well.
-Takahiro Akashi
[-] Section: test_vboot[sha1-basic-sha1--None-False-True] TIME: NOW: 2021/11/04 19:52:55.916263
TIME: SINCE-PREV: 0:00:00.429408
TIME: SINCE-START: 0:00:00.429408
[-] Section: test_vboot[sha1-basic-sha1--None-False-True]/Starting U-Boot TIME: NOW: 2021/11/04 19:52:55.916582
TIME: SINCE-PREV: 0:00:00.000319
TIME: SINCE-START: 0:00:00.429727
[-] Stream: console Creating new bloblist size 400 at c000 sandbox_serial serial: pinctrl_select_state_full: uclass_get_device_by_phandle_id: err=-19
U-Boot 2021.10-00200-g458c5ec2f57-dirty (Nov 04 2021 - 19:52:48 -0600)
Model: sandbox DRAM: 128 MiB Core: 246 devices, 88 uclasses, devicetree: board WDT: Not starting gpio-wdt WDT: Not starting wdt@0 MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) Loading Environment from nowhere... OK In: cros-ec-keyb Out: vidconsole Err: vidconsole Model: sandbox SCSI: Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1 Hit any key to stop autoboot: 2 %08%08%08 0 => TIME: NOW: 2021/11/04 19:52:56.023596
TIME: SINCE-PREV: 0:00:00.107014
TIME: SINCE-START: 0:00:00.536741
TIME: SINCE-SECTION: 0:00:00.107114
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/dev.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...................+++++ ...............+++++
TIME: NOW: 2021/11/04 19:52:56.067325
TIME: SINCE-PREV: 0:00:00.043729
TIME: SINCE-START: 0:00:00.580470
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/dev.key -out /tmp/b/sandbox/sha1-basic/dev.crt
TIME: NOW: 2021/11/04 19:52:56.077671
TIME: SINCE-PREV: 0:00:00.010346
TIME: SINCE-START: 0:00:00.590816
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/prod.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...........................+++++ ............+++++
TIME: NOW: 2021/11/04 19:52:56.127578
TIME: SINCE-PREV: 0:00:00.049907
TIME: SINCE-START: 0:00:00.640723
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/prod.key -out /tmp/b/sandbox/sha1-basic/prod.crt
TIME: NOW: 2021/11/04 19:52:56.136682
TIME: SINCE-PREV: 0:00:00.009104
TIME: SINCE-START: 0:00:00.649827
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-kernel.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-kernel.dtb
TIME: NOW: 2021/11/04 19:52:56.142636
TIME: SINCE-PREV: 0:00:00.005954
TIME: SINCE-START: 0:00:00.655781
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-u-boot.dtb /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts:7.10-9.4: Warning (unit_address_vs_reg): /reset@0: node has a unit name, but no reg or ranges property
TIME: NOW: 2021/11/04 19:52:56.147797
Regards, Simon
Hi Takahiro,
On Thu, 4 Nov 2021 at 21:24, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Nov 04, 2021 at 08:02:37PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:21, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Wed, Nov 03, 2021 at 08:49:04PM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote: > > Hi Takahiro, > > > > On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro > > takahiro.akashi@linaro.org wrote: > > > > > > Add a couple of test cases against capsule image authentication > > > for capsule-on-disk, where only a signed capsule file with the verified > > > signature will be applied to the system. > > > > > > Due to the difficulty of embedding a public key (esl file) in U-Boot > > > binary during pytest setup time, all the keys/certificates are pre-created. > > > > > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > > > --- > > > .../py/tests/test_efi_capsule/capsule_defs.py | 5 + > > > test/py/tests/test_efi_capsule/conftest.py | 35 ++- > > > test/py/tests/test_efi_capsule/signature.dts | 10 + > > > .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ > > > 4 files changed, 280 insertions(+), 3 deletions(-) > > > create mode 100644 test/py/tests/test_efi_capsule/signature.dts > > > create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py > > > > > > diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py > > > index 4fd6353c2040..aa9bf5eee3aa 100644 > > > --- a/test/py/tests/test_efi_capsule/capsule_defs.py > > > +++ b/test/py/tests/test_efi_capsule/capsule_defs.py > > > @@ -3,3 +3,8 @@ > > > # Directories > > > CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' > > > CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule' > > > + > > > +# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and > > > +# you need build a newer version on your own. > > > +# The path must terminate with '/'. > > > +EFITOOLS_PATH = '' > > > diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py > > > index 6ad5608cd71c..b0e84dec4931 100644 > > > --- a/test/py/tests/test_efi_capsule/conftest.py > > > +++ b/test/py/tests/test_efi_capsule/conftest.py > > > @@ -10,13 +10,13 @@ import pytest > > > from capsule_defs import * > > > > > > # > > > -# Fixture for UEFI secure boot test > > > +# Fixture for UEFI capsule test > > > # > > > > > > - > > > @pytest.fixture(scope='session') > > > def efi_capsule_data(request, u_boot_config): > > > - """Set up a file system to be used in UEFI capsule test. > > > + """Set up a file system to be used in UEFI capsule and > > > + authentication test. > > > > > > Args: > > > request: Pytest request object. > > > @@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): > > > check_call('mkdir -p %s' % data_dir, shell=True) > > > check_call('mkdir -p %s' % install_dir, shell=True) > > > > > > + capsule_auth_enabled = u_boot_config.buildconfig.get( > > > + 'config_efi_capsule_authenticate') > > > + if capsule_auth_enabled: > > > + # Create private key (SIGNER.key) and certificate (SIGNER.crt) > > > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365' > > > + % data_dir, shell=True) > > > > run_and_log()? > > I have always used this style of coding in this file as well as > other my pytests in test/py/tests (filesystem and secure boot). > > So, at least in this patch, I don't want to have mixed styles.
I don't mind about the style.
Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
Yes.
The test log is supposed to show everything that happened. It does that with other tests
It does? (I don't think so.)
and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
What is missing in general is that confest.py doesn't generate line-by-line trace logs if needed. It's not my test specific.
Can you try checking test-log.html ?
Here is an example with a vboot test. See the lines with 'openssl' and 'dtc' ? That is what I am talking about.
Do you see this output with the command you are using?
No. In your case, openssl and dtc are called in a test function, while my tool is invoked as part of fixture in confest.py.
What I requested is that command executions in fixtures be logged as well.
OK, so it isn't possible to call run_and_log() in fixtures?
Regards, Simon
-Takahiro Akashi
[-] Section: test_vboot[sha1-basic-sha1--None-False-True] TIME: NOW: 2021/11/04 19:52:55.916263
TIME: SINCE-PREV: 0:00:00.429408
TIME: SINCE-START: 0:00:00.429408
[-] Section: test_vboot[sha1-basic-sha1--None-False-True]/Starting U-Boot TIME: NOW: 2021/11/04 19:52:55.916582
TIME: SINCE-PREV: 0:00:00.000319
TIME: SINCE-START: 0:00:00.429727
[-] Stream: console Creating new bloblist size 400 at c000 sandbox_serial serial: pinctrl_select_state_full: uclass_get_device_by_phandle_id: err=-19
U-Boot 2021.10-00200-g458c5ec2f57-dirty (Nov 04 2021 - 19:52:48 -0600)
Model: sandbox DRAM: 128 MiB Core: 246 devices, 88 uclasses, devicetree: board WDT: Not starting gpio-wdt WDT: Not starting wdt@0 MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) Loading Environment from nowhere... OK In: cros-ec-keyb Out: vidconsole Err: vidconsole Model: sandbox SCSI: Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1 Hit any key to stop autoboot: 2 %08%08%08 0 => TIME: NOW: 2021/11/04 19:52:56.023596
TIME: SINCE-PREV: 0:00:00.107014
TIME: SINCE-START: 0:00:00.536741
TIME: SINCE-SECTION: 0:00:00.107114
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/dev.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...................+++++ ...............+++++
TIME: NOW: 2021/11/04 19:52:56.067325
TIME: SINCE-PREV: 0:00:00.043729
TIME: SINCE-START: 0:00:00.580470
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/dev.key -out /tmp/b/sandbox/sha1-basic/dev.crt
TIME: NOW: 2021/11/04 19:52:56.077671
TIME: SINCE-PREV: 0:00:00.010346
TIME: SINCE-START: 0:00:00.590816
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/prod.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...........................+++++ ............+++++
TIME: NOW: 2021/11/04 19:52:56.127578
TIME: SINCE-PREV: 0:00:00.049907
TIME: SINCE-START: 0:00:00.640723
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/prod.key -out /tmp/b/sandbox/sha1-basic/prod.crt
TIME: NOW: 2021/11/04 19:52:56.136682
TIME: SINCE-PREV: 0:00:00.009104
TIME: SINCE-START: 0:00:00.649827
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-kernel.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-kernel.dtb
TIME: NOW: 2021/11/04 19:52:56.142636
TIME: SINCE-PREV: 0:00:00.005954
TIME: SINCE-START: 0:00:00.655781
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-u-boot.dtb /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts:7.10-9.4: Warning (unit_address_vs_reg): /reset@0: node has a unit name, but no reg or ranges property
TIME: NOW: 2021/11/04 19:52:56.147797
Regards, Simon
On Fri, Nov 05, 2021 at 10:12:20AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 21:24, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Nov 04, 2021 at 08:02:37PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:21, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Wed, Nov 03, 2021 at 08:49:04PM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote: > Hi Takahiro, > > On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro > takahiro.akashi@linaro.org wrote: > > > > On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote: > > > Hi Takahiro, > > > > > > On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro > > > takahiro.akashi@linaro.org wrote: > > > > > > > > Add a couple of test cases against capsule image authentication > > > > for capsule-on-disk, where only a signed capsule file with the verified > > > > signature will be applied to the system. > > > > > > > > Due to the difficulty of embedding a public key (esl file) in U-Boot > > > > binary during pytest setup time, all the keys/certificates are pre-created. > > > > > > > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > > > > --- > > > > .../py/tests/test_efi_capsule/capsule_defs.py | 5 + > > > > test/py/tests/test_efi_capsule/conftest.py | 35 ++- > > > > test/py/tests/test_efi_capsule/signature.dts | 10 + > > > > .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ > > > > 4 files changed, 280 insertions(+), 3 deletions(-) > > > > create mode 100644 test/py/tests/test_efi_capsule/signature.dts > > > > create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py > > > > > > > > diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py > > > > index 4fd6353c2040..aa9bf5eee3aa 100644 > > > > --- a/test/py/tests/test_efi_capsule/capsule_defs.py > > > > +++ b/test/py/tests/test_efi_capsule/capsule_defs.py > > > > @@ -3,3 +3,8 @@ > > > > # Directories > > > > CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' > > > > CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule' > > > > + > > > > +# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and > > > > +# you need build a newer version on your own. > > > > +# The path must terminate with '/'. > > > > +EFITOOLS_PATH = '' > > > > diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py > > > > index 6ad5608cd71c..b0e84dec4931 100644 > > > > --- a/test/py/tests/test_efi_capsule/conftest.py > > > > +++ b/test/py/tests/test_efi_capsule/conftest.py > > > > @@ -10,13 +10,13 @@ import pytest > > > > from capsule_defs import * > > > > > > > > # > > > > -# Fixture for UEFI secure boot test > > > > +# Fixture for UEFI capsule test > > > > # > > > > > > > > - > > > > @pytest.fixture(scope='session') > > > > def efi_capsule_data(request, u_boot_config): > > > > - """Set up a file system to be used in UEFI capsule test. > > > > + """Set up a file system to be used in UEFI capsule and > > > > + authentication test. > > > > > > > > Args: > > > > request: Pytest request object. > > > > @@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): > > > > check_call('mkdir -p %s' % data_dir, shell=True) > > > > check_call('mkdir -p %s' % install_dir, shell=True) > > > > > > > > + capsule_auth_enabled = u_boot_config.buildconfig.get( > > > > + 'config_efi_capsule_authenticate') > > > > + if capsule_auth_enabled: > > > > + # Create private key (SIGNER.key) and certificate (SIGNER.crt) > > > > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365' > > > > + % data_dir, shell=True) > > > > > > run_and_log()? > > > > I have always used this style of coding in this file as well as > > other my pytests in test/py/tests (filesystem and secure boot). > > > > So, at least in this patch, I don't want to have mixed styles. > > I don't mind about the style. > > Does the command appear in the test log?
I don't think so as it is invoked in conftest.py. If the command fails, the tests will skip, and if it generates a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
Yes.
The test log is supposed to show everything that happened. It does that with other tests
It does? (I don't think so.)
and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
What is missing in general is that confest.py doesn't generate line-by-line trace logs if needed. It's not my test specific.
Can you try checking test-log.html ?
Here is an example with a vboot test. See the lines with 'openssl' and 'dtc' ? That is what I am talking about.
Do you see this output with the command you are using?
No. In your case, openssl and dtc are called in a test function, while my tool is invoked as part of fixture in confest.py.
What I requested is that command executions in fixtures be logged as well.
OK, so it isn't possible to call run_and_log() in fixtures?
My fixtures are declared with the scope "session", and so we can't use u_boot_console which has the scope "function". Any workaround?
-Takahiro Akashi
Regards, Simon
-Takahiro Akashi
[-] Section: test_vboot[sha1-basic-sha1--None-False-True] TIME: NOW: 2021/11/04 19:52:55.916263
TIME: SINCE-PREV: 0:00:00.429408
TIME: SINCE-START: 0:00:00.429408
[-] Section: test_vboot[sha1-basic-sha1--None-False-True]/Starting U-Boot TIME: NOW: 2021/11/04 19:52:55.916582
TIME: SINCE-PREV: 0:00:00.000319
TIME: SINCE-START: 0:00:00.429727
[-] Stream: console Creating new bloblist size 400 at c000 sandbox_serial serial: pinctrl_select_state_full: uclass_get_device_by_phandle_id: err=-19
U-Boot 2021.10-00200-g458c5ec2f57-dirty (Nov 04 2021 - 19:52:48 -0600)
Model: sandbox DRAM: 128 MiB Core: 246 devices, 88 uclasses, devicetree: board WDT: Not starting gpio-wdt WDT: Not starting wdt@0 MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) Loading Environment from nowhere... OK In: cros-ec-keyb Out: vidconsole Err: vidconsole Model: sandbox SCSI: Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1 Hit any key to stop autoboot: 2 %08%08%08 0 => TIME: NOW: 2021/11/04 19:52:56.023596
TIME: SINCE-PREV: 0:00:00.107014
TIME: SINCE-START: 0:00:00.536741
TIME: SINCE-SECTION: 0:00:00.107114
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/dev.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...................+++++ ...............+++++
TIME: NOW: 2021/11/04 19:52:56.067325
TIME: SINCE-PREV: 0:00:00.043729
TIME: SINCE-START: 0:00:00.580470
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/dev.key -out /tmp/b/sandbox/sha1-basic/dev.crt
TIME: NOW: 2021/11/04 19:52:56.077671
TIME: SINCE-PREV: 0:00:00.010346
TIME: SINCE-START: 0:00:00.590816
[-] Stream: openssl +openssl genpkey -algorithm RSA -out /tmp/b/sandbox/sha1-basic/prod.key -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 ...........................+++++ ............+++++
TIME: NOW: 2021/11/04 19:52:56.127578
TIME: SINCE-PREV: 0:00:00.049907
TIME: SINCE-START: 0:00:00.640723
[-] Stream: openssl +openssl req -batch -new -x509 -key /tmp/b/sandbox/sha1-basic/prod.key -out /tmp/b/sandbox/sha1-basic/prod.crt
TIME: NOW: 2021/11/04 19:52:56.136682
TIME: SINCE-PREV: 0:00:00.009104
TIME: SINCE-START: 0:00:00.649827
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-kernel.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-kernel.dtb
TIME: NOW: 2021/11/04 19:52:56.142636
TIME: SINCE-PREV: 0:00:00.005954
TIME: SINCE-START: 0:00:00.655781
[-] Stream: dtc +dtc -I dts -O dtb -i /tmp/b/sandbox/sha1-basic/ /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts -O dtb -o /tmp/b/sandbox/sha1-basic/sandbox-u-boot.dtb /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py/tests/vboot/sandbox-u-boot.dts:7.10-9.4: Warning (unit_address_vs_reg): /reset@0: node has a unit name, but no reg or ranges property
TIME: NOW: 2021/11/04 19:52:56.147797
Regards, Simon
Hi Takahiro,
On Sun, 7 Nov 2021 at 21:15, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Nov 05, 2021 at 10:12:20AM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 21:24, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Nov 04, 2021 at 08:02:37PM -0600, Simon Glass wrote:
Hi Takahiro,
On Thu, 4 Nov 2021 at 19:21, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Wed, Nov 03, 2021 at 08:49:04PM -0600, Simon Glass wrote:
Hi Takahiro,
On Wed, 3 Nov 2021 at 20:04, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Tue, Nov 02, 2021 at 08:58:15AM -0600, Simon Glass wrote: > > Hi Takahiro, > > > > On Thu, 28 Oct 2021 at 23:25, AKASHI Takahiro > > takahiro.akashi@linaro.org wrote: > > > > > > On Thu, Oct 28, 2021 at 09:17:49PM -0600, Simon Glass wrote: > > > > Hi Takahiro, > > > > > > > > On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro > > > > takahiro.akashi@linaro.org wrote: > > > > > > > > > > Add a couple of test cases against capsule image authentication > > > > > for capsule-on-disk, where only a signed capsule file with the verified > > > > > signature will be applied to the system. > > > > > > > > > > Due to the difficulty of embedding a public key (esl file) in U-Boot > > > > > binary during pytest setup time, all the keys/certificates are pre-created. > > > > > > > > > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > > > > > --- > > > > > .../py/tests/test_efi_capsule/capsule_defs.py | 5 + > > > > > test/py/tests/test_efi_capsule/conftest.py | 35 ++- > > > > > test/py/tests/test_efi_capsule/signature.dts | 10 + > > > > > .../test_capsule_firmware_signed.py | 233 ++++++++++++++++++ > > > > > 4 files changed, 280 insertions(+), 3 deletions(-) > > > > > create mode 100644 test/py/tests/test_efi_capsule/signature.dts > > > > > create mode 100644 test/py/tests/test_efi_capsule/test_capsule_firmware_signed.py > > > > > > > > > > diff --git a/test/py/tests/test_efi_capsule/capsule_defs.py b/test/py/tests/test_efi_capsule/capsule_defs.py > > > > > index 4fd6353c2040..aa9bf5eee3aa 100644 > > > > > --- a/test/py/tests/test_efi_capsule/capsule_defs.py > > > > > +++ b/test/py/tests/test_efi_capsule/capsule_defs.py > > > > > @@ -3,3 +3,8 @@ > > > > > # Directories > > > > > CAPSULE_DATA_DIR = '/EFI/CapsuleTestData' > > > > > CAPSULE_INSTALL_DIR = '/EFI/UpdateCapsule' > > > > > + > > > > > +# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and > > > > > +# you need build a newer version on your own. > > > > > +# The path must terminate with '/'. > > > > > +EFITOOLS_PATH = '' > > > > > diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py > > > > > index 6ad5608cd71c..b0e84dec4931 100644 > > > > > --- a/test/py/tests/test_efi_capsule/conftest.py > > > > > +++ b/test/py/tests/test_efi_capsule/conftest.py > > > > > @@ -10,13 +10,13 @@ import pytest > > > > > from capsule_defs import * > > > > > > > > > > # > > > > > -# Fixture for UEFI secure boot test > > > > > +# Fixture for UEFI capsule test > > > > > # > > > > > > > > > > - > > > > > @pytest.fixture(scope='session') > > > > > def efi_capsule_data(request, u_boot_config): > > > > > - """Set up a file system to be used in UEFI capsule test. > > > > > + """Set up a file system to be used in UEFI capsule and > > > > > + authentication test. > > > > > > > > > > Args: > > > > > request: Pytest request object. > > > > > @@ -40,6 +40,26 @@ def efi_capsule_data(request, u_boot_config): > > > > > check_call('mkdir -p %s' % data_dir, shell=True) > > > > > check_call('mkdir -p %s' % install_dir, shell=True) > > > > > > > > > > + capsule_auth_enabled = u_boot_config.buildconfig.get( > > > > > + 'config_efi_capsule_authenticate') > > > > > + if capsule_auth_enabled: > > > > > + # Create private key (SIGNER.key) and certificate (SIGNER.crt) > > > > > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_SIGNER/ -keyout SIGNER.key -out SIGNER.crt -nodes -days 365' > > > > > + % data_dir, shell=True) > > > > > > > > run_and_log()? > > > > > > I have always used this style of coding in this file as well as > > > other my pytests in test/py/tests (filesystem and secure boot). > > > > > > So, at least in this patch, I don't want to have mixed styles. > > > > I don't mind about the style. > > > > Does the command appear in the test log? > > I don't think so as it is invoked in conftest.py. > If the command fails, the tests will skip, and if it generates > a improper signature, the tests will fail.
Well that is what I am getting at. Can you check?
Yes.
The test log is supposed to show everything that happened. It does that with other tests
It does? (I don't think so.)
and I worry that using this function to run things will mean that no one will be able to debug your test in CI.
What is missing in general is that confest.py doesn't generate line-by-line trace logs if needed. It's not my test specific.
Can you try checking test-log.html ?
Here is an example with a vboot test. See the lines with 'openssl' and 'dtc' ? That is what I am talking about.
Do you see this output with the command you are using?
No. In your case, openssl and dtc are called in a test function, while my tool is invoked as part of fixture in confest.py.
What I requested is that command executions in fixtures be logged as well.
OK, so it isn't possible to call run_and_log() in fixtures?
My fixtures are declared with the scope "session", and so we can't use u_boot_console which has the scope "function". Any workaround?
I'm really not sure. It sounds like it might be tricky...
Reviewed-by: Simon Glass sjg@chromium.org
Regards, Simon
The existing options, "--fit" and "--raw," are only used to put a proper GUID in a capsule header, where GUID identifies a particular FMP (Firmware Management Protocol) driver which then would handle the firmware binary in a capsule. In fact, mkeficapsule does the exact same job in creating a capsule file whatever the firmware binary type is.
To prepare for the future extension, the command syntax will be a bit modified to allow users to specify arbitrary GUID for their own FMP driver. OLD: [--fit <image> | --raw <image>] <capsule file> NEW: [--fit | --raw | --guid <guid-string>] <image> <capsule file>
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- doc/develop/uefi/uefi.rst | 4 +- doc/mkeficapsule.1 | 26 +++++++++---- tools/mkeficapsule.c | 78 ++++++++++++++++++++++++++++++--------- 3 files changed, 81 insertions(+), 27 deletions(-)
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 864d61734bee..54fefd76f0f5 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -367,8 +367,8 @@ and used by the steps highlighted below. --private-key CRT.key \ --certificate CRT.crt \ --index 1 --instance 0 \ - [--fit <FIT image> | --raw <raw image>] \ - <capsule_file_name> + [--fit | --raw | --guid <guid-string] \ + <image_blob> <capsule_file_name>
4. Insert the signature list into a device tree in the following format::
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 index 837e09ab451e..312e8a8b3188 100644 --- a/doc/mkeficapsule.1 +++ b/doc/mkeficapsule.1 @@ -5,7 +5,7 @@ mkeficapsule - Generate EFI capsule file for U-Boot
.SH SYNOPSIS .B mkeficapsule -.RB [\fIoptions\fP] " \fIcapsule-file\fP" +.RB [\fIoptions\fP] " \fIimage-blob\fP \fIcapsule-file\fP"
.SH "DESCRIPTION" The @@ -21,7 +21,7 @@ Optionally, a capsule file can be signed with a given private key. In this case, the update will be authenticated by verifying the signature before applying.
-\fBmkeficapsule\fP supports two different format of image files: +\fBmkeficapsule\fP takes any type of image files, including: .TP .I raw image format is a single binary blob of any type of firmware. @@ -33,16 +33,28 @@ is the same as used in the new \fIuImage\fP format and allows for multiple binary blobs in a single capsule file. This type of image file can be generated by \fBmkimage\fP.
+.PP +If you want to use other types than above two, you should explicitly +specify a guid for the FMP driver. + .SH "OPTIONS" -One of \fB--fit\fP or \fB--raw\fP option must be specified. +One of \fB--fit\fP, \fB--raw\fP or \fB--guid\fP option must be specified.
.TP -.BI "-f, --fit \fIfit-image-file\fP" -Specify a FIT image file +.BI "-f, --fit +Indicate that the blob is a FIT image file
.TP -.BI "-r, --raw \fIraw-image-file\fP" -Specify a raw image file +.BI "-r, --raw +Indicate that the blob is a raw image file + +.TP +.BI "-g, --guid \fIguid-string\fP" +Specify guid for image blob type. The format is: + xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx + +The first three elements are in little endian, while the rest +is in big endian.
.TP .BI "-i, --index \fIindex\fP" diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 5541e4bda894..2e61ee196caf 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -14,7 +14,7 @@
#include <sys/stat.h> #include <sys/types.h> - +#include <uuid/uuid.h> #include <linux/kconfig.h> #ifdef CONFIG_TOOLS_LIBCRYPTO #include <openssl/asn1.h> @@ -51,14 +51,15 @@ efi_guid_t efi_guid_image_type_uboot_raw = efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
#ifdef CONFIG_TOOLS_LIBCRYPTO -static const char *opts_short = "f:r:i:I:v:p:c:m:dh"; +static const char *opts_short = "frg:i:I:v:p:c:m:dh"; #else -static const char *opts_short = "f:r:i:I:v:h"; +static const char *opts_short = "frg:i:I:v:h"; #endif
static struct option options[] = { - {"fit", required_argument, NULL, 'f'}, - {"raw", required_argument, NULL, 'r'}, + {"fit", no_argument, NULL, 'f'}, + {"raw", no_argument, NULL, 'r'}, + {"guid", required_argument, NULL, 'g'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'}, #ifdef CONFIG_TOOLS_LIBCRYPTO @@ -73,11 +74,12 @@ static struct option options[] = {
static void print_usage(void) { - printf("Usage: %s [options] <output file>\n" + printf("Usage: %s [options] <image blob> <output file>\n" "Options:\n"
- "\t-f, --fit <fit image> new FIT image file\n" - "\t-r, --raw <raw image> new raw image file\n" + "\t-f, --fit FIT image type\n" + "\t-r, --raw raw image type\n" + "\t-g, --guid <guid string> guid for image blob type\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n" #ifdef CONFIG_TOOLS_LIBCRYPTO @@ -510,6 +512,37 @@ err_1: return -1; }
+/** + * convert_uuid_to_guid() - convert uuid string to guid string + * @buf: String for UUID + * + * UUID and GUID have the same data structure, but their string + * formats are different due to the endianness. See lib/uuid.c. + * Since uuid_parse() can handle only UUID, this function must + * be called to get correct data for GUID when parsing a string. + * + * The correct data will be returned in @buf. + */ +void convert_uuid_to_guid(unsigned char *buf) +{ + unsigned char c; + + c = buf[0]; + buf[0] = buf[3]; + buf[3] = c; + c = buf[1]; + buf[1] = buf[2]; + buf[2] = c; + + c = buf[4]; + buf[4] = buf[5]; + buf[5] = c; + + c = buf[6]; + buf[6] = buf[7]; + buf[7] = c; +} + /** * main - main entry function of mkeficapsule * @argc: Number of arguments @@ -524,14 +557,13 @@ err_1: */ int main(int argc, char **argv) { - char *file; efi_guid_t *guid; + unsigned char uuid_buf[16]; unsigned long index, instance; uint64_t mcount; char *privkey_file, *cert_file; int c, idx;
- file = NULL; guid = NULL; index = 0; instance = 0; @@ -546,21 +578,31 @@ int main(int argc, char **argv)
switch (c) { case 'f': - if (file) { - printf("Image already specified\n"); + if (guid) { + printf("Image type already specified\n"); return -1; } - file = optarg; guid = &efi_guid_image_type_uboot_fit; break; case 'r': - if (file) { - printf("Image already specified\n"); + if (guid) { + printf("Image type already specified\n"); return -1; } - file = optarg; guid = &efi_guid_image_type_uboot_raw; break; + case 'g': + if (guid) { + printf("Image type already specified\n"); + return -1; + } + if (uuid_parse(optarg, uuid_buf)) { + printf("Wrong guid format\n"); + return -1; + } + convert_uuid_to_guid(uuid_buf); + guid = (efi_guid_t *)uuid_buf; + break; case 'i': index = strtoul(optarg, NULL, 0); break; @@ -596,14 +638,14 @@ int main(int argc, char **argv) }
/* check necessary parameters */ - if ((argc != optind + 1) || !file || + if ((argc != optind + 2) || !guid || ((privkey_file && !cert_file) || (!privkey_file && cert_file))) { print_usage(); return -1; }
- if (create_fwbin(argv[optind], file, guid, index, instance, + if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, mcount, privkey_file, cert_file) < 0) { printf("Creating firmware capsule failed\n"); return -1;
Since the syntax of mkeficapsule was changed in the previous commit, we need to modify command line arguments in a pytest script.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- test/py/tests/test_efi_capsule/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index b0e84dec4931..08bcd74dd991 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -70,10 +70,10 @@ def efi_capsule_data(request, u_boot_config): check_call('cd %s; %s/tools/mkimage -f uboot_bin_env.its uboot_bin_env.itb' % (data_dir, u_boot_config.build_dir), shell=True) - check_call('cd %s; %s/tools/mkeficapsule --fit uboot_bin_env.itb --index 1 Test01' % + check_call('cd %s; %s/tools/mkeficapsule --index 1 --fit uboot_bin_env.itb Test01' % (data_dir, u_boot_config.build_dir), shell=True) - check_call('cd %s; %s/tools/mkeficapsule --raw u-boot.bin.new --index 1 Test02' % + check_call('cd %s; %s/tools/mkeficapsule --index 1 --raw u-boot.bin.new Test02' % (data_dir, u_boot_config.build_dir), shell=True) if capsule_auth_enabled:
This test scenario tests a new feature of mkeficapsule, "--guid" option, which allows us to specify FMP driver's guid explicitly at the command line.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- test/py/tests/test_efi_capsule/conftest.py | 3 + .../test_efi_capsule/test_capsule_firmware.py | 67 +++++++++++++++++++ 2 files changed, 70 insertions(+)
diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 08bcd74dd991..b31dea5c1df6 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -76,6 +76,9 @@ def efi_capsule_data(request, u_boot_config): check_call('cd %s; %s/tools/mkeficapsule --index 1 --raw u-boot.bin.new Test02' % (data_dir, u_boot_config.build_dir), shell=True) + check_call('cd %s; %s/tools/mkeficapsule --index 1 --guid E2BB9C06-70E9-4B14-97A3-5A7913176E3F u-boot.bin.new Test03' % + (data_dir, u_boot_config.build_dir), + shell=True) if capsule_auth_enabled: # firmware signed with proper key check_call('cd %s; %s/tools/mkeficapsule --index 1 --monotonic-count 1 --private-key SIGNER.key --certificate SIGNER.crt --raw u-boot.bin.new Test11' % diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 9eeaae27d626..9cc973560fa1 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -247,3 +247,70 @@ class TestEfiCapsuleFirmwareFit(object): 'sf read 4000000 100000 10', 'md.b 4000000 10']) assert 'u-boot:New' in ''.join(output) + + def test_efi_capsule_fw4( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 4 - Test "--guid" option of mkeficapsule + The test scenario is the same as Case 3. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 4-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test03' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test03 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test03' in ''.join(output) + + # reboot + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 4-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test03' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000') + + output = u_boot_console.run_command_list(['efidebug capsule esrt']) + + # ensure that EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID is in the ESRT. + assert 'E2BB9C06-70E9-4B14-97A3-5A7913176E3F' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test03' not in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:New' in ''.join(output)
Before the capsule authentication is supported, this test script works correctly, but with the feature enabled, most tests will fail due to unsigned capsules. So check the results depending on CAPSULE_AUTHENTICATE or not.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- .../test_efi_capsule/test_capsule_firmware.py | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 9cc973560fa1..6e803f699f2f 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -148,6 +148,8 @@ class TestEfiCapsuleFirmwareFit(object):
capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -171,12 +173,18 @@ class TestEfiCapsuleFirmwareFit(object): 'sf probe 0:0', 'sf read 4000000 100000 10', 'md.b 4000000 10']) - assert 'u-boot:New' in ''.join(output) + if capsule_auth: + assert 'u-boot:Old' in ''.join(output) + else: + assert 'u-boot:New' in ''.join(output)
output = u_boot_console.run_command_list([ 'sf read 4000000 150000 10', 'md.b 4000000 10']) - assert 'u-boot-env:New' in ''.join(output) + if capsule_auth: + assert 'u-boot-env:Old' in ''.join(output) + else: + assert 'u-boot-env:New' in ''.join(output)
def test_efi_capsule_fw3( self, u_boot_config, u_boot_console, efi_capsule_data): @@ -215,6 +223,8 @@ class TestEfiCapsuleFirmwareFit(object):
capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -246,7 +256,10 @@ class TestEfiCapsuleFirmwareFit(object): 'sf probe 0:0', 'sf read 4000000 100000 10', 'md.b 4000000 10']) - assert 'u-boot:New' in ''.join(output) + if capsule_auth: + assert 'u-boot:Old' in ''.join(output) + else: + assert 'u-boot:New' in ''.join(output)
def test_efi_capsule_fw4( self, u_boot_config, u_boot_console, efi_capsule_data): @@ -285,6 +298,8 @@ class TestEfiCapsuleFirmwareFit(object):
capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -313,4 +328,7 @@ class TestEfiCapsuleFirmwareFit(object): 'sf probe 0:0', 'sf read 4000000 100000 10', 'md.b 4000000 10']) - assert 'u-boot:New' in ''.join(output) + if capsule_auth: + assert 'u-boot:Old' in ''.join(output) + else: + assert 'u-boot:New' in ''.join(output)
With this script, a public key is added to a device tree blob as the default efi_get_public_key_data() expects.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- MAINTAINERS | 1 + tools/fdtsig.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100755 tools/fdtsig.sh
diff --git a/MAINTAINERS b/MAINTAINERS index 96233f0aad51..2d83d60619c9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -727,6 +727,7 @@ F: cmd/bootefi.c F: cmd/efidebug.c F: cmd/nvedit_efi.c F: tools/efivar.py +F: tools/fdtsig.sh F: tools/file2include.c F: tools/mkeficapsule.c
diff --git a/tools/fdtsig.sh b/tools/fdtsig.sh new file mode 100755 index 000000000000..c2b2a6dc5ec8 --- /dev/null +++ b/tools/fdtsig.sh @@ -0,0 +1,40 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0+ +# +# script to add a certificate (efi-signature-list) to dtb blob + +usage() { + if [ -n "$*" ]; then + echo "ERROR: $*" + fi + echo "Usage: "$(basename $0) " <esl file> <dtb file>" +} + +if [ "$#" -ne 2 ]; then + usage "Arguments missing" + exit 1 +fi + +ESL=$1 +DTB=$2 +NEW_DTB=$(basename $DTB)_tmp +SIG=signature + +cat << 'EOF' > $SIG.dts +/dts-v1/; +/plugin/; + +&{/} { + signature { +EOF +echo "capsule-key = /incbin/("$ESL");" >> $SIG.dts +cat << 'EOF' >> $SIG.dts + }; +}; +EOF + +dtc -@ -I dts -O dtb -o $SIG.dtbo $SIG.dts +fdtoverlay -i $DTB -o $NEW_DTB $SIG.dtbo +mv $NEW_DTB $DTB + +rm $SIG.dts $SIG.dtsn $SIG.dtbo
By specifying CONFIG_EFI_CAPSULE_KEY_PATH, the build process will automatically insert the given key into the device tree. Otherwise, users are required to do so manually, possibly, with the utility script, fdtsig.sh.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- doc/develop/uefi/uefi.rst | 4 ++++ dts/Makefile | 23 +++++++++++++++++++++-- lib/efi_loader/Kconfig | 7 +++++++ 3 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 54fefd76f0f5..7f85b9e5a4a6 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -347,6 +347,7 @@ following config, in addition to the configs listed above for capsule update::
CONFIG_EFI_CAPSULE_AUTHENTICATE=y + CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
The public and private keys used for the signing process are generated and used by the steps highlighted below. @@ -392,6 +393,9 @@ and used by the steps highlighted below. }; };
+ If CONFIG_EFI_CAPSULE_KEY_PATH is specified, the build process will + take care of it for you. + Executing the boot manager ~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/dts/Makefile b/dts/Makefile index cb3111382959..6c5486719ecd 100644 --- a/dts/Makefile +++ b/dts/Makefile @@ -20,11 +20,30 @@ $(obj)/dt-$(SPL_NAME).dtb: dts/dt.dtb $(objtree)/tools/fdtgrep FORCE mkdir -p $(dir $@) $(call if_changed,fdtgrep)
+quiet_cmd_fdtsig = FDTSIG $@ + cmd_fdtsig = \ + cat $< > $@; \ + $(srctree)/tools/fdtsig.sh \ + $(patsubst "%",%,$(CONFIG_EFI_CAPSULE_KEY_PATH)) $@ + +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y) +ifneq ($(patsubst "%",%,$(CONFIG_EFI_CAPSULE_KEY_PATH)),) +DTB_ov := $(obj)/dt.dtb_ov + +$(obj)/dt.dtb_ov: $(DTB) FORCE + $(call if_changed,fdtsig) +else +DTB_ov := $(DTB) +endif +else +DTB_ov := $(DTB) +endif + ifeq ($(CONFIG_OF_DTB_PROPS_REMOVE),y) -$(obj)/dt.dtb: $(DTB) $(objtree)/tools/fdtgrep FORCE +$(obj)/dt.dtb: $(DTB_ov) $(objtree)/tools/fdtgrep FORCE $(call if_changed,fdt_rm_props) else -$(obj)/dt.dtb: $(DTB) FORCE +$(obj)/dt.dtb: $(DTB_ov) FORCE $(call if_changed,shipped) endif
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 52f71c07c991..d12b1e56ae80 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -208,6 +208,13 @@ config EFI_CAPSULE_AUTHENTICATE Select this option if you want to enable capsule authentication
+config EFI_CAPSULE_KEY_PATH + string "Path to .esl cert for capsule authentication" + depends on EFI_CAPSULE_AUTHENTICATE + help + Provide the EFI signature list (esl) certificate used for capsule + authentication + config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y
participants (3)
-
AKASHI Takahiro -
Mark Kettenis -
Simon Glass