From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3C3B5C77B7E for ; Mon, 29 May 2023 13:12:50 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 97CF48476C; Mon, 29 May 2023 15:12:43 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.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=linaro.org header.i=@linaro.org header.b="PI57+UYX"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 84FC584775; Mon, 29 May 2023 15:12:42 +0200 (CEST) Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id C19108470F for ; Mon, 29 May 2023 15:12:39 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=rui.silva@linaro.org Received: by mail-wr1-x430.google.com with SMTP id ffacd0b85a97d-30aef0499b6so408440f8f.1 for ; Mon, 29 May 2023 06:12:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1685365959; x=1687957959; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=QEqyr/HA5zZYG/d5ZysNqUGoIkujlhk43yWo7Bm5TtU=; b=PI57+UYXw/nClmVzVTDA7AKG8TlCqI+1TdAXq4NxD1EtGB/9gaEVpzcRp/RY7yAaFo jFtBx0278ERUenj7iAVZYxcxJIse31HZKG9tZSGQwsJB2CTlW34f4/r+9h7z/STjOYka l8xtGGeXVm3EuXKmil8R5vBFpxJ+32uWpPNSuorXgxOkt47JMrEhqPmZd5ZqYZWaUsFR Evj/faZmf7ZkeW5WfEjwbutBDpH4o5br79sk+AKHPuaj/kEPfj61uGds2oTwToY/CyCr PYdFa5VFYhhg/l8f3ssCywJl13fQczaoAShDd1efxhCyX3LCMTY+sruzh42+Fnb7/vEP 59JQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685365959; x=1687957959; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=QEqyr/HA5zZYG/d5ZysNqUGoIkujlhk43yWo7Bm5TtU=; b=Pr+xpUMuUNWj7C2B0j4A54+T4RCuqQ+Mf0mvTw0SPOnmSyi8/KoIp2QjNdGW2N05+y htFAbXHs5GsuwWCfV7wjCsthPKe3ZGQSdJMeEXtVrQCn6cVBt4CWFbAb3uhq4EPnJvpG BcG7QmI5w2CKsHUXWMZWrEIST8+kIywkCpDr6CxsU4HXlHrjBQ63LkKhTyi+Sblm8wkh C3vRHDBfi6m8g1FR+R5H9kCQIskRRaT/Kw7hUP8sSG8+F8F0oJ5sa7Zz/wielSGWlhTA 7JzV0+VtCGfvJtjyv4sSnOazE/Lc0YggyaWqLUYEvOiNIfZA+rDB3PakP3O/b/7zdEp3 OQaQ== X-Gm-Message-State: AC+VfDxVYtt2Ix0S3ZjlljVL08Yif1BOR3oYaoXH/9O3pyg91S2EGAnG 5E5smwYVsAYcpmr3m0o8LjfxtQ== X-Google-Smtp-Source: ACHHUZ4spw+Yyo6OV5TEnXtCcQRZ5t7E7mCX/ytLZ60UqAQtps3q4A62zDhBLcrjKxln9tSECH1g3g== X-Received: by 2002:adf:db4c:0:b0:303:2583:9635 with SMTP id f12-20020adfdb4c000000b0030325839635mr9723535wrj.20.1685365959186; Mon, 29 May 2023 06:12:39 -0700 (PDT) Received: from localhost (a109-49-33-111.cpe.netcabo.pt. [109.49.33.111]) by smtp.gmail.com with ESMTPSA id o27-20020a5d58db000000b0030aec5e020fsm2835502wrf.86.2023.05.29.06.12.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 May 2023 06:12:38 -0700 (PDT) From: Rui Miguel Silva To: Heinrich Schuchardt , Ilias Apalodimas Cc: u-boot@lists.denx.de, Simon Glass , Tom Rini Subject: Re: [PATCH 1/6] fwu_metadata: make sure structures are packed In-Reply-To: References: <20230502131200.2551513-1-rui.silva@linaro.org> <20230502131200.2551513-2-rui.silva@linaro.org> Date: Mon, 29 May 2023 14:12:38 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain 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.8 at phobos.denx.de X-Virus-Status: Clean Hi Heinrich, thanks for the review, Heinrich Schuchardt writes: > On 5/3/23 10:06, Ilias Apalodimas wrote: >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva wrote: >> >> Hi Rui, >>> >>> The fwu metadata in the metadata partitions >>> should/are packed to guarantee that the info is >>> correct in all platforms. Also the size of them >>> are used to calculate the crc32 and that is important >>> to get it right. >>> >>> Signed-off-by: Rui Miguel Silva >>> --- >>> include/fwu_mdata.h | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h >>> index 8fda4f4ac225..c61221a91735 100644 >>> --- a/include/fwu_mdata.h >>> +++ b/include/fwu_mdata.h >>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info { >>> efi_guid_t image_uuid; >>> uint32_t accepted; >>> uint32_t reserved; >>> -}; >>> +} __packed; > > This structure is naturally packed. Why should adding a __packed > attribute make a difference? Agree. > >>> >>> /** >>> * struct fwu_image_entry - information for a particular type of image >>> @@ -38,7 +38,7 @@ struct fwu_image_entry { >>> efi_guid_t image_type_uuid; >>> efi_guid_t location_uuid; >>> struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS]; >>> -}; >>> +} __packed; > > ditto Agree. > >>> >>> /** >>> * struct fwu_mdata - FWU metadata structure for multi-bank updates >>> @@ -62,6 +62,6 @@ struct fwu_mdata { >>> uint32_t previous_active_index; >>> >>> struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; >>> -}; >>> +} __packed; > > Depending on the alignment of efi_guid_t __packed might make a > difference here. But as efi_guid_t is defined as 4 aligned I can't see > an impact here either. but efi_guid_t is defined as 8 aligned, right? Even though, I think that this type of data written to disk/flash without guaranteeing (trusting natural picketing) packed and endianness is never a good idea. Cheers, Rui > > Best regards > > Heinrich > >>> >>> #endif /* _FWU_MDATA_H_ */ >>> -- >>> 2.40.0 >>> >> >> Yep, that's a good catch. The spec defines 'just' the offsets and not >> the C representation, so those should be indeed packed. >> >> Reviewed-by: Ilias Apalodimas