On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> UEFI specification requires that we shall support three type of
> certificates of authenticode in PE image:
> WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
> WIN_CERT_TYPE_PKCS_SIGNED_DATA
> WIN_CERT_TYPE_EFI_PKCS1_15
>
> As EDK2 does, we will support the first two that are pkcs7 SignedData.
>
> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> ---
> lib/efi_loader/efi_image_loader.c | 55 ++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c
> b/lib/efi_loader/efi_image_loader.c
> index 7f35b1e58fe5..a13ba3692ec2 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -375,7 +375,7 @@ bool efi_image_parse(void *efi, size_t len, struct
> efi_image_regions **regp,
> }
>
> /* Return Certificates Table */
> - if (authsz) {
> + if (authsz > 0) {
authsz is unsigned. We should avoid superfluous comparisons with 0.
> if (len < authoff + authsz) {
> debug("%s: Size for auth too large: %u >= %zu\n",
> __func__, authsz, len - authoff);
> @@ -480,8 +480,8 @@ static bool efi_image_authenticate(void *efi, size_t
> efi_size)
> struct pkcs7_message *msg = NULL;
> struct efi_signature_store *db = NULL, *dbx = NULL;
> struct x509_certificate *cert = NULL;
> - void *new_efi = NULL;
> - size_t new_efi_size;
> + void *new_efi = NULL, *auth, *wincerts_end;
> + size_t new_efi_size, auth_size;
> bool ret = false;
>
> if (!efi_secure_boot_enabled())
> @@ -530,20 +530,51 @@ static bool efi_image_authenticate(void *efi, size_t
> efi_size)
> }
>
> /* go through WIN_CERTIFICATE list */
> - for (wincert = wincerts;
> - (void *)wincert < (void *)wincerts + wincerts_len;
> + for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> + (void *)wincert < wincerts_end;
Adding to (void *) is not defined in the C spec (though gcc treats it
according to your intention). Please, use (u8 *) if you want to add byte
lengths.
> wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> - if (wincert->dwLength < sizeof(*wincert)) {
> - debug("%s: dwLength too small: %u < %zu\n",
> - __func__, wincert->dwLength, sizeof(*wincert));
> - goto err;
> + if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
> + break;
> +
> + if (wincert->dwLength <= sizeof(*wincert)) {
> + debug("dwLength too small: %u < %zu\n",
> + wincert->dwLength, sizeof(*wincert));
> + continue;
> }
> - msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> - wincert->dwLength - sizeof(*wincert));
> +
> + debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> + wincert->wCertificateType);
Should this EFI_PRINT()?
Best regards
Heinrich
> +
> + auth = (void *)wincert + sizeof(*wincert);
> + auth_size = wincert->dwLength - sizeof(*wincert);
> + if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> + if (auth + sizeof(efi_guid_t) >= wincerts_end)
> + break;
> +
> + if (auth_size <= sizeof(efi_guid_t)) {
> + debug("dwLength too small: %u < %zu\n",
> + wincert->dwLength, sizeof(*wincert));
> + continue;
> + }
> + if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> + debug("Certificate type not supported: %pUl\n",
> + auth);
> + continue;
> + }
> +
> + auth += sizeof(efi_guid_t);
> + auth_size -= sizeof(efi_guid_t);
> + } else if (wincert->wCertificateType
> + != WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> + debug("Certificate type not supported\n");
> + continue;
> + }
> +
> + msg = pkcs7_parse_message(auth, auth_size);
> if (IS_ERR(msg)) {
> debug("Parsing image's signature failed\n");
> msg = NULL;
> - goto err;
> + continue;
> }
>
> /* try black-list first */
>
