From patchwork Tue Feb 21 20:19:10 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Pali_Roh=C3=A1r?= X-Patchwork-Id: 1745916 X-Patchwork-Delegate: sr@denx.de Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.denx.de (client-ip=2a01:238:438b:c500:173d:9f52:ddab:ee01; helo=phobos.denx.de; envelope-from=u-boot-bounces@lists.denx.de; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=flSQklV4; dkim-atps=neutral Received: from phobos.denx.de (phobos.denx.de [IPv6:2a01:238:438b:c500:173d:9f52:ddab:ee01]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4PLrfV0JYwz240n for ; Wed, 22 Feb 2023 07:36:18 +1100 (AEDT) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AE9A285B30; Tue, 21 Feb 2023 21:25:40 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="flSQklV4"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 214DD85B10; Tue, 21 Feb 2023 21:24:04 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on phobos.denx.de X-Spam-Level: X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.2 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 983FF85B2A for ; Tue, 21 Feb 2023 21:22:45 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 38183611F4; Tue, 21 Feb 2023 20:22:36 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 51251C433D2; Tue, 21 Feb 2023 20:22:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1677010955; bh=lkSLKbCzr7dScLJ/I75tEj3q3o0IA7tieTZnrn/c9pc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=flSQklV4svAsCH7iYDarxwx0wqiECIq4boHawYo/Yt3cC5zEAH4x7wCW1E95uupeL AF061tCDBLEcdBb605oL+4FroJsyOrcDq0JPUP/vtKBErPWR0qyP/qImkT7wCNxdqG CIG95MeLjvzzTuOTo5SAHSzYWPizwH3pD1R0RS+UUXeGFwnH8n21WTT9O/uf7+vsBl d0G8ydm/d85fdZjuu8H2EmiG9yl2l0qtZOxrf0hCRlVUndUcKJ1yJwyIbNXcWRCin+ RlwRtkr2xMzKsnHuq5vrP3Y7dUIl2M4Aa6Me+UxeCyZLfhSobjyvhMDJgLmxsdIc67 SQTU9PKVqMEtQ== Received: by pali.im (Postfix) id 0E536AA6; Tue, 21 Feb 2023 21:22:35 +0100 (CET) From: =?utf-8?q?Pali_Roh=C3=A1r?= To: u-boot@lists.denx.de Cc: Stefan Roese , Tony Dinh , Josua Mayer Subject: [PATCH RFC u-boot-mvebu 44/59] tools: kwbimage: Fix invalid secure boot header signature Date: Tue, 21 Feb 2023 21:19:10 +0100 Message-Id: <20230221201925.9644-45-pali@kernel.org> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20230221201925.9644-1-pali@kernel.org> References: <20230221201925.9644-1-pali@kernel.org> MIME-Version: 1.0 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Secure boot header signature is calculated from the image header with zeroed header checksum. Calculation is done in add_secure_header_v1() function. So after calling this function no header member except main_hdr->checksum can be modified. Commit 2b0980c24027 ("tools: kwbimage: Fill the real header size into the main header") broke this requirement as final header size started to be filled into main_hdr->headersz_* members after the add_secure_header_v1() call. Fix this issue by following steps: - Split header size and image data offset into two variables (headersz and *dataoff). - Change image_headersz_v0() and add_binary_header_v1() functions to return real (unaligned) header size instead of image data offset. - On every place use correct variable (headersz or *dataoff) After these steps variable headersz is correctly filled into the main_hdr->headersz_* members and so overwriting them in the end of the image_create_v1() function is not needed anymore. Remove those overwriting which effectively reverts changes in problematic commit without affecting value in main_hdr->headersz_* members and makes secure boot header signature valid again. Fixes: 2b0980c24027 ("tools: kwbimage: Fill the real header size into the main header") Signed-off-by: Pali Rohár --- tools/kwbimage.c | 41 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/tools/kwbimage.c b/tools/kwbimage.c index a8a59c154b9c..da539541742d 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -959,7 +959,7 @@ static size_t image_headersz_v0(int *hasext) *hasext = 1; } - return image_headersz_align(headersz, image_get_bootfrom()); + return headersz; } static void *image_create_v0(size_t *dataoff, struct image_tool_params *params, @@ -972,10 +972,11 @@ static void *image_create_v0(size_t *dataoff, struct image_tool_params *params, int has_ext = 0; /* - * Calculate the size of the header and the size of the + * Calculate the size of the header and the offset of the * payload */ headersz = image_headersz_v0(&has_ext); + *dataoff = image_headersz_align(headersz, image_get_bootfrom()); image = malloc(headersz); if (!image) { @@ -990,7 +991,7 @@ static void *image_create_v0(size_t *dataoff, struct image_tool_params *params, /* Fill in the main header */ main_hdr->blocksize = cpu_to_le32(payloadsz); - main_hdr->srcaddr = cpu_to_le32(headersz); + main_hdr->srcaddr = cpu_to_le32(*dataoff); main_hdr->ext = has_ext; main_hdr->version = 0; main_hdr->destaddr = cpu_to_le32(params->addr); @@ -1013,10 +1014,9 @@ static void *image_create_v0(size_t *dataoff, struct image_tool_params *params, /* * For SATA srcaddr is specified in number of sectors. * This expects the sector size to be 512 bytes. - * Header size is already aligned. */ if (main_hdr->blockid == IBR_HDR_SATA_ID) - main_hdr->srcaddr = cpu_to_le32(headersz / 512); + main_hdr->srcaddr = cpu_to_le32(le32_to_cpu(main_hdr->srcaddr) / 512); /* For PCIe srcaddr is not used and must be set to 0xFFFFFFFF. */ if (main_hdr->blockid == IBR_HDR_PEX_ID) @@ -1050,7 +1050,6 @@ static void *image_create_v0(size_t *dataoff, struct image_tool_params *params, main_hdr->checksum = image_checksum8(image, sizeof(struct main_hdr_v0)); - *dataoff = headersz; return image; } @@ -1064,10 +1063,6 @@ static size_t image_headersz_v1(int *hasext) int cfgi; int ret; - /* - * Calculate the size of the header and the size of the - * payload - */ headersz = sizeof(struct main_hdr_v1); if (image_get_csk_index() >= 0) { @@ -1163,7 +1158,7 @@ static size_t image_headersz_v1(int *hasext) if (count > 0) headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4; - return image_headersz_align(headersz, image_get_bootfrom()); + return headersz; } static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, @@ -1390,7 +1385,6 @@ static void *image_create_v1(size_t *dataoff, struct image_tool_params *params, { struct image_cfg_element *e; struct main_hdr_v1 *main_hdr; - struct opt_hdr_v1 *ohdr; struct register_set_hdr_v1 *register_set_hdr; struct secure_hdr_v1 *secure_hdr = NULL; size_t headersz; @@ -1401,12 +1395,13 @@ static void *image_create_v1(size_t *dataoff, struct image_tool_params *params, uint8_t delay; /* - * Calculate the size of the header and the size of the + * Calculate the size of the header and the offset of the * payload */ headersz = image_headersz_v1(&hasext); if (headersz == 0) return NULL; + *dataoff = image_headersz_align(headersz, image_get_bootfrom()); image = malloc(headersz); if (!image) { @@ -1428,7 +1423,7 @@ static void *image_create_v1(size_t *dataoff, struct image_tool_params *params, main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16; main_hdr->destaddr = cpu_to_le32(params->addr); main_hdr->execaddr = cpu_to_le32(params->ep); - main_hdr->srcaddr = cpu_to_le32(headersz); + main_hdr->srcaddr = cpu_to_le32(*dataoff); main_hdr->ext = hasext; main_hdr->version = 1; main_hdr->blockid = image_get_bootfrom(); @@ -1458,10 +1453,9 @@ static void *image_create_v1(size_t *dataoff, struct image_tool_params *params, /* * For SATA srcaddr is specified in number of sectors. * This expects the sector size to be 512 bytes. - * Header size is already aligned. */ if (main_hdr->blockid == IBR_HDR_SATA_ID) - main_hdr->srcaddr = cpu_to_le32(headersz / 512); + main_hdr->srcaddr = cpu_to_le32(le32_to_cpu(main_hdr->srcaddr) / 512); /* For PCIe srcaddr is not used and must be set to 0xFFFFFFFF. */ if (main_hdr->blockid == IBR_HDR_PEX_ID) @@ -1528,19 +1522,10 @@ static void *image_create_v1(size_t *dataoff, struct image_tool_params *params, &datai, delay); } - if (secure_hdr && add_secure_header_v1(params, ptr + headersz, payloadsz, + if (secure_hdr && add_secure_header_v1(params, ptr + *dataoff, payloadsz, image, headersz, secure_hdr)) return NULL; - *dataoff = headersz; - - /* Fill the real header size without padding into the main header */ - headersz = sizeof(*main_hdr); - for_each_opt_hdr_v1 (ohdr, main_hdr) - headersz += opt_hdr_v1_size(ohdr); - main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF); - main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16; - /* Calculate and set the header checksum */ main_hdr->checksum = image_checksum8(main_hdr, headersz); @@ -1889,7 +1874,7 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd, memcpy((uint8_t *)ptr + dataoff + datasz, &checksum, sizeof(uint32_t)); /* Finally copy the header into the image area */ - memcpy(ptr, image, dataoff); + memcpy(ptr, image, kwbheader_size(image)); free(image); } @@ -2109,6 +2094,8 @@ static int kwbimage_generate(struct image_tool_params *params, exit(EXIT_FAILURE); } + alloc_len = image_headersz_align(alloc_len, image_get_bootfrom()); + free(image_cfg); hdr = malloc(alloc_len);