Re: [PATCH] image: Control FIT signature verification at runtime
On 2/27/22 19:29, Andrew Jeffery wrote: On Tue, 15 Feb 2022, at 13:55, Andrew Jeffery wrote: On Tue, 15 Feb 2022, at 13:42, Dhananjay Phadke wrote: On 2/14/2022 3:13 PM, Patrick Williams wrote: On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote: There's a key-requirement policy already implemented [1]. [1] https://lore.kernel.org/u-boot/cover.1597643014.git.thir...@linux.microsoft.com/ Board code can patch "required-policy" = none at runtime based appropriate logic. [...] Isn't this jumper proposal just like the TCG Physical Presence requirements? This is a software implementation and requires a particular hardware design for it to be done right, but it seems to be along the same lines. I'm supporting idea of having control on FIT verification, just pointed that it maybe done by board code by just patching U-Boot control FDT, either the "required-policy" property at /signature or "required" property in individual key nodes. This might separate the logic out in a way that's acceptable to Alex. Let me poke at it. I've thought about this some more and adding support for `required-mode = "none";` or similar seems like a massive footgun given that (as I understand it) the FIT image as a whole isn't verified. Only supporting "all" or "any" seems okay because some verification must succeed in the context of the keys available in the current stage. After some internal discussion this effort has been set aside so I'm not going to pursue it further for the moment. I don't think it's easy to proceed anyway without feedback from Alex. Don't let my thoughts stop you. I don't think there is a perfect way to address this situation, and we don't have to. Code can be changed later. As a general preference, I would like to see a single decision point on whether to verify/skip. It can be changing `required-mode = "none", or any other similar solution. Keep in mind that the FIT is the image you're trying to authenticate. It is completely different from "required-mode", which is part of u-boot's or SPL's embedded dtb. Alex
Re: [PATCH] image: Control FIT signature verification at runtime
On 1/30/22 21:41, Andrew Jeffery wrote:
Some platform designs include support for disabling secure-boot via a
jumper on the board. Sometimes this control can be separate from the
mechanism enabling the root-of-trust for the platform. Add support for
this latter scenario by allowing boards to implement
board_fit_image_require_verfied(), which is then invoked in the usual
FIT verification paths.
Signed-off-by: Andrew Jeffery
---
Hi,
This patch is extracted from and motivated by a series adding run-time
control of FIT signature verification to u-boot in OpenBMC:
https://lore.kernel.org/openbmc/20220131012538.73021-1-and...@aj.id.au/
Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
upstream and contains a bunch of out-of-tree work as well. As such I'm
looking to upstream the couple of changes that make sense against
master.
I don't understand the practical use of a mechanism to disable security
if secure boot was enabled in the first place. Sure it can be
technically done, but why is this not a bad idea?
Please take a look!
Andrew
boot/Kconfig | 8
boot/image-fit.c | 21 +
include/image.h | 9 +
3 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig
index c8d5906cd304..ec413151fd5a 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -78,6 +78,14 @@ config FIT_SIGNATURE
format support in this case, enable it using
CONFIG_LEGACY_IMAGE_FORMAT.
+if FIT_SIGNATURE
+config FIT_RUNTIME_SIGNATURE
+ bool "Control verification of FIT uImages at runtime"
+ help
+ This option allows board support to disable verification of
+ signatures at runtime, for example through the state of a GPIO.
The commit message does a much nicer job explaining this option, even
though it is this kconfig help text that almost all users will ever see.
+endif # FIT_SIGNATURE
+
config FIT_SIGNATURE_MAX_SIZE
hex "Max size of signed FIT structures"
depends on FIT_SIGNATURE
diff --git a/boot/image-fit.c b/boot/image-fit.c
index f01cafe4e277..919dbfa4ee1d 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1308,6 +1308,14 @@ static int fit_image_check_hash(const void *fit, int
noffset, const void *data,
return 0;
}
+#ifndef __weak
+#define __weak
+#endif
What?
+__weak int board_fit_image_require_verified(void)
+{
+ return 1;
+}
+
I caution against having any platform security related functionality as
a weak function. Did I get the right function, or something else with
the same name was compiled that returns 0 and silently disables security
in my platform?
I think a weak function in this context is a source of confusion and
future bugs. You could instead require the symbol to be defined if and
only if the appropriate kconfig is selected. Symbol not defined ->
compiler error. Symbol defined twice -> compiler error. Solves the
issues in the preceding paragraph.
[snip]
diff --git a/include/image.h b/include/image.h
index 97e5f2eb24d6..98900c2e839b 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1173,6 +1173,15 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
# define FIT_IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(FIT_SIGNATURE)
#endif
+/*
+ * Further, allow run-time control of verification, e.g. via a jumper
+ */
+#if defined(CONFIG_FIT_RUNTIME_SIGNATURE)
+# define fit_image_require_verified() board_fit_image_require_verified()
+#else
+# define fit_image_require_verified() FIT_IMAGE_ENABLE_VERIFY
+#endif
+
image.h is also used for host code. When only changing target code
behavior, there should be a very good reason to modify common code. I'm
not convinced that threshold has been met.
My second concern here is subjective: I don't like functions defined as
macros, further depending on config selections. It makes many code
parsers and IDEs poop their pantaloons. It makes u-boot harder to work
with as a result. I suggest finding a way to turn this into a static inline.
Alex
Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n
On 1/11/22 13:09, Tom Rini wrote: On Tue, Jan 11, 2022 at 07:58:05PM +0100, Marek Vasut wrote: On 1/11/22 17:16, Tom Rini wrote: On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote: On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote: The kwbimage has hard dependency on OpenSSL, do not build it in case TOOLS_LIBCRYPTO is disabled. This patch does not work as kwbimage is required for 32-bit Armada platforms. So kwbimage.o cannot be disabled on these platforms. There is already proposal for fixing this issue: https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-p...@kernel.org/ And needs to be respun to not have Kconfig issues. To answer something noted in that thread, yes, it would be good if Kconfig did, or had an option to make WARNING like that fatal. Or is the problem really that no, it's non-optional, really, to have OpenSSL installed? OpenSSL should be optional, I got this bug report where someone tried to build u-boot on ancient debian oldoldstable with openssl 1.1.0 (without ecdsa support), for a platform which doesn't need any of this crypto stuff. So, disabling TOOLS_LIBCRYPTO was a sufficient for that platform to build in that setup (with this patch). And there are plenty of such platforms in the U-Boot tree (all that are not marvell I think). Right. So your patch is a step in the right direction, but we also need to have the appropriate platforms depend'ing on TOOLS_LIBCRYPTO and updating the help to note that some platforms require it as well, to build their images. I did not intend for TOOLS_LIBCRYPTO to be used beyond an on/off toggle for the user. My suggestion is to allow platforms to build irrespective of TOOLS_LIBCRYPTO, and print a warning if for some reason we can't generate a bootable image. I see quite a few ARMv8 platforms throw such warnings on gitlab-ci. We can compile an elf, right? Any good reason why kwbimage should be different? Alex
Re: [PATCH] lib/rsa: avoid -Wdiscarded-qualifiers
On 1/9/22 8:39 AM, Heinrich Schuchardt wrote: The return type of EVP_PKEY_get0_RSA() is const struct rsa_st *. Our code drops the const qualifier leading to In file included from tools/lib/rsa/rsa-sign.c:1: ./tools/../lib/rsa/rsa-sign.c: In function ‘rsa_add_verify_data’: ./tools/../lib/rsa/rsa-sign.c:631:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 631 | rsa = EVP_PKEY_get0_RSA(pkey); | ^ Add a type conversion. Signed-off-by: Heinrich Schuchardt --- lib/rsa/rsa-sign.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c index 44f21416ce..3b6e5f0f86 100644 --- a/lib/rsa/rsa-sign.c +++ b/lib/rsa/rsa-sign.c @@ -628,7 +628,7 @@ int rsa_add_verify_data(struct image_sign_info *info, void *keydest) if (ret) goto err_get_pub_key; - rsa = EVP_PKEY_get0_RSA(pkey); + rsa = (RSA *)EVP_PKEY_get0_RSA(pkey); I think it's the wrong path to discard const qualifiers, whether unwillingly or by type punning. I suggest making 'rsa' a "const RSA *" and fixing the downstream users to do the same. Alex ret = rsa_get_params(rsa, , _inv, , _squared); if (ret) goto err_get_params;
Re: a question about falcon mode
On 11/26/21 4:36 PM, Abder wrote:
Hi Alex,
Just a quick remarque that intrigued me:
Le jeu. 25 nov. 2021 à 15:57, Alex G. a écrit :
On 11/25/21 1:07 AM, Chan Kim wrote:
Hello all,
I'm trying to implement falcon mode for our board. Then should I first
implement the normal mode(spl + proper)?
It looks like so while I'm reading doc/README.falcon. (It says, after
loading kernel, DT etc. I should give 'spl export' command).
Falcon mode is a bit board dependent. There are a couple of ways you
could go about this.
The first is to have an "fdtargs" partition. This is where "spl export"
comes in. Once you run "spl export", it will give a modified dtb at
"$fdtargsaddr". It's that DTB that you need to write to your ftdargs
partition. For example:
> spl export fdt $loadaddr - $fdt_addr_r
> mmc write $fdtargsaddr 0x9800 0x8000
In this example the ftdargs partition starts at sector 0x9800, and is
0x800 sectors long.
The second option is to forget about "spl export" and "fdtargs", and
package your kernel, devicetree, and overlays in a FIT container. You'd
make sure to enable SPL_LOAD_FIT_APPLY_OVERLAY. There isn't much more to
this other than the usual gotcha's with FIT and overlays.
Do you mean by this that the SPL has the capability to generate the
"fdtargs" by it self (if we provide it with the dtb in the fitImage) ?
Form my last experience with the falcon mode, I had a - not sure -
conclusion that the only way to generate the "fdtargs" is by using the
"spl export" command from uboot cmdline !
because the reality of the fdtargs blob, as its name indicates, is not
just the fdt but it has also the bootargs (inside the chosen node )
that are required by the kernel. So if you give only the DTB to the
SPL it will not work - to my knowledge -, cuz the data that will be
passed to the kernel needs to contain also the bootargs !
Can you please confirm to me if this capability is implemented on the
SPL and that we can actually forget about the "spl export" command ?
It might not be obvious that an overlay can contain the "/chosen" node
with the appropriate bootargs:
/dts-v1/;
/plugin/;
/ {
fragment@1 {
target-path = "/chosen";
__overlay__ {
bootargs = "root=blablabla console=ttyeS0";
};
};
};
Thanks
And apologies Chan for jumping on your thread,
Best regards,
--
Abder
Re: a question about falcon mode
On 11/26/21 1:53 AM, Chan Kim wrote: Hi Alex, Thanks for the reply. So I gather that to be able to use 'spl export fdt' to store the 'snapshot' to the storage I should make the spl program runnable at least to that stage. (being able to load kernel image, dtb, initrd and give the spl export command to capture it to storage like SD card). What I'm thinking of is to let another processor(cortext-M7 based scp, system control processor) load the u-boot-spl.bin into on-chip RAM and kernel(including initramfs)and dtb into SDRAM all from SD card (RAW mode, no file system) and let the AP(application processor) start from u-boot-spl.bin. In this scheme u-boot-spl doesn't need to load anything and DRAM is already initialized and it just needs to run from kernel image after very minimal setup (passing kernel arguments). Do you think this is possible or an absurd idea? Any comment will be a big help for me. I'm not sure if that mode is currently supported. You could try using CONFIG_SPL_RAM_SUPPORT=y, and one of two things: a) Package your kernel and DTB in a FIT. b) Extend common/spl/spl_ram.c to support the 'snapshot' dtb Alex Thanks! Chan -Original Message- From: Alex G. Sent: Thursday, November 25, 2021 11:57 PM To: Chan Kim ; U-Boot Mailing List Subject: Re: a question about falcon mode On 11/25/21 1:07 AM, Chan Kim wrote: Hello all, I'm trying to implement falcon mode for our board. Then should I first implement the normal mode(spl + proper)? It looks like so while I'm reading doc/README.falcon. (It says, after loading kernel, DT etc. I should give 'spl export' command). Falcon mode is a bit board dependent. There are a couple of ways you could go about this. The first is to have an "fdtargs" partition. This is where "spl export" comes in. Once you run "spl export", it will give a modified dtb at "$fdtargsaddr". It's that DTB that you need to write to your ftdargs partition. For example: > spl export fdt $loadaddr - $fdt_addr_r > mmc write $fdtargsaddr 0x9800 0x8000 In this example the ftdargs partition starts at sector 0x9800, and is 0x800 sectors long. The second option is to forget about "spl export" and "fdtargs", and package your kernel, devicetree, and overlays in a FIT container. You'd make sure to enable SPL_LOAD_FIT_APPLY_OVERLAY. There isn't much more to this other than the usual gotcha's with FIT and overlays. Alex
Re: a question about falcon mode
On 11/25/21 1:07 AM, Chan Kim wrote: Hello all, I'm trying to implement falcon mode for our board. Then should I first implement the normal mode(spl + proper)? It looks like so while I'm reading doc/README.falcon. (It says, after loading kernel, DT etc. I should give 'spl export' command). Falcon mode is a bit board dependent. There are a couple of ways you could go about this. The first is to have an "fdtargs" partition. This is where "spl export" comes in. Once you run "spl export", it will give a modified dtb at "$fdtargsaddr". It's that DTB that you need to write to your ftdargs partition. For example: > spl export fdt $loadaddr - $fdt_addr_r > mmc write $fdtargsaddr 0x9800 0x8000 In this example the ftdargs partition starts at sector 0x9800, and is 0x800 sectors long. The second option is to forget about "spl export" and "fdtargs", and package your kernel, devicetree, and overlays in a FIT container. You'd make sure to enable SPL_LOAD_FIT_APPLY_OVERLAY. There isn't much more to this other than the usual gotcha's with FIT and overlays. Alex
Re: [Query - 5 Mins] Information on U-BOOT FIT Malloc changes
On 10/25/21 6:55 PM, Spandan Mahadevegowda wrote: Hello Alexandru, I'm currently working on some POC on Pine64 that requires a Trusted Execution Environment. I was using U-Boot 2020.10 with SPL_FIT_GENERATOR and modified mksunxi_fit_atf.sh to accommodate OP-TEE. However, due to new changes in OPTEE and ARM_TF, I was required to update the components and as such had to change to the recent version of U-Boot. I see that from the 2021.01 release and later, all ARM64 devices including sunxi boards were modified to use binman. I did the necessary changes in the latest u-boot to use my tee.bin and generate the u-boot-sunxi-with-spl.bin. However, when I run this on the board, I see that the SPL throws the "alloc space exhausted", "Could not get FIT buffer of 1694208 bytes" errors and I understand that this is due to the change in how SPL now loads images from your commit *03f1f78a9b44b5fd6fc09faf81639879d2d0f85f*. I am not very familiar with FIT and DTB handling, could you guild me with some pointers on how I can fix this? Try increasing CONFIG_SYS_SPL_MALLOC_SIZE. After debugging a little, I found that the BL31 (ARM-TF) does not find any DTB to load BL33 (UBOOT) after the BL31->BL32->BL31 (ARM-TF-> OPTEE -> ARM-TF) flow. In the Configuration, if I put "tee" as the first loadable, the OPTEE loads and does its initialization and gives back control to ARM-TF after which ARM-TF does not know where to find UBOOT. But if I put uboot first under loadable, ARM-TF does not find OPTEE (BL32). Any inputs from your end would greatly help me. Thank you for your time and consideration, -- Best Regards, *Spandan Mahadevegowda*
Re: [PATCH v3 1/4] tools: Separate image types which depend on OpenSSL
On 10/14/21 10:19 PM, Samuel Holland wrote: Some image types (kwbimage and mxsimage) always depend on OpenSSL, so they can only be included in mkimage when TOOLS_LIBCRYPTO is selected. Use Makefile logic to conditionally link the files. When building for platforms which use those image types, automatically select TOOLS_LIBCRYPTO, since it is required for the build to complete. Signed-off-by: Samuel Holland NAK. The intent, as detailed in tools/Makefile, is to _NOT_ to conflate target options with tools options. Disabling openssl libs is purely at the user's discretion. If platforms can't build a usable image, I suggest just printing a loud warning instead of overriding the user. Alex --- Changes in v3: - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best as I can tell, using the suggestions from Pali Rohár) Changes in v2: - Refactored the first patch on top of TOOLS_LIBCRYPTO arch/arm/Kconfig | 3 +++ arch/arm/mach-imx/mxs/Kconfig | 2 ++ scripts/config_whitelist.txt | 1 - tools/Makefile| 19 +-- tools/mxsimage.c | 3 --- 5 files changed, 10 insertions(+), 18 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index d8c041a877..380ad4f670 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -566,6 +566,7 @@ config ARCH_KIRKWOOD select BOARD_EARLY_INIT_F select CPU_ARM926EJS select GPIO_EXTRA_HEADER + select TOOLS_LIBCRYPTO config ARCH_MVEBU bool "Marvell MVEBU family (Armada XP/375/38x/3700/7K/8K)" @@ -580,12 +581,14 @@ config ARCH_MVEBU select OF_CONTROL select OF_SEPARATE select SPI + select TOOLS_LIBCRYPTO imply CMD_DM config ARCH_ORION5X bool "Marvell Orion" select CPU_ARM926EJS select GPIO_EXTRA_HEADER + select TOOLS_LIBCRYPTO config TARGET_STV0991 bool "Support stv0991" diff --git a/arch/arm/mach-imx/mxs/Kconfig b/arch/arm/mach-imx/mxs/Kconfig index b2026a3758..6f138d25e9 100644 --- a/arch/arm/mach-imx/mxs/Kconfig +++ b/arch/arm/mach-imx/mxs/Kconfig @@ -3,6 +3,7 @@ if ARCH_MX23 config MX23 bool default y + select TOOLS_LIBCRYPTO choice prompt "MX23 board select" @@ -34,6 +35,7 @@ if ARCH_MX28 config MX28 bool default y + select TOOLS_LIBCRYPTO choice prompt "MX28 board select" diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt index 3a6865dc70..bea6b6f83b 100644 --- a/scripts/config_whitelist.txt +++ b/scripts/config_whitelist.txt @@ -838,7 +838,6 @@ CONFIG_MXC_UART_BASE CONFIG_MXC_USB_FLAGS CONFIG_MXC_USB_PORT CONFIG_MXC_USB_PORTSC -CONFIG_MXS CONFIG_MXS_AUART CONFIG_MXS_AUART_BASE CONFIG_MXS_OCOTP diff --git a/tools/Makefile b/tools/Makefile index 999fd46531..a9b3d982d8 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto. AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \ aes-encrypt.o aes-decrypt.o) -# Cryptographic helpers that depend on openssl/libcrypto -LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \ - fdt-libcrypto.o) +# Cryptographic helpers and image types that depend on openssl/libcrypto +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \ + lib/fdt-libcrypto.o \ + kwbimage.o \ + mxsimage.o ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o @@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \ imximage.o \ imx8image.o \ imx8mimage.o \ - kwbimage.o \ lib/md5.o \ lpc32xximage.o \ - mxsimage.o \ omapimage.o \ os_support.o \ pblimage.o \ @@ -156,22 +156,13 @@ fit_info-objs := $(dumpimage-mkimage-objs) fit_info.o fit_check_sign-objs := $(dumpimage-mkimage-objs) fit_check_sign.o file2include-objs := file2include.o -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),) -# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register -# the mxsimage support within tools/mxsimage.c . -HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS -endif - ifdef CONFIG_TOOLS_LIBCRYPTO # This affects include/image.h, but including the board config file # is tricky, so manually define this options here. HOST_EXTRACFLAGS += -DCONFIG_FIT_SIGNATURE HOST_EXTRACFLAGS += -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0x HOST_EXTRACFLAGS += -DCONFIG_FIT_CIPHER -endif -# MXSImage needs LibSSL -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
Re: Broken build with disabling OpenSSL crypto
On 10/15/21 3:30 PM, Pali Rohár wrote:
On Friday 15 October 2021 09:35:43 Alex G. wrote:
On 10/15/21 6:34 AM, Pali Rohár wrote:
On Wednesday 06 October 2021 17:05:24 Alex G. wrote:
Hi Jernej,
On 10/6/21 4:27 PM, Jernej Škrabec wrote:
Hi everyone!
Commit cb9faa6f98ae ("tools: Use a single target-independent config to enable
OpenSSL") recently introduced option to disable usage of OpenSSL via
CONFIG_TOOLS_LIBCRYPTO. However, just a bit later, another commit b4f3cc2c42d9
("tools: kwbimage: Do not hide usage of secure header under
CONFIG_ARMADA_38X") made U-Boot tools hard dependent on OpenSSL. That totally
defeats the purpose of first commit. I suggest that it gets reverted.
I would like disable OpenSSL for my usage, since it gives me troubles when
cross-compiling U-Boot inside LibreELEC build system. It's not needed for our
case anyway.
Best regards,
Can you please give the following diff a try, and if it works for you, submit
as patch?
This change is incorrect and will break mvebu builds. mvebu requires
kwbimage for building boot images and so you cannot disable it or make
it optional.
If kwbimage is required and missing the CI builds and tests don't catch
that. I ran buildman with the change, and nothing broke. Sounds like that
needs to be addressed.
It is possible that tests do not covert all scenarios.
That being said, I'm not okay with making everyone a slave to OpenSSL
because of any given platform.
I propose to revert commit b4f3cc2c42d9 ("tools: kwbimage: Do not hide usage
of secure header under CONFIG_ARMADA_38X"), and rework it such that it
doesn't force libcrypto on everyone. And we very likely need a CI test
against libcrypto linkage when TOOLS_LIBCRYPTO is not set.
Reverting that commit is not a solution as it can lead to broken
kwbimage (when crypto stuff is not enabled). Plus there is lot of other
changes and fixes in kwboot and kwbimage...
There are lots of CI tests that do not build usable images. I caution
against conflating testability with usability, as anyone who intends to
run images on hardware will likely have done their diligence and enabled
TOOLS_LIBCRYPTO.
Alex
Some information with another approach how to solve build issues are in
this email:
https://lore.kernel.org/u-boot/20211015114735.rig3e4cuc7mn6a7e@pali/
Alex
diff --git a/tools/Makefile b/tools/Makefile
index 4a86321f64..7f72ff9645 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -96,7 +96,8 @@ AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
# Cryptographic helpers that depend on openssl/libcrypto
LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
- fdt-libcrypto.o)
+ fdt-libcrypto.o) \
+ kwbimage.o
ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
@@ -117,7 +118,6 @@ dumpimage-mkimage-objs := aisimage.o \
imximage.o \
imx8image.o \
imx8mimage.o \
- kwbimage.o \
lib/md5.o \
lpc32xximage.o \
mxsimage.o \
@@ -169,8 +169,8 @@ HOST_EXTRACFLAGS+=
-DCONFIG_FIT_SIGNATURE_MAX_SIZE=0x
HOST_EXTRACFLAGS += -DCONFIG_FIT_CIPHER
endif
-# MXSImage needs LibSSL
-ifneq
($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
+# MXSImage needs LibSSL <- Nope! Read the frogging notice at the top
+ifneq ($(CONFIG_TOOLS_LIBCRYPTO),)
HOSTCFLAGS_kwbimage.o += \
$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
HOSTLDLIBS_mkimage += \
Re: Broken build with disabling OpenSSL crypto
On 10/15/21 6:34 AM, Pali Rohár wrote:
On Wednesday 06 October 2021 17:05:24 Alex G. wrote:
Hi Jernej,
On 10/6/21 4:27 PM, Jernej Škrabec wrote:
Hi everyone!
Commit cb9faa6f98ae ("tools: Use a single target-independent config to enable
OpenSSL") recently introduced option to disable usage of OpenSSL via
CONFIG_TOOLS_LIBCRYPTO. However, just a bit later, another commit b4f3cc2c42d9
("tools: kwbimage: Do not hide usage of secure header under
CONFIG_ARMADA_38X") made U-Boot tools hard dependent on OpenSSL. That totally
defeats the purpose of first commit. I suggest that it gets reverted.
I would like disable OpenSSL for my usage, since it gives me troubles when
cross-compiling U-Boot inside LibreELEC build system. It's not needed for our
case anyway.
Best regards,
Can you please give the following diff a try, and if it works for you, submit
as patch?
This change is incorrect and will break mvebu builds. mvebu requires
kwbimage for building boot images and so you cannot disable it or make
it optional.
If kwbimage is required and missing the CI builds and tests don't catch
that. I ran buildman with the change, and nothing broke. Sounds like
that needs to be addressed.
That being said, I'm not okay with making everyone a slave to OpenSSL
because of any given platform.
I propose to revert commit b4f3cc2c42d9 ("tools: kwbimage: Do not hide
usage of secure header under CONFIG_ARMADA_38X"), and rework it such
that it doesn't force libcrypto on everyone. And we very likely need a
CI test against libcrypto linkage when TOOLS_LIBCRYPTO is not set.
Alex
diff --git a/tools/Makefile b/tools/Makefile
index 4a86321f64..7f72ff9645 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -96,7 +96,8 @@ AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
# Cryptographic helpers that depend on openssl/libcrypto
LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
- fdt-libcrypto.o)
+ fdt-libcrypto.o) \
+ kwbimage.o
ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
@@ -117,7 +118,6 @@ dumpimage-mkimage-objs := aisimage.o \
imximage.o \
imx8image.o \
imx8mimage.o \
- kwbimage.o \
lib/md5.o \
lpc32xximage.o \
mxsimage.o \
@@ -169,8 +169,8 @@ HOST_EXTRACFLAGS+=
-DCONFIG_FIT_SIGNATURE_MAX_SIZE=0x
HOST_EXTRACFLAGS += -DCONFIG_FIT_CIPHER
endif
-# MXSImage needs LibSSL
-ifneq
($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
+# MXSImage needs LibSSL <- Nope! Read the frogging notice at the top
+ifneq ($(CONFIG_TOOLS_LIBCRYPTO),)
HOSTCFLAGS_kwbimage.o += \
$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
HOSTLDLIBS_mkimage += \
Re: using device-tree fragments/overlays
Hi Tim,
On 10/7/21 4:07 PM, Tim Harvey wrote:
Greetings,
I'm working on some features for the imx8mm-venice boards I support
which depend on making small changes to the dt before booting Linux.
The purpose is not to have any of this apply to the U-Boot controlling
dt but instead to the Linux kernel dt applied within ft_board_setup. I
could apply these changes with code but it would be way more readable
if there was a way to store these as fragments in the FIT image and
apply them.
The fragments would be small and have to do with two UART's routed to
a multi-protocol RS232/RS485 transceiver that can be used as either of
the following:
- two tx/rx UART's (the default configuration for my dt's)
- one rs232 uart with CTS/RTS flow control
- one rs485 uart
Here would be an example dt overlay to apply over the default
arch/arm/dts/imx8mm-venice-gw73xx-0x.dts to change it from 2x RS232
UARTS with TX/RX (uart2/uart4) to RS485 half duplex:
/* For RS485 HD:
* - UART4_TX is DE for RS485 transmitter
* - RS485_EN needs to be pulled high
* - RS485_HALF needs to be pulled high
* - RS485_TERM enables on-chip termination
*/
{
rts-gpios = < 29 GPIO_ACTIVE_HIGH>;
rs485-term-gpios = < 0 GPIO_ACTIVE_HIGH>;
};
{
status = "disabled";
};
{
pinctrl-names = "default";
pinctrl-0 = <_hog>;
pinctrl_hog: hoggrp {
fsl,pins = <
MX8MM_IOMUXC_GPIO1_IO00_GPIO1_IO0
0x4104 /* RS485_TERM */
MX8MM_IOMUXC_SAI1_RXFS_GPIO4_IO0
0x4144 /* RS485_EN */
MX8MM_IOMUXC_SAI1_RXD0_GPIO4_IO2
0x4144 /* RS485_HALF */
>;
};
pinctrl_uart2: uart2grp {
fsl,pins = <
MX8MM_IOMUXC_UART2_RXD_UART2_DCE_RX 0x140
MX8MM_IOMUXC_UART2_TXD_UART2_DCE_TX 0x140
MX8MM_IOMUXC_UART4_TXD_GPIO5_IO29 0x140
>;
};
};
Anyone do anything like this before or work with dt fragments/overlays
in U-Boot or have any other suggestions?
I had the exact same use case. A couple of prerequisites:
* Compile kernel DTB with __symbols__ ('-@' argument to dtc)
* Enable FIT devicetree overlays in u-boot
- Slightly different CONFIG_s for loading FITs from u-boot or SPL
* Assemble a FIT with overlays in it
I think there's also a way to do apply overlays from the u-boot
commandline, but I never found that to be intuitive.
You might have to specify a "loadaddr=" for the overlays if you're
loading the FIT from u-boot instead of SPL. AFAIR the u-boot FIT loader
is defective in this regard.
Alex
APPENDIX A: Example FIT ITS
/dts-v1/
;/ {
description = "Flu-boot image with overlays";
#address-cells = <1>;
images {
kernel-1 {
description = "Linux kernel";
data = /incbin/("kernel/zImage");
type = "kernel";
arch = "arm";
os = "linux";
compression = "none";
load = <0xc240>;
entry = <0xc240>;
hash-1 {
algo = "sha256";
};
};
fdt-stm32mp157c-dk2.dtb {
description = "Flattened Device Tree blob";
data = /incbin/("kernel/stm32mp157c-dk2.dtb");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
fdt-dk2-can1-enable.dto {
description = "Flattened Device Tree blob";
data = /incbin/("overlays/dk2-can1-enable.dto");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
};
configurations {
secure-stm32mp157c-dk2.dtb {
description = "Linux for stm32mp157c-dk2.dtb";
kernel = "kernel-1";
fdt = "fdt-stm32mp157c-dk2.dtb",
"fdt-dk2-can1-enable.dto";
hash-1 {
algo = "sha256";
};
};
};
};
Re: Broken build with disabling OpenSSL crypto
On 10/10/21 6:06 AM, Jernej Škrabec wrote: Dne četrtek, 07. oktober 2021 ob 00:05:24 CEST je Alex G. napisal(a): Can you please give the following diff a try, and if it works for you, submit as patch? This works, I'll submit it as a patch. Should I keep you as original author and add your SoB tag? You can take authorship. Alex Best regards, Jernej Alex diff --git a/tools/Makefile b/tools/Makefile index 4a86321f64..7f72ff9645 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -96,7 +96,8 @@ AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \ # Cryptographic helpers that depend on openssl/libcrypto LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \ - fdt-libcrypto.o) + fdt-libcrypto.o) \ + kwbimage.o ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o @@ -117,7 +118,6 @@ dumpimage-mkimage-objs := aisimage.o \ imximage.o \ imx8image.o \ imx8mimage.o \ - kwbimage.o \ lib/md5.o \ lpc32xximage.o \ mxsimage.o \ @@ -169,8 +169,8 @@ HOST_EXTRACFLAGS+= - DCONFIG_FIT_SIGNATURE_MAX_SIZE=0x HOST_EXTRACFLAGS += -DCONFIG_FIT_CIPHER endif -# MXSImage needs LibSSL -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$ (CONFIG_TOOLS_LIBCRYPTO),) +# MXSImage needs LibSSL <- Nope! Read the frogging notice at the top +ifneq ($(CONFIG_TOOLS_LIBCRYPTO),) HOSTCFLAGS_kwbimage.o += \ $(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "") HOSTLDLIBS_mkimage += \
Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()
On 10/7/21 4:04 PM, Tom Rini wrote: On Thu, Oct 07, 2021 at 03:33:32PM -0500, Alex G. wrote: On 10/7/21 2:39 PM, Tom Rini wrote: On Thu, Oct 07, 2021 at 02:32:42PM -0500, Alex G. wrote: On 10/7/21 1:50 PM, Simon Glass wrote: Hi Tom, On Thu, 7 Oct 2021 at 12:30, Tom Rini wrote: On Thu, Oct 07, 2021 at 12:02:24PM -0600, Simon Glass wrote: Hi Tom, On Thu, 7 Oct 2021 at 07:42, Tom Rini wrote: On Thu, Oct 07, 2021 at 07:32:04AM -0600, Simon Glass wrote: Hi Tom, On Wed, 6 Oct 2021 at 20:52, Tom Rini wrote: On Wed, Oct 06, 2021 at 08:49:13PM -0600, Simon Glass wrote: Hi Tom, On Wed, 6 Oct 2021 at 18:26, Tom Rini wrote: On Sat, Sep 25, 2021 at 07:43:15PM -0600, Simon Glass wrote: At present we must separately test for the host build for many options, since we force them to be enabled. For example, CONFIG_FIT is always enabled in the host tools, even if CONFIG_FIT is not enabled by the board itself. It would be more convenient if we could use, for example, CONFIG_IS_ENABLED(FIT) and get CONFIG_HOST_FIT, when building for the host. Add support for this. With this and the tools_build() function, we should be able to remove all the #ifdefs currently needed in code that is build by tools and targets. This will be even nicer when we move to using CONFIG(xxx) everywhere, since all the #ifdef and IS_ENABLED/CONFIG_IS_ENABLED stuff will go away. Signed-off-by: Simon Glass Suggested-by: Rasmus Villemoes # b4f73886 Reviewed-by: Alexandru Gagniuc The problem here is we don't include automatically when building host stuff, I believe. This is why doing this breaks test_mkimage_hashes for me on am335x_evm with: /tmp/.bm-work/am335x_evm/tools/mkimage -D -I dts -O dtb -i /tmp/.bm-work/am335x_evm -f /home/trini/work/u-boot/u-boot/test/py/tests/vboot//hash-images.its /tmp/.bm-work/am335x_evm/test.fit *** stack smashing detected ***: terminated Oh dear, and no CI coverage. I was reluctant to include kconfig.h everywhere but perhaps that is the best approach. Will take a look ASAP. Maybe we need to think a bit harder too about how we structure intentionally shared code. Why not, for example, for these common algorithms, rely on typical system headers/libraries in the tooling, which means we validated U-Boot vs common reference, rather than just our implementations? Do you mean we use openssl for sha1, for example? I guess, yes. Just flat out saying we require openssl for tools, and doing our best to not make compatibility with libressl difficult, seems likely to cause less headaches for people than what we already require in terms of Python. I'm OK with that, although I do think the problem identified here (CONFIG_SHA256 not enabled) is somewhat sideways from that. We already OK, I've taken what you posted on IRC and folded that in, continuing tests now. use separate code paths to run hashing. Perhaps we could make it optional? What about those people that complain about crypto libraries on their systems? I'm not sure how big a problem that really is, currently. I guess one thing would be to make a separate thread on it, and put it in the next -rc email as well, for people to explain why it would be a hardship. That in turn, I think, is coming down to modern vs very old openssl support, rather than having any at all. OK I'll take a look at some point. Or perhaps Alex might like to? We just got a complain about OpenSSL yesterday [1] Alex [1] https://lists.denx.de/pipermail/u-boot/2021-October/462728.html Oh goodness, LibreELC is a custom build system... I'll have to chime in there, thanks. I am in favor of keeping libcrapto separate. We still need our own code for CRC32 and other weak or non-crypto hashes, a tidbit which makes me doubt the wisdom of relying entirely on an external lib. I had to make a similar decision when writing the hashes test. Originally, I was going to use pyCrypto, crcelk, to re-hash everything and compare to mkimage. It turned out to be neither necessarry nor efficient. Is there perhaps a happy medium? Or do we just need to think harder on how to make the code U-Boot needs shared between target and host tools clean and clear and obvious enough? I think hard that's an honorable goal irrespective of the status of libcrypto. libcrypto isolation is a happy side-effect. Alex
Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()
On 10/7/21 2:39 PM, Tom Rini wrote: On Thu, Oct 07, 2021 at 02:32:42PM -0500, Alex G. wrote: On 10/7/21 1:50 PM, Simon Glass wrote: Hi Tom, On Thu, 7 Oct 2021 at 12:30, Tom Rini wrote: On Thu, Oct 07, 2021 at 12:02:24PM -0600, Simon Glass wrote: Hi Tom, On Thu, 7 Oct 2021 at 07:42, Tom Rini wrote: On Thu, Oct 07, 2021 at 07:32:04AM -0600, Simon Glass wrote: Hi Tom, On Wed, 6 Oct 2021 at 20:52, Tom Rini wrote: On Wed, Oct 06, 2021 at 08:49:13PM -0600, Simon Glass wrote: Hi Tom, On Wed, 6 Oct 2021 at 18:26, Tom Rini wrote: On Sat, Sep 25, 2021 at 07:43:15PM -0600, Simon Glass wrote: At present we must separately test for the host build for many options, since we force them to be enabled. For example, CONFIG_FIT is always enabled in the host tools, even if CONFIG_FIT is not enabled by the board itself. It would be more convenient if we could use, for example, CONFIG_IS_ENABLED(FIT) and get CONFIG_HOST_FIT, when building for the host. Add support for this. With this and the tools_build() function, we should be able to remove all the #ifdefs currently needed in code that is build by tools and targets. This will be even nicer when we move to using CONFIG(xxx) everywhere, since all the #ifdef and IS_ENABLED/CONFIG_IS_ENABLED stuff will go away. Signed-off-by: Simon Glass Suggested-by: Rasmus Villemoes # b4f73886 Reviewed-by: Alexandru Gagniuc The problem here is we don't include automatically when building host stuff, I believe. This is why doing this breaks test_mkimage_hashes for me on am335x_evm with: /tmp/.bm-work/am335x_evm/tools/mkimage -D -I dts -O dtb -i /tmp/.bm-work/am335x_evm -f /home/trini/work/u-boot/u-boot/test/py/tests/vboot//hash-images.its /tmp/.bm-work/am335x_evm/test.fit *** stack smashing detected ***: terminated Oh dear, and no CI coverage. I was reluctant to include kconfig.h everywhere but perhaps that is the best approach. Will take a look ASAP. Maybe we need to think a bit harder too about how we structure intentionally shared code. Why not, for example, for these common algorithms, rely on typical system headers/libraries in the tooling, which means we validated U-Boot vs common reference, rather than just our implementations? Do you mean we use openssl for sha1, for example? I guess, yes. Just flat out saying we require openssl for tools, and doing our best to not make compatibility with libressl difficult, seems likely to cause less headaches for people than what we already require in terms of Python. I'm OK with that, although I do think the problem identified here (CONFIG_SHA256 not enabled) is somewhat sideways from that. We already OK, I've taken what you posted on IRC and folded that in, continuing tests now. use separate code paths to run hashing. Perhaps we could make it optional? What about those people that complain about crypto libraries on their systems? I'm not sure how big a problem that really is, currently. I guess one thing would be to make a separate thread on it, and put it in the next -rc email as well, for people to explain why it would be a hardship. That in turn, I think, is coming down to modern vs very old openssl support, rather than having any at all. OK I'll take a look at some point. Or perhaps Alex might like to? We just got a complain about OpenSSL yesterday [1] Alex [1] https://lists.denx.de/pipermail/u-boot/2021-October/462728.html Oh goodness, LibreELC is a custom build system... I'll have to chime in there, thanks. I am in favor of keeping libcrapto separate. We still need our own code for CRC32 and other weak or non-crypto hashes, a tidbit which makes me doubt the wisdom of relying entirely on an external lib. I had to make a similar decision when writing the hashes test. Originally, I was going to use pyCrypto, crcelk, to re-hash everything and compare to mkimage. It turned out to be neither necessarry nor efficient. Alex
Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()
On 10/7/21 1:50 PM, Simon Glass wrote: Hi Tom, On Thu, 7 Oct 2021 at 12:30, Tom Rini wrote: On Thu, Oct 07, 2021 at 12:02:24PM -0600, Simon Glass wrote: Hi Tom, On Thu, 7 Oct 2021 at 07:42, Tom Rini wrote: On Thu, Oct 07, 2021 at 07:32:04AM -0600, Simon Glass wrote: Hi Tom, On Wed, 6 Oct 2021 at 20:52, Tom Rini wrote: On Wed, Oct 06, 2021 at 08:49:13PM -0600, Simon Glass wrote: Hi Tom, On Wed, 6 Oct 2021 at 18:26, Tom Rini wrote: On Sat, Sep 25, 2021 at 07:43:15PM -0600, Simon Glass wrote: At present we must separately test for the host build for many options, since we force them to be enabled. For example, CONFIG_FIT is always enabled in the host tools, even if CONFIG_FIT is not enabled by the board itself. It would be more convenient if we could use, for example, CONFIG_IS_ENABLED(FIT) and get CONFIG_HOST_FIT, when building for the host. Add support for this. With this and the tools_build() function, we should be able to remove all the #ifdefs currently needed in code that is build by tools and targets. This will be even nicer when we move to using CONFIG(xxx) everywhere, since all the #ifdef and IS_ENABLED/CONFIG_IS_ENABLED stuff will go away. Signed-off-by: Simon Glass Suggested-by: Rasmus Villemoes # b4f73886 Reviewed-by: Alexandru Gagniuc The problem here is we don't include automatically when building host stuff, I believe. This is why doing this breaks test_mkimage_hashes for me on am335x_evm with: /tmp/.bm-work/am335x_evm/tools/mkimage -D -I dts -O dtb -i /tmp/.bm-work/am335x_evm -f /home/trini/work/u-boot/u-boot/test/py/tests/vboot//hash-images.its /tmp/.bm-work/am335x_evm/test.fit *** stack smashing detected ***: terminated Oh dear, and no CI coverage. I was reluctant to include kconfig.h everywhere but perhaps that is the best approach. Will take a look ASAP. Maybe we need to think a bit harder too about how we structure intentionally shared code. Why not, for example, for these common algorithms, rely on typical system headers/libraries in the tooling, which means we validated U-Boot vs common reference, rather than just our implementations? Do you mean we use openssl for sha1, for example? I guess, yes. Just flat out saying we require openssl for tools, and doing our best to not make compatibility with libressl difficult, seems likely to cause less headaches for people than what we already require in terms of Python. I'm OK with that, although I do think the problem identified here (CONFIG_SHA256 not enabled) is somewhat sideways from that. We already OK, I've taken what you posted on IRC and folded that in, continuing tests now. use separate code paths to run hashing. Perhaps we could make it optional? What about those people that complain about crypto libraries on their systems? I'm not sure how big a problem that really is, currently. I guess one thing would be to make a separate thread on it, and put it in the next -rc email as well, for people to explain why it would be a hardship. That in turn, I think, is coming down to modern vs very old openssl support, rather than having any at all. OK I'll take a look at some point. Or perhaps Alex might like to? We just got a complain about OpenSSL yesterday [1] Alex [1] https://lists.denx.de/pipermail/u-boot/2021-October/462728.html
Re: [Uboot-stm32] [PATCH v2 03/11] stm32mp1: Add support for falcon mode boot from SD card
On 10/4/21 9:57 AM, Patrick DELAUNAY wrote:
Hi,
=> if OPTEE is loaded after SPL the U-Boot configuration change (running
in secure world or not)
I am starting to work on these issues in the branch
https://github.com/u-boot/u-boot/compare/master...patrickdelaunay:spl_optee_W2140
https://github.com/u-boot/u-boot/commit/04ad553e9c6bee62781460d2952df4962e58ae14
https://github.com/u-boot/u-boot/commit/aebb687a1557590bf070cf5d347854420ca1
But it is still not working, OP-TEE is not correctly started
What do you mean by "OP-TEE is not correctly started". Here's the .its
that I use for my FIT image. I hope this will be helpful.
/dts-v1/
;/ {
description = "U-Boot fitImage for stm32mp1";
#address-cells = <1>;
images {
optee-1 {
description = "OP-TEE secure world firmware";
data = /incbin/("firmware/tee.bin");
type = "tee";
arch = "arm";
os = "tee";
compression = "none";
load = <0xdde4>;
entry = <0xde00>;
hash-1 {
algo = "sha256";
};
};
kernel-1 {
description = "Linux kernel";
data = /incbin/("kernel/zImage");
type = "kernel";
arch = "arm";
os = "linux";
compression = "none";
load = <0xc240>;
entry = <0xc240>;
hash-1 {
algo = "sha256";
};
};
fdt-stm32mp157c-ev1.dtb {
description = "Flattened Device Tree blob";
data = /incbin/("kernel/stm32mp157c-ev1.dtb");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
fdt-stm32mp157c-dk2.dtb {
description = "Flattened Device Tree blob";
data = /incbin/("kernel/stm32mp157c-dk2.dtb");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
fdt-dk2-optee.dto {
description = "Flattened Device Tree blob";
data = /incbin/("firmware/dk2-optee.dto");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
fdt-dk2-can1-enable.dto {
description = "Flattened Device Tree blob";
data = /incbin/("firmware/dk2-can1-enable.dto");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
fdt-bootargs.dto {
description = "Flattened Device Tree blob";
data = /incbin/("firmware/bootargs.dto");
type = "flat_dt";
arch = "arm";
compression = "none";
hash-1 {
algo = "sha256";
};
};
};
configurations {
default = "secure-stm32mp157c-ev1.dtb";
secure-stm32mp157c-ev1.dtb {
description = "Linux with OP-TEE for
stm32mp157c-ev1.dtb";
kernel = "optee-1";
fdt = "fdt-stm32mp157c-ev1.dtb", "fdt-bootargs.dto";
loadables = "kernel-1";
hash-1 {
algo = "sha256";
};
};
secure-stm32mp157c-dk2.dtb {
description = "Linux with OP-TEE for
stm32mp157c-dk2.dtb";
kernel = "optee-1";
fdt = "fdt-stm32mp157c-dk2.dtb", "fdt-bootargs.dto",
"fdt-dk2-can1-enable.dto", "fdt-dk2-optee.dto";
loadables = "kernel-1";
hash-1 {
algo = "sha256";
};
};
};
};
Re: [PATCH 0/3] stm32mp: Attempt to resolve unintended breakage with v2021.10-rc2
Hi Patrick,
On 9/14/21 7:26 AM, Patrick DELAUNAY wrote:
Hi Alexandru,
I think you need to update arch/arm/mach-stm32mp/Kconfig
something like:
config STM32MP15x
bool "Support STMicroelectronics STM32MP15x Soc"
- select ARCH_SUPPORT_PSCI if !TFABOOT
- select ARM_SMCCC if TFABOOT
+ select ARCH_SUPPORT_PSCI if !TFABOOT && !SPL_OPTEE_IMAGE
+ select ARM_SMCCC if TFABOOT || SPL_OPTEE_IMAGE
select CPU_V7A
- select CPU_V7_HAS_NONSEC if !TFABOOT
+ select CPU_V7_HAS_NONSEC if !TFABOOT && !SPL_OPTEE_IMAGE
select CPU_V7_HAS_VIRT
select OF_BOARD_SETUP
select PINCTRL_STM32
@@ -47,8 +47,8 @@ config STM32MP15x
select STM32_SERIAL
select SYS_ARCH_TIMER
imply CMD_NVEDIT_INFO
- imply SYSRESET_PSCI if TFABOOT
- imply SYSRESET_SYSCON if !TFABOOT
+ imply SYSRESET_PSCI if TFABOOT || SPL_OPTEE_IMAGE
+ imply SYSRESET_SYSCON if !TFABOOT && !SPL_OPTEE_IMAGE
help
support of STMicroelectronics SOC STM32MP15x family
STM32MP157, STM32MP153 or STM32MP151
@@ -153,7 +153,7 @@ config NR_DRAM_BANKS
This is a terrible idea. We're trying to answer a few questions:
* Did the FSBL provide a PSCI secure monitor
* Is u-boot running in normal or secure world
But instead of clearly defining those answers, we're trying to infer
them from other config options. This is confusing to start with, but
it's also wrong.
For example, SPL_OPTEE_IMAGE means "we support OPTEE images in SPL". It
doesn't mean that we loaded an OP-TEE kernel at all. SPL with
SPL_OPTEE_IMAGE may as well boot a legacy uboot image -- nothing
prevents it. So to infer from this that u-boot runs in the normal world
is wrong.
Without loss of generality, any CONFIG that conflates u-boot options
with SPL options is likely to cause some changes down the line.
So just introduce CONFIG_TFABOOT_FIP_CONTAINER don't solve all the
issues
I think you need to manage CONFIG_SPL_OPTEE_IMAGE
to handle specific case when OPTEE is running after SPL.
Sure, but I'd have to adjust this at runtime, not in Kconfig for the
reasons above.
I try to experiment the OPTEE load by SPL but I don't see how
the OP-TEE pager can be managed by SPL in the current code.
It must loaded in SYRAM at 0x2ffc, with a risk to overwrite the SPL
code loaded by rom code at 0x2ffc2500.
This consideration is not unique to SPL. I don't have that problem
because SPL loads OP-TEE to DRAM at 0xde00. If OP-TEE wants to load
parts of itself to SYSRAM, that happens after SPL passed control, so the
conflict is not relevant.
or how to manage several binary, see OP-TEE header v2 support in OP-TEE,
Several file it is already supported in TF-A BL2 with FIP:
tee-header_v2.bin
tee-pager_v2.bin
tee-pageable_v2.bin
I don't know how to use use the OP-TEE pager with SPL, so I elected not to:
EXTRA_OEMAKE = "PLATFORM=${OPTEE_PLATFORM} \
CFG_WITH_PAGER=n \
CFG_NS_ENTRY_ADDR=${KERNEL_UIMAGE_LOADADDRESS} \
CROSS_COMPILE=${HOST_PREFIX} \
CFG_TEE_CORE_DEBUG=y \
CFG_TEE_CORE_LOG_LEVEL=2 \
${TZDRAM_FLAGS} \
"
TZDRAM_FLAGS = "CFG_TZDRAM_START= 0xde00\
CFG_DRAM_SIZE=0x2000 "
Alex
Re: Broken build with disabling OpenSSL crypto
Hi Jernej,
On 10/6/21 4:27 PM, Jernej Škrabec wrote:
Hi everyone!
Commit cb9faa6f98ae ("tools: Use a single target-independent config to enable
OpenSSL") recently introduced option to disable usage of OpenSSL via
CONFIG_TOOLS_LIBCRYPTO. However, just a bit later, another commit b4f3cc2c42d9
("tools: kwbimage: Do not hide usage of secure header under
CONFIG_ARMADA_38X") made U-Boot tools hard dependent on OpenSSL. That totally
defeats the purpose of first commit. I suggest that it gets reverted.
I would like disable OpenSSL for my usage, since it gives me troubles when
cross-compiling U-Boot inside LibreELEC build system. It's not needed for our
case anyway.
Best regards,
Can you please give the following diff a try, and if it works for you, submit
as patch?
Alex
diff --git a/tools/Makefile b/tools/Makefile
index 4a86321f64..7f72ff9645 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -96,7 +96,8 @@ AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
# Cryptographic helpers that depend on openssl/libcrypto
LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
- fdt-libcrypto.o)
+ fdt-libcrypto.o) \
+ kwbimage.o
ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
@@ -117,7 +118,6 @@ dumpimage-mkimage-objs := aisimage.o \
imximage.o \
imx8image.o \
imx8mimage.o \
- kwbimage.o \
lib/md5.o \
lpc32xximage.o \
mxsimage.o \
@@ -169,8 +169,8 @@ HOST_EXTRACFLAGS+=
-DCONFIG_FIT_SIGNATURE_MAX_SIZE=0x
HOST_EXTRACFLAGS += -DCONFIG_FIT_CIPHER
endif
-# MXSImage needs LibSSL
-ifneq
($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
+# MXSImage needs LibSSL <- Nope! Read the frogging notice at the top
+ifneq ($(CONFIG_TOOLS_LIBCRYPTO),)
HOSTCFLAGS_kwbimage.o += \
$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
HOSTLDLIBS_mkimage += \
Re: [PATCH] fit: display proper node on error
+ Simon
On 10/6/21 10:47 AM, Angelo Dureghello wrote:
Fix final error message from
Verification failed for '' hash node in 'conf@1' config node
to
Verification failed for 'signature@1' hash node in 'conf@1' config node
Signed-off-by: Angelo Dureghello
---
common/image-fit-sig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index b979cd2a4b..4f2a6ef214 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -166,8 +166,8 @@ static int fit_image_verify_sig(const void *fit, int
image_noffset,
} else {
puts("+ ");
verified = 1;
- break;
}
+ break;
This would stop checking after the first signature- node. It seems
counter-intuitive, as I would expect all signatures to be checked.
In my mind, the 'break;' clause should only happen when
fit_image_check_sig() returns an error. I have no idea why it happened
on success. Simon, any thoughts?
Alex
Re: [PATCH v5 15/29] image: Drop IMAGE_ENABLE_IGNORE
On 9/25/21 8:43 PM, Simon Glass wrote:
We can use the new host_build() function for this, so drop it.
s/host_build/tools_build/
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
(no changes since v1)
common/image-fit.c | 2 +-
include/image.h| 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index 17c6d4e7813..f44f5527b9f 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1277,7 +1277,7 @@ static int fit_image_check_hash(const void *fit, int
noffset, const void *data,
}
printf("%s", algo);
- if (IMAGE_ENABLE_IGNORE) {
+ if (!tools_build()) {
fit_image_hash_get_ignore(fit, noffset, );
if (ignore) {
printf("-skipped ");
diff --git a/include/image.h b/include/image.h
index e190f59232d..a236180ccdd 100644
--- a/include/image.h
+++ b/include/image.h
@@ -27,7 +27,6 @@ struct fdt_region;
#include
#include
-#define IMAGE_ENABLE_IGNORE 0
#define IMAGE_INDENT_STRING ""
#else
@@ -37,8 +36,6 @@ struct fdt_region;
#include
#include
-/* Take notice of the 'ignore' property for hashes */
-#define IMAGE_ENABLE_IGNORE1
#define IMAGE_INDENT_STRING " "
#endif /* USE_HOSTCC */
Re: [PATCH v5 14/29] image: Drop IMAGE_OF_SYSTEM_SETUP
On 9/25/21 8:43 PM, Simon Glass wrote:
This is not needed with Kconfig, since we can use IS_ENABLED() easily
enough. Drop it.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
(no changes since v1)
common/image-fdt.c | 2 +-
include/image.h| 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c
index cd664649e2c..cf87e455230 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -597,7 +597,7 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
}
}
}
- if (IMAGE_OF_SYSTEM_SETUP) {
+ if (IS_ENABLED(CONFIG_OF_SYSTEM_SETUP)) {
fdt_ret = ft_system_setup(blob, gd->bd);
if (fdt_ret) {
printf("ERROR: system-specific fdt fixup failed: %s\n",
diff --git a/include/image.h b/include/image.h
index e1e4148e4c8..e190f59232d 100644
--- a/include/image.h
+++ b/include/image.h
@@ -49,12 +49,6 @@ struct fdt_region;
#include
#endif /* FIT */
-#ifdef CONFIG_OF_SYSTEM_SETUP
-# define IMAGE_OF_SYSTEM_SETUP 1
-#else
-# define IMAGE_OF_SYSTEM_SETUP 0
-#endif
-
extern ulong image_load_addr; /* Default Load Address */
extern ulong image_save_addr; /* Default Save Address */
extern ulong image_save_size; /* Default Save Size */
Re: [PATCH v5 13/29] image: Drop IMAGE_OF_BOARD_SETUP
On 9/25/21 8:43 PM, Simon Glass wrote:
This is not needed with Kconfig, since we can use IS_ENABLED() easily
enough. Drop it.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
(no changes since v1)
common/image-fdt.c | 4 ++--
include/image.h| 6 --
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c
index 9441e63a3d4..cd664649e2c 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -582,7 +582,7 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
/* Append PStore configuration */
fdt_fixup_pstore(blob);
#endif
- if (IMAGE_OF_BOARD_SETUP) {
+ if (IS_ENABLED(CONFIG_OF_BOARD_SETUP)) {
const char *skip_board_fixup;
skip_board_fixup = env_get("skip_board_fixup");
@@ -629,7 +629,7 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
goto err;
#if defined(CONFIG_SOC_KEYSTONE)
- if (IMAGE_OF_BOARD_SETUP)
+ if (IS_ENABLED(CONFIG_OF_BOARD_SETUP))
ft_board_setup_ex(blob, gd->bd);
#endif
diff --git a/include/image.h b/include/image.h
index 00a80999584..e1e4148e4c8 100644
--- a/include/image.h
+++ b/include/image.h
@@ -49,12 +49,6 @@ struct fdt_region;
#include
#endif /* FIT */
-#ifdef CONFIG_OF_BOARD_SETUP
-# define IMAGE_OF_BOARD_SETUP 1
-#else
-# define IMAGE_OF_BOARD_SETUP 0
-#endif
-
#ifdef CONFIG_OF_SYSTEM_SETUP
# define IMAGE_OF_SYSTEM_SETUP1
#else
Re: [PATCH v5 12/29] image: Drop IMAGE_BOOT_GET_CMDLINE
On 9/25/21 8:43 PM, Simon Glass wrote:
This is not needed with Kconfig, since we can use IS_ENABLED() easily
enough and the board code is now in a separate file. Update the only place
where this is used and drop it.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Update commit message to indicate that the code is in a separate file
common/image-board.c | 2 +-
include/image.h | 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/common/image-board.c b/common/image-board.c
index a9a6011ce02..599d6779df5 100644
--- a/common/image-board.c
+++ b/common/image-board.c
@@ -898,7 +898,7 @@ int image_setup_linux(bootm_headers_t *images)
if (CONFIG_IS_ENABLED(OF_LIBFDT))
boot_fdt_add_mem_rsv_regions(lmb, *of_flat_tree);
- if (IMAGE_BOOT_GET_CMDLINE) {
+ if (IS_ENABLED(CONFIG_SYS_BOOT_GET_CMDLINE)) {
This would return 0 on a tools build, right?
ret = boot_get_cmdline(lmb, >cmdline_start,
>cmdline_end);
if (ret) {
diff --git a/include/image.h b/include/image.h
index dc872ef5b24..00a80999584 100644
--- a/include/image.h
+++ b/include/image.h
@@ -49,12 +49,6 @@ struct fdt_region;
#include
#endif /* FIT */
-#ifdef CONFIG_SYS_BOOT_GET_CMDLINE
-# define IMAGE_BOOT_GET_CMDLINE1
-#else
-# define IMAGE_BOOT_GET_CMDLINE0
-#endif
-
#ifdef CONFIG_OF_BOARD_SETUP
# define IMAGE_OF_BOARD_SETUP 1
#else
Re: [PATCH v5 11/29] image: Use the correct checks for CRC32
On 9/25/21 8:43 PM, Simon Glass wrote:
Add a host Kconfig for CRC32. With this we can use CONFIG_IS_ENABLED(CRC32)
directly in the host build, so drop the unnecessary indirection.
Add a few more conditions to SPL_CRC32 to avoid build failures as well as
TPL_CRC32. Also update hash.c to make crc32 optional and to actually take
notice of SPL_CRC32.
Signed-off-by: Simon Glass
I'm not entirely convinced, but it does make CRC32 more consistent with
other hashes.
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Rebase to next
- Use TOOLS_ instead of HOST_
common/hash.c | 13 -
common/spl/Kconfig | 13 -
lib/Kconfig| 5 +
lib/Makefile | 4 +---
tools/Kconfig | 5 +
5 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/common/hash.c b/common/hash.c
index e92f9a9594f..b4b01c193b6 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -178,7 +178,7 @@ static int hash_finish_crc16_ccitt(struct hash_algo *algo,
void *ctx,
return 0;
}
-static int hash_init_crc32(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_crc32(struct hash_algo *algo, void **ctxp)
{
uint32_t *ctx = malloc(sizeof(uint32_t));
*ctx = 0;
@@ -186,15 +186,16 @@ static int hash_init_crc32(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_crc32(struct hash_algo *algo, void *ctx,
-const void *buf, unsigned int size, int is_last)
+static int __maybe_unused hash_update_crc32(struct hash_algo *algo, void *ctx,
+ const void *buf, unsigned int size,
+ int is_last)
{
*((uint32_t *)ctx) = crc32(*((uint32_t *)ctx), buf, size);
return 0;
}
-static int hash_finish_crc32(struct hash_algo *algo, void *ctx, void *dest_buf,
-int size)
+static int __maybe_unused hash_finish_crc32(struct hash_algo *algo, void *ctx,
+ void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -311,6 +312,7 @@ static struct hash_algo hash_algo[] = {
.hash_update= hash_update_crc16_ccitt,
.hash_finish= hash_finish_crc16_ccitt,
},
+#if CONFIG_IS_ENABLED(CRC32)
{
.name = "crc32",
.digest_size= 4,
@@ -320,6 +322,7 @@ static struct hash_algo hash_algo[] = {
.hash_update= hash_update_crc32,
.hash_finish= hash_finish_crc32,
},
+#endif
};
/* Try to minimize code size for boards that don't want much hashing */
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 34f6fc2cfad..91bb6c89fb4 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -419,7 +419,8 @@ config SYS_MMCSD_RAW_MODE_EMMC_BOOT_PARTITION
config SPL_CRC32
bool "Support CRC32"
- default y if SPL_LEGACY_IMAGE_SUPPORT
+ default y if SPL_LEGACY_IMAGE_SUPPORT || SPL_EFI_PARTITION
+ default y if SPL_ENV_SUPPORT || TPL_BLOBLIST
help
Enable this to support CRC32 in uImages or FIT images within SPL.
This is a 32-bit checksum value that can be used to verify images.
@@ -1419,6 +1420,16 @@ config TPL_BOOTROM_SUPPORT
BOOT_DEVICE_BOOTROM (or fall-through to the next boot device in the
boot device list, if not implemented for a given board)
+config TPL_CRC32
+ bool "Support CRC32 in TPL"
+ default y if TPL_ENV_SUPPORT || TPL_BLOBLIST
+ help
+ Enable this to support CRC32 in uImages or FIT images within SPL.
+ This is a 32-bit checksum value that can be used to verify images.
+ For FIT images, this is the least secure type of checksum, suitable
+ for detected accidental image corruption. For secure applications you
+ should consider SHA1 or SHA256.
+
config TPL_DRIVERS_MISC
bool "Support misc drivers in TPL"
help
diff --git a/lib/Kconfig b/lib/Kconfig
index 64765acfa61..70bf8e7a464 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -496,6 +496,11 @@ config SPL_MD5
security applications, but it can be useful for providing a quick
checksum of a block of data.
+config CRC32
+ def_bool y
+ help
+ Enables CRC32 support in U-Boot. This is normally required.
+
config CRC32C
bool
diff --git a/lib/Makefile b/lib/Makefile
index 9c0373e2955..c523bb74119 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -99,9 +99,7 @@ obj-y += display_options.o
CFLAGS_display_options.o := $(if $(BUILD_TAG),-DBUILD_TAG='"$(BUILD_TAG)"')
obj-$(CONFIG_BCH) += bch.o
obj-$(CONFIG_MMC_SPI) += crc7.o
-#ifndef CONFIG_TPL_BUILD
-obj-y += crc32.o
-#endif
+obj-$(CONFIG_$(SPL_TPL_)CRC32) += crc32.o
obj-$(CONFIG_CRC32C) += crc32c.o
obj-y += ctype.o
obj-y += div64.o
diff --git
Re: [PATCH v5 10/29] image: Use Kconfig to enable FIT_RSASSA_PSS on host
On 9/25/21 8:43 PM, Simon Glass wrote:
Add a host Kconfig for FIT_RSASSA_PSS. With this we can use
CONFIG_IS_ENABLED(FIT_RSASSA_PSS) directly in the host build, so drop the
forcing of this in the image.h header.
Drop the #ifdef around padding_pss_verify() too since it is not needed.
Use the compiler to check the config where possible, instead of the
preprocessor.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Avoid preprocessor in a few more places
- Use TOOLS_ instead of HOST_
include/image.h | 3 ---
include/u-boot/rsa.h | 2 --
lib/rsa/rsa-sign.c | 5 ++---
lib/rsa/rsa-verify.c | 16 +++-
tools/Kconfig| 5 +
5 files changed, 10 insertions(+), 21 deletions(-)
Now that's what I'm talking about! deletions > insertions
diff --git a/include/image.h b/include/image.h
index 6efbef06e64..dc872ef5b24 100644
--- a/include/image.h
+++ b/include/image.h
@@ -27,9 +27,6 @@ struct fdt_region;
#include
#include
-/* new uImage format support enabled on host */
-#define CONFIG_FIT_RSASSA_PSS 1
-
#define IMAGE_ENABLE_IGNORE 0
#define IMAGE_INDENT_STRING ""
diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h
index 89a9c4caa0a..7556aa5b4b7 100644
--- a/include/u-boot/rsa.h
+++ b/include/u-boot/rsa.h
@@ -103,11 +103,9 @@ int padding_pkcs_15_verify(struct image_sign_info *info,
uint8_t *msg, int msg_len,
const uint8_t *hash, int hash_len);
-#ifdef CONFIG_FIT_RSASSA_PSS
int padding_pss_verify(struct image_sign_info *info,
uint8_t *msg, int msg_len,
const uint8_t *hash, int hash_len);
-#endif /* CONFIG_FIT_RSASSA_PSS */
#define RSA_DEFAULT_PADDING_NAME "pkcs-1.5"
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index c27a784c429..0579e5294ee 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -401,15 +401,14 @@ static int rsa_sign_with_key(EVP_PKEY *pkey, struct
padding_algo *padding_algo,
goto err_sign;
}
-#ifdef CONFIG_FIT_RSASSA_PSS
- if (padding_algo && !strcmp(padding_algo->name, "pss")) {
+ if (CONFIG_IS_ENABLED(FIT_RSASSA_PSS) && padding_algo &&
+ !strcmp(padding_algo->name, "pss")) {
if (EVP_PKEY_CTX_set_rsa_padding(ckey,
RSA_PKCS1_PSS_PADDING) <= 0) {
ret = rsa_err("Signer padding setup failed");
goto err_sign;
}
}
-#endif /* CONFIG_FIT_RSASSA_PSS */
for (i = 0; i < region_count; i++) {
if (!EVP_DigestSignUpdate(context, region[i].data,
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index ad6d33d043a..9e522d210d7 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -102,7 +102,6 @@ U_BOOT_PADDING_ALGO(pkcs_15) = {
};
#endif
-#ifdef CONFIG_FIT_RSASSA_PSS
static void u32_i2osp(uint32_t val, uint8_t *buf)
{
buf[0] = (uint8_t)((val >> 24) & 0xff);
@@ -311,9 +310,6 @@ U_BOOT_PADDING_ALGO(pss) = {
};
#endif
-#endif
-
-#if CONFIG_IS_ENABLED(FIT_SIGNATURE) || CONFIG_IS_ENABLED(RSA_VERIFY_WITH_PKEY)
/**
* rsa_verify_key() - Verify a signature against some data using RSA Key
*
@@ -385,9 +381,7 @@ static int rsa_verify_key(struct image_sign_info *info,
return 0;
}
-#endif
-#if CONFIG_IS_ENABLED(RSA_VERIFY_WITH_PKEY)
/**
* rsa_verify_with_pkey() - Verify a signature against some data using
* only modulus and exponent as RSA key properties.
@@ -408,6 +402,9 @@ int rsa_verify_with_pkey(struct image_sign_info *info,
struct key_prop *prop;
int ret;
+ if (!CONFIG_IS_ENABLED(RSA_VERIFY_WITH_PKEY))
+ return -EACCES;
+
/* Public key is self-described to fill key_prop */
ret = rsa_gen_key_prop(info->key, info->keylen, );
if (ret) {
@@ -422,13 +419,6 @@ int rsa_verify_with_pkey(struct image_sign_info *info,
return ret;
}
-#else
-int rsa_verify_with_pkey(struct image_sign_info *info,
-const void *hash, uint8_t *sig, uint sig_len)
-{
- return -EACCES;
-}
-#endif
#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
/**
diff --git a/tools/Kconfig b/tools/Kconfig
index 9d1c0efd40c..8685c800f93 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -35,6 +35,11 @@ config TOOLS_FIT_PRINT
help
Print the content of the FIT verbosely in the tools builds
+config TOOLS_FIT_RSASSA_PSS
+ def_bool y
+ help
+ Support the rsassa-pss signature scheme in the tools builds
If we're going to have these TOOLS_ configs always on, what's the point
in adding a help text?
+
config TOOLS_FIT_SIGNATURE
def_bool y
help
Re: [PATCH v5 09/29] image: Use Kconfig to enable CONFIG_FIT_VERBOSE on host
On 9/25/21 8:43 PM, Simon Glass wrote:
Add a host Kconfig for FIT_VERBOSE. With this we can use
CONFIG_IS_ENABLED(FIT_VERBOSE) directly in the host build, so drop the
s/host build/ tools build/
forcing of this in the image.h header.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Use TOOLS_ instead of HOST_
include/image.h | 5 ++---
tools/Kconfig | 5 +
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/image.h b/include/image.h
index f09eb9de516..6efbef06e64 100644
--- a/include/image.h
+++ b/include/image.h
@@ -28,7 +28,6 @@ struct fdt_region;
#include
/* new uImage format support enabled on host */
-#define CONFIG_FIT_VERBOSE 1 /* enable fit_format_{error,warning}() */
#define CONFIG_FIT_RSASSA_PSS 1
#define IMAGE_ENABLE_IGNORE 0
@@ -1458,7 +1457,7 @@ int fit_image_cipher_get_algo(const void *fit, int
noffset, char **algo);
struct cipher_algo *image_get_cipher_algo(const char *full_name);
-#ifdef CONFIG_FIT_VERBOSE
+#if CONFIG_IS_ENABLED(FIT_VERBOSE)
#define fit_unsupported(msg) printf("! %s:%d " \
"FIT images not supported for '%s'\n", \
__FILE__, __LINE__, (msg))
I think if we made this a higher level message, like debug, or spew. we
wouldn't need to #ifdef it out with a CONFIG in the first place.
@@ -1470,7 +1469,7 @@ struct cipher_algo *image_get_cipher_algo(const char
*full_name);
#else
#define fit_unsupported(msg)
#define fit_unsupported_reset(msg)
-#endif /* CONFIG_FIT_VERBOSE */
+#endif /* FIT_VERBOSE */
#endif /* CONFIG_FIT */
#if !defined(USE_HOSTCC)
diff --git a/tools/Kconfig b/tools/Kconfig
index 747d221803f..9d1c0efd40c 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -45,6 +45,11 @@ config TOOLS_FIT_SIGNATURE_MAX_SIZE
depends on TOOLS_FIT_SIGNATURE
default 0x1000
+config TOOLS_FIT_VERBOSE
+ def_bool y
+ help
+ Support verbose FIT output in the tools builds
+
config TOOLS_MD5
def_bool y
help
Re: [PATCH v5 08/29] image: Drop IMAGE_ENABLE_OF_LIBFDT
On 9/25/21 8:43 PM, Simon Glass wrote:
Add a host Kconfig for OF_LIBFDT. With this we can use
CONFIG_IS_ENABLED(OF_LIBFDT) directly in the host build, so drop the
s/host build/tools build/
unnecessary indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Use TOOLS_ instead of HOST_
arch/arc/lib/bootm.c| 2 +-
arch/arm/lib/bootm.c| 4 ++--
arch/microblaze/lib/bootm.c | 2 +-
arch/nds32/lib/bootm.c | 4 ++--
arch/riscv/lib/bootm.c | 4 ++--
board/synopsys/hsdk/hsdk.c | 2 +-
common/bootm.c | 4 ++--
common/image-board.c| 8
common/image.c | 2 +-
include/image.h | 3 ---
lib/lmb.c | 2 +-
tools/Kconfig | 5 +
12 files changed, 22 insertions(+), 20 deletions(-)
I would have hoped deletions > insertions
diff --git a/arch/arc/lib/bootm.c b/arch/arc/lib/bootm.c
index 41408c2b460..ed6c5dfa584 100644
--- a/arch/arc/lib/bootm.c
+++ b/arch/arc/lib/bootm.c
@@ -63,7 +63,7 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
"(fake run for tracing)" : "");
bootstage_mark_name(BOOTSTAGE_ID_BOOTM_HANDOFF, "start_kernel");
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len) {
r0 = 2;
r2 = (unsigned int)images->ft_addr;
} else {
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index dd6a69315ac..a59a5e6c0ea 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -199,7 +199,7 @@ static void boot_prep_linux(bootm_headers_t *images)
{
char *commandline = env_get("bootargs");
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len) {
#ifdef CONFIG_OF_LIBFDT
debug("using: FDT\n");
if (image_setup_linux(images)) {
@@ -356,7 +356,7 @@ static void boot_jump_linux(bootm_headers_t *images, int
flag)
bootstage_mark(BOOTSTAGE_ID_RUN_OS);
announce_and_cleanup(fake);
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len)
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len)
r2 = (unsigned long)images->ft_addr;
else
r2 = gd->bd->bi_boot_params;
diff --git a/arch/microblaze/lib/bootm.c b/arch/microblaze/lib/bootm.c
index 3a6da6e29ff..12ea32488e6 100644
--- a/arch/microblaze/lib/bootm.c
+++ b/arch/microblaze/lib/bootm.c
@@ -75,7 +75,7 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
static void boot_prep_linux(bootm_headers_t *images)
{
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len) {
debug("using: FDT\n");
if (image_setup_linux(images)) {
printf("FDT creation failed! hanging...");
diff --git a/arch/nds32/lib/bootm.c b/arch/nds32/lib/bootm.c
index 1c7f7856999..71ebfb4225b 100644
--- a/arch/nds32/lib/bootm.c
+++ b/arch/nds32/lib/bootm.c
@@ -69,7 +69,7 @@ int do_bootm_linux(int flag, int argc, char *argv[],
bootm_headers_t *images)
debug("## Transferring control to Linux (at address %08lx) ...\n",
(ulong)theKernel);
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len) {
#ifdef CONFIG_OF_LIBFDT
debug("using: FDT\n");
if (image_setup_linux(images)) {
@@ -110,7 +110,7 @@ int do_bootm_linux(int flag, int argc, char *argv[],
bootm_headers_t *images)
#endif
}
cleanup_before_linux();
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len)
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len)
theKernel(0, machid, (unsigned long)images->ft_addr);
else
theKernel(0, machid, bd->bi_boot_params);
diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
index ff1bdf71318..2e1e286c8ef 100644
--- a/arch/riscv/lib/bootm.c
+++ b/arch/riscv/lib/bootm.c
@@ -64,7 +64,7 @@ static void announce_and_cleanup(int fake)
static void boot_prep_linux(bootm_headers_t *images)
{
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len) {
#ifdef CONFIG_OF_LIBFDT
debug("using: FDT\n");
if (image_setup_linux(images)) {
@@ -96,7 +96,7 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
announce_and_cleanup(fake);
if (!fake) {
- if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
+ if (CONFIG_IS_ENABLED(OF_LIBFDT) && images->ft_len) {
#ifdef CONFIG_SMP
ret = smp_call_function(images->ep,
(ulong)images->ft_addr, 0, 0);
diff --git a/board/synopsys/hsdk/hsdk.c b/board/synopsys/hsdk/hsdk.c
index 892b94bb083..226fbba6296 100644
Re: [PATCH v5 07/29] image: Drop IMAGE_ENABLE_FIT
On 9/25/21 8:43 PM, Simon Glass wrote:
Make use of the host Kconfig for FIT. With this we can use
CONFIG_IS_ENABLED(FIT) directly in the host build, so drop the unnecessary
indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Rebase to next
arch/arm/mach-imx/hab.c | 2 +-
common/bootm.c | 10 +-
common/image-board.c| 16
common/image.c | 2 +-
include/fdt_support.h | 2 +-
include/image.h | 17 -
6 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
index cc39e6bf569..55317abba23 100644
--- a/arch/arm/mach-imx/hab.c
+++ b/arch/arm/mach-imx/hab.c
@@ -591,7 +591,7 @@ static ulong get_image_ivt_offset(ulong img_addr)
return (image_get_image_size((image_header_t *)img_addr)
+ 0x1000 - 1) & ~(0x1000 - 1);
#endif
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
case IMAGE_FORMAT_FIT:
return (fit_get_size(buf) + 0x1000 - 1) & ~(0x1000 - 1);
#endif
diff --git a/common/bootm.c b/common/bootm.c
index fe17d1da9e5..8d614fe140e 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -115,7 +115,7 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag,
int argc,
images.os.arch = image_get_arch(os_hdr);
break;
#endif
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
case IMAGE_FORMAT_FIT:
if (fit_image_get_type(images.fit_hdr_os,
images.fit_noffset_os,
@@ -187,7 +187,7 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag,
int argc,
/* Kernel entry point is the setup.bin */
} else if (images.legacy_hdr_valid) {
images.ep = image_get_ep(_hdr_os_copy);
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
} else if (images.fit_uname_os) {
int ret;
@@ -295,7 +295,7 @@ int bootm_find_images(int flag, int argc, char *const argv[], ulong start,
set_working_fdt_addr(map_to_sysmem(images.ft_addr));
#endif
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
if (IS_ENABLED(CONFIG_FPGA)) {
/* find bitstreams */
ret = boot_get_fpga(argc, argv, , IH_ARCH_DEFAULT,
@@ -858,7 +858,7 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp,
int flag, int argc,
const void *buf;
const char *fit_uname_config = NULL;
const char *fit_uname_kernel = NULL;
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
int os_noffset;
#endif
@@ -916,7 +916,7 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc,
bootstage_mark(BOOTSTAGE_ID_DECOMP_IMAGE);
break;
#endif
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
case IMAGE_FORMAT_FIT:
os_noffset = fit_image_load(images, img_addr,
_uname_kernel, _uname_config,
diff --git a/common/image-board.c b/common/image-board.c
index c13d0493ca3..c699232ba13 100644
--- a/common/image-board.c
+++ b/common/image-board.c
@@ -282,7 +282,7 @@ int genimg_get_format(const void *img_addr)
if (image_check_magic(hdr))
return IMAGE_FORMAT_LEGACY;
#endif
-#if IMAGE_ENABLE_FIT || IMAGE_ENABLE_OF_LIBFDT
+#if CONFIG_IS_ENABLED(FIT) || IMAGE_ENABLE_OF_LIBFDT
if (fdt_check_header(img_addr) == 0)
return IMAGE_FORMAT_FIT;
#endif
@@ -307,7 +307,7 @@ int genimg_get_format(const void *img_addr)
*/
int genimg_has_config(bootm_headers_t *images)
{
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
if (images->fit_uname_cfg)
return 1;
#endif
@@ -348,7 +348,7 @@ int boot_get_ramdisk(int argc, char *const argv[],
bootm_headers_t *images,
#ifdef CONFIG_SUPPORT_RAW_INITRD
char *end;
#endif
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
const char *fit_uname_config = images->fit_uname_cfg;
const char *fit_uname_ramdisk = NULL;
ulong default_addr;
@@ -380,7 +380,7 @@ int boot_get_ramdisk(int argc, char *const argv[],
bootm_headers_t *images,
rd_len = 0;
rd_data = 0;
} else if (select || genimg_has_config(images)) {
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
if (select) {
/*
* If the init ramdisk comes from the FIT image and
@@ -409,7 +409,7 @@ int boot_get_ramdisk(int argc, char *const argv[],
bootm_headers_t *images,
debug("* ramdisk: cmdline image address =
0x%08lx\n",
rd_addr);
}
-#if IMAGE_ENABLE_FIT
+#if CONFIG_IS_ENABLED(FIT)
} else {
/* use FIT configuration provided in first bootm
Re: [PATCH v5 06/29] hash: Drop some #ifdefs in hash.c
On 9/25/21 8:43 PM, Simon Glass wrote:
We can use the __maybe_unused attribute to avoid some of the #ifdefs in
this file. Update the functions accordingly.
Note: The actual hashing interface is still a mess, with four separate
combinations and lots of #ifdefs. This should really use a driver
approach, e.g. as is done with partition drivers.
Signed-off-by: Simon Glass
---
I'm not too excited about a qualifier that maybe tells me something
about the function.
Assuming we move these to linker lists in the near (not far) future,
Reviewed-by: Alexandru Gagniuc
Changes in v5:
- Rebase to next
common/hash.c | 54 ---
1 file changed, 25 insertions(+), 29 deletions(-)
diff --git a/common/hash.c b/common/hash.c
index 0fe65c959d0..e92f9a9594f 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -24,6 +24,7 @@
#include
#else
#include "mkimage.h"
+#include
#include
#include
#endif /* !USE_HOSTCC*/
@@ -42,8 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
static void reloc_update(void);
-#if CONFIG_IS_ENABLED(SHA1) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
-static int hash_init_sha1(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp)
{
sha1_context *ctx = malloc(sizeof(sha1_context));
sha1_starts(ctx);
@@ -51,15 +51,16 @@ static int hash_init_sha1(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_sha1(struct hash_algo *algo, void *ctx, const void *buf,
- unsigned int size, int is_last)
+static int __maybe_unused hash_update_sha1(struct hash_algo *algo, void *ctx,
+ const void *buf, unsigned int size,
+ int is_last)
{
sha1_update((sha1_context *)ctx, buf, size);
return 0;
}
-static int hash_finish_sha1(struct hash_algo *algo, void *ctx, void *dest_buf,
- int size)
+static int __maybe_unused hash_finish_sha1(struct hash_algo *algo, void *ctx,
+ void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -68,10 +69,8 @@ static int hash_finish_sha1(struct hash_algo *algo, void
*ctx, void *dest_buf,
free(ctx);
return 0;
}
-#endif
-#if CONFIG_IS_ENABLED(SHA256) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
-static int hash_init_sha256(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_sha256(struct hash_algo *algo, void **ctxp)
{
sha256_context *ctx = malloc(sizeof(sha256_context));
sha256_starts(ctx);
@@ -79,15 +78,16 @@ static int hash_init_sha256(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_sha256(struct hash_algo *algo, void *ctx,
- const void *buf, unsigned int size, int is_last)
+static int __maybe_unused hash_update_sha256(struct hash_algo *algo, void *ctx,
+const void *buf, uint size,
+int is_last)
{
sha256_update((sha256_context *)ctx, buf, size);
return 0;
}
-static int hash_finish_sha256(struct hash_algo *algo, void *ctx, void
- *dest_buf, int size)
+static int __maybe_unused hash_finish_sha256(struct hash_algo *algo, void *ctx,
+void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -96,10 +96,8 @@ static int hash_finish_sha256(struct hash_algo *algo, void
*ctx, void
free(ctx);
return 0;
}
-#endif
-#if CONFIG_IS_ENABLED(SHA384) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
-static int hash_init_sha384(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_sha384(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
sha384_starts(ctx);
@@ -107,15 +105,16 @@ static int hash_init_sha384(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_sha384(struct hash_algo *algo, void *ctx,
- const void *buf, unsigned int size, int is_last)
+static int __maybe_unused hash_update_sha384(struct hash_algo *algo, void *ctx,
+const void *buf, uint size,
+int is_last)
{
sha384_update((sha512_context *)ctx, buf, size);
return 0;
}
-static int hash_finish_sha384(struct hash_algo *algo, void *ctx, void
- *dest_buf, int size)
+static int __maybe_unused hash_finish_sha384(struct hash_algo *algo, void *ctx,
+void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -124,10 +123,8 @@ static
Re: [PATCH v5 05/29] hash: Use Kconfig to enable hashing in host tools and SPL
On 9/25/21 8:43 PM, Simon Glass wrote:
At present when building host tools, we force CONFIG_SHAxxx to be enabled
regardless of the board Kconfig setting. This is done in the image.h
header file.
For SPL we currently just assume the algorithm is desired if U-Boot proper
enables it.
Clean this up by adding new Kconfig options to enable hashing on the host,
relying on CONFIG_IS_ENABLED() to deal with the different builds.
Add new SPL Kconfigs for hardware-accelerated hashing, to maintain the
current settings.
This allows us to drop the image.h code and the I_WANT_MD5 hack.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Drop I_WANT_MD5
- Use TOOLS_ instead of HOST_
Changes in v2:
- Add SPL_ Kconfigs also, since otherwise hashing algorithms drop from SPL
common/hash.c | 49 +++--
include/image.h | 5 -
lib/Kconfig | 18 ++
tools/Kconfig | 25 +
4 files changed, 66 insertions(+), 31 deletions(-)
diff --git a/common/hash.c b/common/hash.c
index 6277fe65b3e..0fe65c959d0 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -25,6 +25,7 @@
#else
#include "mkimage.h"
#include
+#include
#endif /* !USE_HOSTCC*/
#include
@@ -41,7 +42,7 @@ DECLARE_GLOBAL_DATA_PTR;
static void reloc_update(void);
-#if defined(CONFIG_SHA1) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA1) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha1(struct hash_algo *algo, void **ctxp)
{
sha1_context *ctx = malloc(sizeof(sha1_context));
@@ -69,7 +70,7 @@ static int hash_finish_sha1(struct hash_algo *algo, void
*ctx, void *dest_buf,
}
#endif
One day, I'd like to make these init__ functions part of
linker lists and move each to their respective .c
One day.
-#if defined(CONFIG_SHA256) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA256) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha256(struct hash_algo *algo, void **ctxp)
{
sha256_context *ctx = malloc(sizeof(sha256_context));
@@ -97,7 +98,7 @@ static int hash_finish_sha256(struct hash_algo *algo, void
*ctx, void
}
#endif
-#if defined(CONFIG_SHA384) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA384) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha384(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
@@ -125,7 +126,7 @@ static int hash_finish_sha384(struct hash_algo *algo, void
*ctx, void
}
#endif
-#if defined(CONFIG_SHA512) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA512) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha512(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
@@ -207,18 +208,13 @@ static int hash_finish_crc32(struct hash_algo *algo, void
*ctx, void *dest_buf,
return 0;
}
-#ifdef USE_HOSTCC
-# define I_WANT_MD51
-#else
-# define I_WANT_MD5CONFIG_IS_ENABLED(MD5)
-#endif
/*
* These are the hash algorithms we support. If we have hardware acceleration
* is enable we will use that, otherwise a software version of the algorithm.
* Note that algorithm names must be in lower case.
*/
static struct hash_algo hash_algo[] = {
-#if I_WANT_MD5
+#if CONFIG_IS_ENABLED(MD5)
{
.name = "md5",
.digest_size= MD5_SUM_LEN,
@@ -226,17 +222,17 @@ static struct hash_algo hash_algo[] = {
.hash_func_ws = md5_wd,
},
#endif
-#ifdef CONFIG_SHA1
+#if CONFIG_IS_ENABLED(SHA1)
{
.name = "sha1",
.digest_size= SHA1_SUM_LEN,
.chunk_size = CHUNKSZ_SHA1,
-#ifdef CONFIG_SHA_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_HW_ACCEL)
.hash_func_ws = hw_sha1,
#else
.hash_func_ws = sha1_csum_wd,
#endif
-#ifdef CONFIG_SHA_PROG_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
.hash_init = hw_sha_init,
.hash_update= hw_sha_update,
.hash_finish= hw_sha_finish,
@@ -247,17 +243,17 @@ static struct hash_algo hash_algo[] = {
#endif
},
#endif
-#ifdef CONFIG_SHA256
+#if CONFIG_IS_ENABLED(SHA256)
{
.name = "sha256",
.digest_size= SHA256_SUM_LEN,
.chunk_size = CHUNKSZ_SHA256,
-#ifdef CONFIG_SHA_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_HW_ACCEL)
.hash_func_ws = hw_sha256,
#else
.hash_func_ws = sha256_csum_wd,
#endif
-#ifdef CONFIG_SHA_PROG_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
.hash_init = hw_sha_init,
.hash_update= hw_sha_update,
.hash_finish= hw_sha_finish,
@@ -268,17 +264,17 @@ static struct
Re: [PATCH v5 04/29] spl: cypto: Bring back SPL_ versions of SHA
On 9/25/21 8:43 PM, Simon Glass wrote:
Unfortunately these were removed by mistake. This means that adding hash
support to SPL brings in all software algorithms, with a substantial
increase in code size.
The origin of the problem was renaming them to SPL_FIT_xxx and then these
were removed altogether in a later commit.
Add them back. This aligns with CONFIG_MD5, for example, which has an SPL
variant.
Signed-off-by: Simon Glass
Fixes: f5bc9c25f31 ("image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx")
Fixes: eb5171ddec9 ("common: Remove unused CONFIG_FIT_SHAxxx selectors")
Reviewed-by: Alexandru Gagniuc
I don't think these ever worked as intended. I had issues disabling SHA1
in SPL before either of the suspect patches were merged.
---
For now this has no effect but the next patch updates hash.c to deal with
this condition for both SPL and tools.
Changes in v5:
- Add new patch to bring back SPL_ versions of SHA
lib/Kconfig | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/lib/Kconfig b/lib/Kconfig
index 034af724b5d..7899e756f99 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -373,7 +373,6 @@ config SHA256
The SHA256 algorithm produces a 256-bit (32-byte) hash value
(digest).
-
config SHA512
bool "Enable SHA512 support"
help
@@ -399,6 +398,48 @@ config SHA_HW_ACCEL
hashing algorithms. This affects the 'hash' command and also the
hash_lookup_algo() function.
+if SPL
+
+config SPL_SHA1
+ bool "Enable SHA1 support in SPL"
+ default y if SHA1
+ help
+ This option enables support of hashing using SHA1 algorithm.
+ The hash is calculated in software.
+ The SHA1 algorithm produces a 160-bit (20-byte) hash value
+ (digest).
+
+config SPL_SHA256
+ bool "Enable SHA256 support in SPL"
+ default y if SHA256
+ help
+ This option enables support of hashing using SHA256 algorithm.
+ The hash is calculated in software.
+ The SHA256 algorithm produces a 256-bit (32-byte) hash value
+ (digest).
+
+config SPL_SHA512
+ bool "Enable SHA512 support in SPL"
+ default y if SHA512
+ help
+ This option enables support of hashing using SHA512 algorithm.
+ The hash is calculated in software.
+ The SHA512 algorithm produces a 512-bit (64-byte) hash value
+ (digest).
+
+config SPL_SHA384
+ bool "Enable SHA384 support in SPL"
+ default y if SHA384
+ select SPL_SHA512
+ help
+ This option enables support of hashing using SHA384 algorithm.
+ The hash is calculated in software. This is also selects SHA512,
+ because these implementations share the bulk of the code..
+ The SHA384 algorithm produces a 384-bit (48-byte) hash value
+ (digest).
+
+endif
+
if SHA_HW_ACCEL
config SHA512_HW_ACCEL
Re: [PATCH v5 03/29] image: Add Kconfig options for FIT in the tools build
On 9/25/21 8:43 PM, Simon Glass wrote:
In preparation for enabling CONFIG_IS_ENABLED() on the host build, add
some options to enable the various FIT options expected in these tools.
This will ensure that the code builds correctly when CONFIG_TOOLS_xxx
is distinct from CONFIG_xxx.
Drop some #ifdefs which are immediately unnecessary (many more are in
later patches).
Signed-off-by: Simon Glass
Under protest,
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Drop changes to tools/Makefile since they already use TOOLS_ Kconfigs
- Use TOOLS_ instead of HOST_
common/image-fit-sig.c | 3 ++-
common/image-fit.c | 4 ++--
tools/Kconfig | 25 +
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index b979cd2a4b6..e95e64bd2fe 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -72,11 +72,12 @@ static int fit_image_setup_verify(struct image_sign_info
*info,
char *algo_name;
const char *padding_name;
+#ifndef USE_HOSTCC
if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) {
If you set CONFIG_FIT_SIGNATURE_MAX_SIZE to 0x1000, then this
comparison will always be false.
*err_msgp = "Total size too large";
return 1;
}
-
+#endif
if (fit_image_hash_get_algo(fit, noffset, _name)) {
*err_msgp = "Can't get hash algo property";
return -1;
diff --git a/common/image-fit.c b/common/image-fit.c
index 6f8e67e4158..17c6d4e7813 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -170,7 +170,7 @@ int fit_get_subimage_count(const void *fit, int
images_noffset)
return count;
}
-#if CONFIG_IS_ENABLED(FIT_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT)
+#if CONFIG_IS_ENABLED(FIT_PRINT)
/**
* fit_image_print_data() - prints out the hash node details
* @fit: pointer to the FIT format image header
@@ -578,7 +578,7 @@ void fit_image_print(const void *fit, int image_noffset,
const char *p)
#else
void fit_print_contents(const void *fit) { }
void fit_image_print(const void *fit, int image_noffset, const char *p) { }
-#endif /* CONFIG_IS_ENABLED(FIR_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT) */
+#endif /* CONFIG_IS_ENABLED(FIT_PRINT) */
/**
* fit_get_desc - get node description property
diff --git a/tools/Kconfig b/tools/Kconfig
index d6f82cd949b..ea986ab0479 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -20,4 +20,29 @@ config TOOLS_LIBCRYPTO
This selection does not affect target features, such as runtime FIT
signature verification.
+config TOOLS_FIT
+config TOOLS_FIT_FULL_CHECK
+config TOOLS_FIT_PRINT
+config TOOLS_FIT_SIGNATURE
Besides FIT_PRINT, I'm confused as to where the others are used by the
tools build.
+
+config TOOLS_FIT_SIGNATURE_MAX_SIZE
On the other hand, if you #ifdef out the check in image-fit-sig.c, then
it doesn't make sense to have this config on the tools.
+ hex
+ depends on TOOLS_FIT_SIGNATURE
+ default 0x1000
+
endmenu
Re: [PATCH v5 03/29] image: Add Kconfig options for FIT in the tools build
On 9/25/21 8:43 PM, Simon Glass wrote:
In preparation for enabling CONFIG_IS_ENABLED() on the host build, add
some options to enable the various FIT options expected in these tools.
This will ensure that the code builds correctly when CONFIG_TOOLS_xxx
is distinct from CONFIG_xxx.
Drop some #ifdefs which are immediately unnecessary (many more are in
later patches).
Signed-off-by: Simon Glass
Under protest,
Reviewed-by: Alexandru Gagniuc
---
Changes in v5:
- Drop changes to tools/Makefile since they already use TOOLS_ Kconfigs
- Use TOOLS_ instead of HOST_
common/image-fit-sig.c | 3 ++-
common/image-fit.c | 4 ++--
tools/Kconfig | 25 +
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index b979cd2a4b6..e95e64bd2fe 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -72,11 +72,12 @@ static int fit_image_setup_verify(struct image_sign_info
*info,
char *algo_name;
const char *padding_name;
+#ifndef USE_HOSTCC
if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) {
If you set CONFIG_FIT_SIGNATURE_MAX_SIZE to 0x1000, then this
comparison will always be false.
*err_msgp = "Total size too large";
return 1;
}
-
+#endif
if (fit_image_hash_get_algo(fit, noffset, _name)) {
*err_msgp = "Can't get hash algo property";
return -1;
diff --git a/common/image-fit.c b/common/image-fit.c
index 6f8e67e4158..17c6d4e7813 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -170,7 +170,7 @@ int fit_get_subimage_count(const void *fit, int
images_noffset)
return count;
}
-#if CONFIG_IS_ENABLED(FIT_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT)
+#if CONFIG_IS_ENABLED(FIT_PRINT)
/**
* fit_image_print_data() - prints out the hash node details
* @fit: pointer to the FIT format image header
@@ -578,7 +578,7 @@ void fit_image_print(const void *fit, int image_noffset,
const char *p)
#else
void fit_print_contents(const void *fit) { }
void fit_image_print(const void *fit, int image_noffset, const char *p) { }
-#endif /* CONFIG_IS_ENABLED(FIR_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT) */
+#endif /* CONFIG_IS_ENABLED(FIT_PRINT) */
/**
* fit_get_desc - get node description property
diff --git a/tools/Kconfig b/tools/Kconfig
index d6f82cd949b..ea986ab0479 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -20,4 +20,29 @@ config TOOLS_LIBCRYPTO
This selection does not affect target features, such as runtime FIT
signature verification.
+config TOOLS_FIT
+config TOOLS_FIT_FULL_CHECK
+config TOOLS_FIT_PRINT
+config TOOLS_FIT_SIGNATURE
Besides FIT_PRINT, I'm confused as to where the others are used by the
tools build.
+
+config TOOLS_FIT_SIGNATURE_MAX_SIZE
On the other hand, if you #ifdef out the check in image-fit-sig.c, then
it doesn't make sense to have this config on the tools.
+ hex
+ depends on TOOLS_FIT_SIGNATURE
+ default 0x1000
+
endmenu
Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()
On 9/25/21 8:43 PM, Simon Glass wrote: At present we must separately test for the host build for many options, since we force them to be enabled. For example, CONFIG_FIT is always enabled in the host tools, even if CONFIG_FIT is not enabled by the board itself. It would be more convenient if we could use, for example, CONFIG_IS_ENABLED(FIT) and get CONFIG_HOST_FIT, when building for the host. Add support for this. With this and the tools_build() function, we should be able to remove all the #ifdefs currently needed in code that is build by tools and targets. This will be even nicer when we move to using CONFIG(xxx) everywhere, since all the #ifdef and IS_ENABLED/CONFIG_IS_ENABLED stuff will go away. Signed-off-by: Simon Glass Suggested-by: Rasmus Villemoes # b4f73886 Under protest, Reviewed-by: Alexandru Gagniuc --- Changes in v5: - Update commit message - Use TOOLS_ instead of HOST_ Changes in v2: - Correct comment about USE_HOSTCC being undefined in CONFIG_VAL() - Fix up comment to put an underscore after every CONFIG include/linux/kconfig.h | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index d109ed3119e..a1d1a298426 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -31,11 +31,14 @@ (config_enabled(option)) /* - * U-Boot add-on: Helper macros to reference to different macros - * (CONFIG_ or CONFIG_SPL_ prefixed), depending on the build context. + * U-Boot add-on: Helper macros to reference to different macros (prefixed by + * CONFIG_, CONFIG_SPL_, CONFIG_TPL_ or CONFIG_TOOLS_), depending on the build + * context. */ -#if defined(CONFIG_TPL_BUILD) +#ifdef USE_HOSTCC +#define _CONFIG_PREFIX TOOLS_ +#elif defined(CONFIG_TPL_BUILD) #define _CONFIG_PREFIX TPL_ #elif defined(CONFIG_SPL_BUILD) #define _CONFIG_PREFIX SPL_ @@ -49,6 +52,7 @@ /* * CONFIG_VAL(FOO) evaluates to the value of + * CONFIG_TOOLS_FOO if USE_HOSTCC is defined, * CONFIG_FOO if CONFIG_SPL_BUILD is undefined, * CONFIG_SPL_FOO if CONFIG_SPL_BUILD is defined. * CONFIG_TPL_FOO if CONFIG_TPL_BUILD is defined. @@ -76,18 +80,21 @@ /* * CONFIG_IS_ENABLED(FOO) expands to + * 1 if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y', * 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y', * 1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y', * 1 if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y', * 0 otherwise. * * CONFIG_IS_ENABLED(FOO, (abc)) expands to + * abc if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y', * abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y', * nothing otherwise. * * CONFIG_IS_ENABLED(FOO, (abc), (def)) expands to + * abc if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y', * abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
Re: [PATCH v5 01/29] compiler: Rename host_build() to tools_build()
On 9/25/21 8:43 PM, Simon Glass wrote:
With the new TOOLS_LIBCRYPTO and some other changes, it seems that we are
heading towards calling this a tools build rather than a host build,
although of course it does happen on the host.
I cannot think of anything built by the host which cannot be described as
a tool, so rename this function.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
I think this change makes sense. The code becomes clearer, as we remove
the ambiguity of what host means.
---
Changes in v5:
- Add a new patch to rename host_build() to tools_build()
common/image-fit.c | 8
common/image.c | 12 ++--
include/compiler.h | 5 +++--
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index 5a0a0cc2007..6f8e67e4158 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -509,7 +509,7 @@ void fit_image_print(const void *fit, int image_noffset,
const char *p)
ret = fit_image_get_data_and_size(fit, image_noffset, , );
- if (!host_build()) {
+ if (!tools_build()) {
printf("%s Data Start: ", p);
if (ret) {
printf("unavailable\n");
@@ -1845,7 +1845,7 @@ int fit_conf_get_node(const void *fit, const char
*conf_uname)
if (conf_uname == NULL) {
/* get configuration unit name from the default property */
debug("No configuration specified, trying default...\n");
- if (!host_build() && IS_ENABLED(CONFIG_MULTI_DTB_FIT)) {
+ if (!tools_build() && IS_ENABLED(CONFIG_MULTI_DTB_FIT)) {
noffset = fit_find_config_node(fit);
if (noffset < 0)
return noffset;
@@ -2093,7 +2093,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
}
bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
- if (!host_build() && IS_ENABLED(CONFIG_SANDBOX)) {
+ if (!tools_build() && IS_ENABLED(CONFIG_SANDBOX)) {
if (!fit_image_check_target_arch(fit, noffset)) {
puts("Unsupported Architecture\n");
bootstage_error(bootstage_id +
BOOTSTAGE_SUB_CHECK_ARCH);
@@ -2158,7 +2158,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
}
/* perform any post-processing on the image data */
- if (!host_build() && IS_ENABLED(CONFIG_FIT_IMAGE_POST_PROCESS))
+ if (!tools_build() && IS_ENABLED(CONFIG_FIT_IMAGE_POST_PROCESS))
board_fit_image_post_process(fit, noffset, , );
len = (ulong)size;
diff --git a/common/image.c b/common/image.c
index 2f2fd052c50..66685b4ba99 100644
--- a/common/image.c
+++ b/common/image.c
@@ -460,11 +460,11 @@ int image_decomp(int comp, ulong load, ulong image_start,
int type,
ret = -ENOSPC;
break;
case IH_COMP_GZIP:
- if (!host_build() && CONFIG_IS_ENABLED(GZIP))
+ if (!tools_build() && CONFIG_IS_ENABLED(GZIP))
ret = gunzip(load_buf, unc_len, image_buf, _len);
break;
case IH_COMP_BZIP2:
- if (!host_build() && CONFIG_IS_ENABLED(BZIP2)) {
+ if (!tools_build() && CONFIG_IS_ENABLED(BZIP2)) {
uint size = unc_len;
/*
@@ -478,7 +478,7 @@ int image_decomp(int comp, ulong load, ulong image_start,
int type,
}
break;
case IH_COMP_LZMA:
- if (!host_build() && CONFIG_IS_ENABLED(LZMA)) {
+ if (!tools_build() && CONFIG_IS_ENABLED(LZMA)) {
SizeT lzma_len = unc_len;
ret = lzmaBuffToBuffDecompress(load_buf, _len,
@@ -487,7 +487,7 @@ int image_decomp(int comp, ulong load, ulong image_start,
int type,
}
break;
case IH_COMP_LZO:
- if (!host_build() && CONFIG_IS_ENABLED(LZO)) {
+ if (!tools_build() && CONFIG_IS_ENABLED(LZO)) {
size_t size = unc_len;
ret = lzop_decompress(image_buf, image_len, load_buf, );
@@ -495,7 +495,7 @@ int image_decomp(int comp, ulong load, ulong image_start,
int type,
}
break;
case IH_COMP_LZ4:
- if (!host_build() && CONFIG_IS_ENABLED(LZ4)) {
+ if (!tools_build() && CONFIG_IS_ENABLED(LZ4)) {
size_t size = unc_len;
ret = ulz4fn(image_buf, image_len, load_buf, );
@@ -503,7 +503,7 @@ int image_decomp(int comp, ulong load, ulong image_start,
int type,
}
break;
case IH_COMP_ZSTD:
- if (!host_build() && CONFIG_IS_ENABLED(ZSTD)) {
+ if (!tools_build() && CONFIG_IS_ENABLED(ZSTD)) {
struct abuf in, out;
abuf_init_set(, image_buf, image_len);
diff --git a/include/compiler.h
Re: FIT image: load secure FPGA
On 10/4/21 3:32 PM, Jorge Ramirez-Ortiz, Foundries wrote:
Hello,
We are enabling secure boot on Zynqmp with SPL.
The issue however is that during secure boot, the bootrom not only
validates the first loader (SPL and PMUFW combo) but it will also
expect a signed bitstream during load(FPGA).
Since currently the SPL load of an FPGA image from FIT does not
support loading images for authentication (fpga_loads), I'd like to
discuss how to best implement such support.
What do you mean by "loading images for authentication" ?
A pretty standard file.its description of the FPGA loadable looks like
this:
fpga {
description = "FPGA binary";
data = /incbin/("${DEPLOY_DIR_IMAGE}/${SPL_FPGA_BINARY}");
type = "fpga";
arch = "${UBOOT_ARCH}";
compression = "none";
load = <${fpgaloadaddr}>;
hash-1 {
algo = "${FIT_HASH_ALG}";
};
};
We could extend imagetool.h struct image_tool_params to add more
params or perhpas just define different 'types' of fpga?
Check "4) '/images' node"
in doc/uImage.FIT/source_file_format.txt
The intent is to give either:
* loadaddr="$(addr)" : copy image to $(addr), Done
* compatible="": Use this driver to upload the FPGA
It seems to me like the right way to go is to make a new compatible=""
FPGA loader is for fpga_load():
fpga {
description = "FPGA binary";
data = /incbin/("${YOCTO_BS_PATH}");
type = "fpga";
compression = "none";
compatible = "zynqmp-fancy-fpga",
hash-1 {
algo = "${FIT_HASHISH}";
};
};
Something like:
"fpga"
"fpga-auth" : authenticated
"fpga-enc" : encrypted
"fpga-sec" : encrypted and authenticated
Can these properties be inferred from the FPGA image? If not, they could
be required when using a new fpga loader. I don't think they should be
added to "fpga-legacy".
Alex
Then it would be a matter of modifying
https://github.com/u-boot/u-boot/blob/master/common/spl/spl_fit.c#L572
any thoughts?
TIA
Jorge
Re: [PATCH] arm: dts: stm32mp1: use ssbl partition name for U-Boot
On 9/14/21 7:14 AM, Patrick Delaunay wrote:
Continue to use the "ssbl" name for GPT partition of secondary boot
stage = U-Boot for basic boot with SPL to avoid to disturb existing user.
The "fip" partition name is only used for TFA_BOOT with FIP, it is a TF-A
BL2 requirement; it the default configuration for STMicroelectronics
boards.
Fixes: b73e8bf453f8 ("arm: stm32mp: add defconfig for trusted boot with FIP")
Signed-off-by: Patrick Delaunay
Tested-by: Alexandru Gagniuc
Re: [PATCH v5 05/29] hash: Use Kconfig to enable hashing in host tools and SPL
On 9/27/21 11:06 AM, Simon Glass wrote: Hi Alex, On Mon, 27 Sept 2021 at 09:53, Alex G. wrote: Hi Simon On 9/25/21 8:43 PM, Simon Glass wrote: At present when building host tools, we force CONFIG_SHAxxx to be enabled regardless of the board Kconfig setting. This is done in the image.h header file. For SPL we currently just assume the algorithm is desired if U-Boot proper enables it. Clean this up by adding new Kconfig options to enable hashing on the host, relying on CONFIG_IS_ENABLED() to deal with the different builds. Add new SPL Kconfigs for hardware-accelerated hashing, to maintain the current settings. This allows us to drop the image.h code and the I_WANT_MD5 hack. Signed-off-by: Simon Glass --- Changes in v5: - Drop I_WANT_MD5 - Use TOOLS_ instead of HOST_ I was planning to move hash algos to a linker list for target code, and a simple, ifdef-free list for the host, like we've done for crypto algos, and I think padding algos. We'd be able to drop #define I_WANT_MD5 (which is temporary), and drop define CONFIG_SHA_*, and by extension, drop the need for CONFIG_TOOLS_SHA*. I think that's far more elegant. OK but can we do that after my series goes in? The series is a significant #ifdef clean-up and I think you can easily do that on top of the series. Of course! Alex
Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()
On 9/25/21 8:43 PM, Simon Glass wrote: At present we must separately test for the host build for many options, since we force them to be enabled. For example, CONFIG_FIT is always enabled in the host tools, even if CONFIG_FIT is not enabled by the board itself. It would be more convenient if we could use, for example, CONFIG_IS_ENABLED(FIT) and get CONFIG_HOST_FIT, when building for the host. Add support for this. With this and the tools_build() function, we should be able to remove all the #ifdefs currently needed in code that is build by tools and targets. This will be even nicer when we move to using CONFIG(xxx) everywhere, since all the #ifdef and IS_ENABLED/CONFIG_IS_ENABLED stuff will go away. Signed-off-by: Simon Glass Suggested-by: Rasmus Villemoes # b4f73886 --- In my vision, the host tools are target-agostic. They always support the same feature set: all features. From that point of view, it doesn't make sense to have options which enable or disable tools features. I understand the motivation is to use CONFIG_IS_ENABLED(), which has the appearance of making the code cleaner. We have to continue using #if CONFIG_IS_ENABLED() -- is that an improvement over old-school #ifdefs? So I question whether this new direction makes sense for the long term, as opposed to taking a deeper look at the underlying code. The polite thing from me would be to propose alternatives, which I don't have the bandwidth these days. I won't be adding opposition to this series other than these final thoughts. We can fix the code later, and then remove the HOST configs. Alex Changes in v5: - Update commit message - Use TOOLS_ instead of HOST_ Changes in v2: - Correct comment about USE_HOSTCC being undefined in CONFIG_VAL() - Fix up comment to put an underscore after every CONFIG include/linux/kconfig.h | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index d109ed3119e..a1d1a298426 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -31,11 +31,14 @@ (config_enabled(option)) /* - * U-Boot add-on: Helper macros to reference to different macros - * (CONFIG_ or CONFIG_SPL_ prefixed), depending on the build context. + * U-Boot add-on: Helper macros to reference to different macros (prefixed by + * CONFIG_, CONFIG_SPL_, CONFIG_TPL_ or CONFIG_TOOLS_), depending on the build + * context. */ -#if defined(CONFIG_TPL_BUILD) +#ifdef USE_HOSTCC +#define _CONFIG_PREFIX TOOLS_ +#elif defined(CONFIG_TPL_BUILD) #define _CONFIG_PREFIX TPL_ #elif defined(CONFIG_SPL_BUILD) #define _CONFIG_PREFIX SPL_ @@ -49,6 +52,7 @@ /* * CONFIG_VAL(FOO) evaluates to the value of + * CONFIG_TOOLS_FOO if USE_HOSTCC is defined, * CONFIG_FOO if CONFIG_SPL_BUILD is undefined, * CONFIG_SPL_FOO if CONFIG_SPL_BUILD is defined. * CONFIG_TPL_FOO if CONFIG_TPL_BUILD is defined. @@ -76,18 +80,21 @@ /* * CONFIG_IS_ENABLED(FOO) expands to + * 1 if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y', * 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y', * 1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y', * 1 if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y', * 0 otherwise. * * CONFIG_IS_ENABLED(FOO, (abc)) expands to + * abc if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y', * abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y', * nothing otherwise. * * CONFIG_IS_ENABLED(FOO, (abc), (def)) expands to + * abc if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y', * abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y', * abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
Re: [PATCH v5 05/29] hash: Use Kconfig to enable hashing in host tools and SPL
Hi Simon
On 9/25/21 8:43 PM, Simon Glass wrote:
At present when building host tools, we force CONFIG_SHAxxx to be enabled
regardless of the board Kconfig setting. This is done in the image.h
header file.
For SPL we currently just assume the algorithm is desired if U-Boot proper
enables it.
Clean this up by adding new Kconfig options to enable hashing on the host,
relying on CONFIG_IS_ENABLED() to deal with the different builds.
Add new SPL Kconfigs for hardware-accelerated hashing, to maintain the
current settings.
This allows us to drop the image.h code and the I_WANT_MD5 hack.
Signed-off-by: Simon Glass
---
Changes in v5:
- Drop I_WANT_MD5
- Use TOOLS_ instead of HOST_
I was planning to move hash algos to a linker list for target code, and
a simple, ifdef-free list for the host, like we've done for crypto
algos, and I think padding algos.
We'd be able to drop #define I_WANT_MD5 (which is temporary), and drop
define CONFIG_SHA_*, and by extension, drop the need for
CONFIG_TOOLS_SHA*. I think that's far more elegant.
Alex
Changes in v2:
- Add SPL_ Kconfigs also, since otherwise hashing algorithms drop from SPL
common/hash.c | 49 +++--
include/image.h | 5 -
lib/Kconfig | 18 ++
tools/Kconfig | 25 +
4 files changed, 66 insertions(+), 31 deletions(-)
diff --git a/common/hash.c b/common/hash.c
index 6277fe65b3e..0fe65c959d0 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -25,6 +25,7 @@
#else
#include "mkimage.h"
#include
+#include
#endif /* !USE_HOSTCC*/
#include
@@ -41,7 +42,7 @@ DECLARE_GLOBAL_DATA_PTR;
static void reloc_update(void);
-#if defined(CONFIG_SHA1) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA1) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha1(struct hash_algo *algo, void **ctxp)
{
sha1_context *ctx = malloc(sizeof(sha1_context));
@@ -69,7 +70,7 @@ static int hash_finish_sha1(struct hash_algo *algo, void
*ctx, void *dest_buf,
}
#endif
-#if defined(CONFIG_SHA256) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA256) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha256(struct hash_algo *algo, void **ctxp)
{
sha256_context *ctx = malloc(sizeof(sha256_context));
@@ -97,7 +98,7 @@ static int hash_finish_sha256(struct hash_algo *algo, void
*ctx, void
}
#endif
-#if defined(CONFIG_SHA384) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA384) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha384(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
@@ -125,7 +126,7 @@ static int hash_finish_sha384(struct hash_algo *algo, void
*ctx, void
}
#endif
-#if defined(CONFIG_SHA512) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if CONFIG_IS_ENABLED(SHA512) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
static int hash_init_sha512(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
@@ -207,18 +208,13 @@ static int hash_finish_crc32(struct hash_algo *algo, void
*ctx, void *dest_buf,
return 0;
}
-#ifdef USE_HOSTCC
-# define I_WANT_MD51
-#else
-# define I_WANT_MD5CONFIG_IS_ENABLED(MD5)
-#endif
/*
* These are the hash algorithms we support. If we have hardware acceleration
* is enable we will use that, otherwise a software version of the algorithm.
* Note that algorithm names must be in lower case.
*/
static struct hash_algo hash_algo[] = {
-#if I_WANT_MD5
+#if CONFIG_IS_ENABLED(MD5)
{
.name = "md5",
.digest_size= MD5_SUM_LEN,
@@ -226,17 +222,17 @@ static struct hash_algo hash_algo[] = {
.hash_func_ws = md5_wd,
},
#endif
-#ifdef CONFIG_SHA1
+#if CONFIG_IS_ENABLED(SHA1)
{
.name = "sha1",
.digest_size= SHA1_SUM_LEN,
.chunk_size = CHUNKSZ_SHA1,
-#ifdef CONFIG_SHA_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_HW_ACCEL)
.hash_func_ws = hw_sha1,
#else
.hash_func_ws = sha1_csum_wd,
#endif
-#ifdef CONFIG_SHA_PROG_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
.hash_init = hw_sha_init,
.hash_update= hw_sha_update,
.hash_finish= hw_sha_finish,
@@ -247,17 +243,17 @@ static struct hash_algo hash_algo[] = {
#endif
},
#endif
-#ifdef CONFIG_SHA256
+#if CONFIG_IS_ENABLED(SHA256)
{
.name = "sha256",
.digest_size= SHA256_SUM_LEN,
.chunk_size = CHUNKSZ_SHA256,
-#ifdef CONFIG_SHA_HW_ACCEL
+#if CONFIG_IS_ENABLED(SHA_HW_ACCEL)
.hash_func_ws = hw_sha256,
#else
.hash_func_ws = sha256_csum_wd,
#endif
-#ifdef CONFIG_SHA_PROG_HW_ACCEL
+#if
Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support
On 9/23/21 9:49 PM, Simon Glass wrote:> On Thu, 16 Sept 2021 at 09:43, Alex G. wrote: On 7/29/21 8:08 PM, Chia-Wei Wang wrote: + +enum HASH_ALGO hash_algo_lookup_by_name(const char *name) string -> hash_lookup_algo() -> ops struct Is the current way to do things. hash_algo_lookup_by_name() does the roundabout through an enum. That doesn't make sense to me. Actually I'd like to see an enum to describe these, since looking up a string is less efficient in a bootloader. So some way of doing that seems good to me. Of course it means we need a global list of this algos in a header file and each driver needs to indicate which one(s) it supports. Let's assume strcmp() takes a lot of efforts. For a lot of cases, the algo comes in as a string. Think about FIT and image signature verification. So we have to do the strcmp() anyway, making the enum roundabout.
Re: [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT
Hi Oleksandr
On 9/16/21 8:09 AM, Oleksandr Suvorov wrote:
From: Henry Beberman
SPL FIT load checks the signature on loadable images but just continues
in the case of a failure. This is undesirable behavior because the boot
process depends on the authenticity of each loadable part.
Adding CONFIG_SPL_FIT_SIGNATURE_STRICT to halt the platform when any
image fails its signature check, including loadable parts.
This sounds very similar to what FIT config verification already does,
so I don't understand the motivation behind this patch.
SPL already supports image signature verification but had no mechanism
to check that the FIT's configuration block was signed correctly.
This statement is incorrect. This is exactly what required = "conf";
in u-boot's devicetree is intended to do.
NAK
Add a check near the start of spl_load_simple_fit that verifies the
FIT's configuration block, and fails if it's not present or the
signature doesn't match what's stored in the SPL DTB.
Signed-off-by: Henry Beberman
Signed-off-by: Ricardo Salveti
Co-developed-by: Oleksandr Suvorov
Signed-off-by: Oleksandr Suvorov
---
common/Kconfig.boot | 7 +++
common/spl/spl_fit.c | 21 -
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 902a5b8fbea..6f95d009dfa 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -166,6 +166,13 @@ config SPL_FIT_SIGNATURE
select SPL_IMAGE_SIGN_INFO
select SPL_FIT_FULL_CHECK
+config SPL_FIT_SIGNATURE_STRICT
+ bool "Halt if loadables or firmware don't pass FIT signature
verification"
+ select SPL_FIT_SIGNATURE
+ help
+ Strictly requires each loadable or firmware in a FIT image to be
+ passed verification. Halt if any loadable fails to be verified.
+
config SPL_LOAD_FIT
bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
select SPL_FIT
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f41abca0ccb..e7eaaa4cb9e 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -315,7 +315,12 @@ static int spl_load_fit_image(struct spl_load_info *info,
ulong sector,
printf("## Checking hash(es) for Image %s ... ",
fit_get_name(fit, node, NULL));
if (!fit_image_verify_with_data(fit, node, src, length))
- return -EPERM;
+ if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+ puts("Invalid FIT signature found in a required
image.\n");
+ hang();
hang() is rarely the appropriate thing to do. Once you get -EPERM you
could choose to either
a) drop to a lower privilege level
b) enter some sort of recovery mode
c) outright hang.
spl_load_fit_image() isn't the right place to decide (a) or (b), so by
extension, it's the wrong place to decide (c).
+ } else {
+ return -EPERM;
+ }
puts("OK\n");
}
@@ -681,6 +686,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
if (ret < 0)
return ret;
+ if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+ int cfg_noffset = fit_conf_get_node(fit, NULL);
+
+ if (cfg_noffset >= 0) {
+ if (fit_config_verify(fit, cfg_noffset)) {
+ puts("Unable to verify the required FIT
config.\n");
+ hang();
+ }
+ } else {
+ puts("SPL_FIT_SIGNATURE_STRICT needs a config node in
FIT\n");
+ hang();
+ }
+ }
+
/* skip further processing if requested to enable load-only use cases */
if (spl_load_simple_fit_skip_processing())
return 0;
Re: [PATCH 4/4] fit: Use DM hash driver if supported
On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
Calculate hash using DM driver if supported.
For backward compatibility, the call to legacy
hash functions is reserved.
Signed-off-by: Chia-Wei Wang
---
common/image-fit.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/common/image-fit.c b/common/image-fit.c
index d6b2c3c7ec..ec2e526356 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -25,6 +25,10 @@
#include
#include
#include
+#ifdef CONFIG_DM_HASH
+#include
+#include
+#endif
DECLARE_GLOBAL_DATA_PTR;
#endif /* !USE_HOSTCC*/
@@ -1214,6 +1218,31 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp)
int calculate_hash(const void *data, int data_len, const char *algo,
uint8_t *value, int *value_len)
{
+#if !defined(USE_HOSTCC) && defined(CONFIG_DM_HASH)
This file is shared in its entirety with host tools. There isn't a huge
opportunity for using a DM-type approach here without #ifndef USE_HOSTCC.
Alex
+ int rc;
+ enum HASH_ALGO hash_algo;
+ struct udevice *dev;
+
+ rc = uclass_get_device(UCLASS_HASH, 0, );
+ if (rc) {
+ debug("failed to get hash device, rc=%d\n", rc);
+ return -1;
+ }
+
+ hash_algo = hash_algo_lookup_by_name(algo);
+ if (hash_algo == HASH_ALGO_INVALID) {
+ debug("Unsupported hash algorithm\n");
+ return -1;
+ };
+
+ rc = hash_digest_wd(dev, hash_algo, data, data_len, value, CHUNKSZ);
+ if (rc) {
+ debug("failed to get hash value, rc=%d\n", rc);
+ return -1;
+ }
+
+ *value_len = hash_algo_digest_size(hash_algo);
+#else
if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
*((uint32_t *)value) = crc32_wd(0, data, data_len,
CHUNKSZ_CRC32);
@@ -1242,6 +1271,7 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
debug("Unsupported hash alogrithm\n");
return -1;
}
+#endif
return 0;
}
Re: [PATCH 3/4] crypto: hash: Add software hash DM driver
On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
Add purely software-implmented drivers to support multiple
hash operations including CRC, MD5, and SHA family.
This driver is based on the new hash uclass.
Signed-off-by: Chia-Wei Wang
---
drivers/crypto/hash/Kconfig | 11 ++
drivers/crypto/hash/Makefile | 1 +
drivers/crypto/hash/hash_sw.c | 301 ++
3 files changed, 313 insertions(+)
create mode 100644 drivers/crypto/hash/hash_sw.c
diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig
index e226144b9b..cd29a5c6a4 100644
--- a/drivers/crypto/hash/Kconfig
+++ b/drivers/crypto/hash/Kconfig
@@ -3,3 +3,14 @@ config DM_HASH
depends on DM
help
If you want to use driver model for Hash, say Y.
+
+config HASH_SOFTWARE
+ bool "Enable driver for Hash in software"
+ depends on DM_HASH
+ depends on MD5
+ depends on SHA1
+ depends on SHA256
+ depends on SHA512_ALGO
I would have expected a U_BOOT_DRIVER() for each hash algo, rather than
a U_BOOT_DRIVER() wich encompassess all possible algos. If I'm trying to
use SHA256 in SPL, I might not have the room too add SHA1 and MD5, so
I'd have issues using HASH_SOFTWARE, as designed.
diff --git a/drivers/crypto/hash/hash_sw.c b/drivers/crypto/hash/hash_sw.c
new file mode 100644
index 00..fea9d12609
--- /dev/null
+++ b/drivers/crypto/hash/hash_sw.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 ASPEED Technology Inc.
+ * Author: ChiaWei Wang
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* CRC16-CCITT */
+static void hash_init_crc16_ccitt(void *ctx)
+{
+ *((uint16_t *)ctx) = 0;
Undefined behavior: Pointer aliased type-punning. I would suggest using
memset(). Might not be necessarrym as expleined in the next comment.
+static void hash_update_crc16_ccitt(void *ctx, const void *ibuf, uint32_t ilen)
+static void hash_finish_crc16_ccitt(void *ctx, void *obuf)
+/* CRC32 */
+static void hash_init_crc32(void *ctx)
+static void hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen)
+static void hash_finish_crc32(void *ctx, void *obuf)
+/* SHA1 */
+static void hash_init_sha1(void *ctx)
+/* SHA256 */
+/* SHA384 */
+/* SHA512 */
This logic already exists in common/hash.c for hash_Lookup_algo() and
hash_progressive_algo().
Alex
Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support
Hi,
On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
Add UCLASS_HASH for hash driver development. Thus the
hash drivers (SW or HW-accelerated) can be developed
in the DM-based fashion.
Software hashing implementations are shared tightly with host tools.
With DM, there's no opportunity for code sharing with host tools. The
design question that I have is "do we want to DM hashes, or do we want
to DM hardware accelerators for hashes?"
I did some parallel work expose remaining hash algos via
hash_lookup_algo() and hash_progressive_lookup_algo().
Signed-off-by: Chia-Wei Wang
---
drivers/crypto/Kconfig| 2 +
drivers/crypto/Makefile | 1 +
drivers/crypto/hash/Kconfig | 5 ++
drivers/crypto/hash/Makefile | 5 ++
drivers/crypto/hash/hash-uclass.c | 121 ++
include/dm/uclass-id.h| 1 +
include/u-boot/hash.h | 61 +++
7 files changed, 196 insertions(+)
create mode 100644 drivers/crypto/hash/Kconfig
create mode 100644 drivers/crypto/hash/Makefile
create mode 100644 drivers/crypto/hash/hash-uclass.c
create mode 100644 include/u-boot/hash.h
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 1ea116be75..0082177c21 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -1,5 +1,7 @@
menu "Hardware crypto devices"
+source drivers/crypto/hash/Kconfig
+
Hashes are useful outside of cryptographic functions, so it seems odd to
merge them in crypto. For example, CRC32 is not a hash useful in crypto,
but otherwise widely used in u-boot.
[snip]
diff --git a/drivers/crypto/hash/hash-uclass.c
b/drivers/crypto/hash/hash-uclass.c
new file mode 100644
index 00..446eb9e56a
--- /dev/null
+++ b/drivers/crypto/hash/hash-uclass.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 ASPEED Technology Inc.
+ * Author: ChiaWei Wang
+ */
+
+#define LOG_CATEGORY UCLASS_HASH
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct hash_info {
+ char *name;
+ uint32_t digest_size;
+};
+
+static const struct hash_info hash_info[HASH_ALGO_NUM] = {
+ [HASH_ALGO_CRC16_CCITT] = { "crc16-ccitt", 2 },
+ [HASH_ALGO_CRC32] = { "crc32", 4 },
+ [HASH_ALGO_MD5] = { "md5", 16 },
+ [HASH_ALGO_SHA1] = { "sha1", 20 },
+ [HASH_ALGO_SHA256] = { "sha256", 32 },
+ [HASH_ALGO_SHA384] = { "sha384", 48 },
+ [HASH_ALGO_SHA512] = { "sha512", 64},
+};
It seems a step backwards to have to enum {} our hash algos, since we
already identify them by their strings (e.g. "sha256"). and then
associated ops structure. The
+
+enum HASH_ALGO hash_algo_lookup_by_name(const char *name)
string -> hash_lookup_algo() -> ops struct
Is the current way to do things. hash_algo_lookup_by_name() does the
roundabout through an enum. That doesn't make sense to me.
Alex
Re: [PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE
On 9/13/21 4:24 PM, Tom Rini wrote:
In order to move CONFIG_SYS_BAUDRATE_TABLE to Kconfig, we need to rework
the logic a bit. Rename the users of CONFIG_SYS_BAUDRATE_TABLE to
SYS_BAUDRATE_TABLE. Introduce a series of CONFIG_BAUDRATE_TABLE_...
that include some number of baud rates. These match all existing users.
The help for each entry specifies what the exact table is, for a given
option. Define what SYS_BAUDRATE_TABLE will be in include/serial.h now.
Signed-off-by: Tom Rini
---
diff --git a/include/serial.h b/include/serial.h
index 6d1e62c6770c..150644c4c3d4 100644
--- a/include/serial.h
+++ b/include/serial.h
@@ -3,6 +3,42 @@
#include
+#if defined(CONFIG_BAUDRATE_TABLE_300_TO_38400_115200)
+#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 19200, \
+ 38400, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_115200)
+#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 19200, \
+ 38400, 57600, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_230400)
+#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 19200, \
+ 38400, 57600, 115200, 230400 }
+#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_600)
+#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 1800, 2400, 4800, 9600, \
+ 19200, 38400, 57600, 115200, 230400, \
+ 460800, 50, 576000, 921600, 100, \
+ 1152000, 150, 200, 250, \
+ 300, 350, 400, 450, \
+ 500, 550, 600 }
+#elif defined(CONFIG_BAUDRATE_TABLE_4800_TO_115200)
+#define SYS_BAUDRATE_TABLE { 4800, 9600, 19200, 38400, 57600, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_115200)
+#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400)
+#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_460800)
+#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400,
460800 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_921600)
+#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400, \
+ 460800, 921600 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400_50_150)
+#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400, \
+ 50, 150 }
+#elif defined(CONFIG_BAUDRATE_TABLE_38400_115200_ONLY)
+#define SYS_BAUDRATE_TABLE { 38400, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_115200_ONLY)
+#define SYS_BAUDRATE_TABLE { 115200 }
+#endif
+
struct serial_device {
/* enough bytes to match alignment of following func pointer */
charname[16];
This opens the gates to #ifdefing the heck out of serial.h. What happens
to my board that goes from 300 to 200?
* We need a new Kconfig and new ifdef
What happens to my other board that goes from 300 to 250?
* We need a new Kconfig and new ifdef
The pattern doesn't look promising.
I actually think this change can make the situation worse. We trade
having an antiquated and inconvenient SYS_BAUDRATE_TABLE for one Kconfig
per each possible baudrate combination. How does this make sense?
I've seen situations were SPL boots with 2Mbaud and executes
succesfully, u-boot starts up with 2Mbaud just fine. few lines later,
u-boot, downswitches to 115200 because CONFIG_SYS_BAUDRATE_TABLE says so.
Suggestion I: Can we have a MIN/MAX value for baudrates, and have the
code work from there ?
Suggestion II: Define the Kconfig SYS_BAUDRATE_TABLE table to a C array,
like 'default "{ 300, 420, 690}" ' and forego the #ifdefs in serial.h
Suggestion III: Get rid of the logic that says "baudrate must be one of
these predefined values" and let the serial driver return -ENOBUENO or
-EINVAL if the hardware really can't do that baudrate. Most UARTs
nowadays can do a wide range of values, and the baudrate table doesn't
model that very well. Combine this with a CONFIG_MAX_BAUDRATE so that
boards with shitty RS232 converters can set a safe upper limit -- and
make sure CONFIG_BAUDRATE also enforces this.
There's a lot of unrealized potential here.
Alex
Re: [PATCH] Kconfig: Drop duplicate 'select SHA512' instances
On 9/9/21 10:00 AM, Tom Rini wrote:
When dropping SHA512_ALGO in general, we didn't catch some cases where
an option was selecting both SHA512 and SHA512_ALGO and caused them to
select SHA512 twice. Kconfig doesn't complain, but this is still wrong
and should be corrected.
Fixes: e60e44993120 ("lib: Drop SHA512_ALGO in lieu of SHA512")
Reported-by: Andreas Schwab
Signed-off-by: Tom Rini
Reviewed-by: Alexandru Gagniuc
---
lib/crypt/Kconfig | 1 -
lib/efi_loader/Kconfig | 1 -
2 files changed, 2 deletions(-)
diff --git a/lib/crypt/Kconfig b/lib/crypt/Kconfig
index 6a5002964224..a59d5c7d1d54 100644
--- a/lib/crypt/Kconfig
+++ b/lib/crypt/Kconfig
@@ -20,7 +20,6 @@ config CRYPT_PW_SHA256
config CRYPT_PW_SHA512
bool "Provide sha512crypt"
select SHA512
- select SHA512
help
Enables support for the sha512crypt password-hashing algorithm.
The prefix is "$6$".
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 08463251cdf1..edd4ae29f163 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -323,7 +323,6 @@ config EFI_TCG2_PROTOCOL
depends on TPM_V2
select SHA1
select SHA256
- select SHA512
select SHA384
select SHA512
select HASH
Re: [PATCH 1/2] lib: optee: remove the duplicate CONFIG_OPTEE
On 9/6/21 5:39 PM, Alex G. wrote: On 9/6/21 11:53 AM, Patrick DELAUNAY wrote: In fact, the SPL boot path for OP-TEE doesn't use this function. That's intentional. Here's what I suggest: - Remove OPTEE_TZDRAM_BASE and _SIZE There is some legacy here, board/warp7and board/technexion/pico-imx7d. it is not possible, it is used for U-Boot proper on other platforms board/warp7/warp7.c:38: gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE; board/warp7/warp7.c:122: optee_start = optee_end - CONFIG_OPTEE_TZDRAM_SIZE; board/technexion/pico-imx7d/pico-imx7d.c:56: gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE; include/configs/mx7_common.h:52:#if (CONFIG_OPTEE_TZDRAM_SIZE != 0) I have an idea how to work around that. This is what I had in mind [1]. With this, optee_verify_bootm_image() is just three or four checks. I propose that code which would have been under CONFIG_OPTEE_IMAGE, is instead moved under CONFIG_BOOTM_OPTEE. Thus there's no need for CONFIG_OPTEE_IMAGE in the first place, and all the other changes make sense. Alex [1] https://lists.denx.de/pipermail/u-boot/2021-September/459981.html
Re: [PATCH 1/2] lib: optee: remove the duplicate CONFIG_OPTEE
On 9/6/21 11:53 AM, Patrick DELAUNAY wrote:
In fact, the SPL boot path for OP-TEE doesn't use this function. That's
intentional.
Here's what I suggest:
- Remove OPTEE_TZDRAM_BASE and _SIZE
There is some legacy here, board/warp7and board/technexion/pico-imx7d.
it is not possible, it is used for U-Boot proper on other platforms
board/warp7/warp7.c:38: gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE;
board/warp7/warp7.c:122: optee_start = optee_end - CONFIG_OPTEE_TZDRAM_SIZE;
board/technexion/pico-imx7d/pico-imx7d.c:56: gd->ram_size -=
CONFIG_OPTEE_TZDRAM_SIZE;
include/configs/mx7_common.h:52:#if (CONFIG_OPTEE_TZDRAM_SIZE != 0)
I have an idea how to work around that.
And for me this configuration (size of memory used by OPTEE) is more a
system configuration
depending of the OP-TEE firmware used than a Device Tree configuration
at SPL level
PS: for the TF-A case it is done in a secure FW configuration file => in
the FIP
this information is no hardcoded information in BL2
in SPL, the load address / entry point it is already provided by
FIT for OPTEE image
(=> optee_image_get_load_addr / optee_image_get_entry_point)
no need to have this information in DT (optee base address)
tools/default_image.c:119
if (params->os == IH_OS_TEE) {
addr = optee_image_get_load_addr(hdr);
ep = optee_image_get_entry_point(hdr);
}
The OPTEE entry point is available:
1) in both FIT and uImage files.
2) As the optee reserved-memory node in DT
3) Via CONFIG_OPTEE_TZDRAM_BASE
On the one hand, (1) and (2) together could hint that the OPTEE image is
incompatible with the board, so they are not completely redundant.
On the other hand, there is no point in (3) given that the information
could be obtained in at least two other ways.
for CONFIG_OPTEE_TZDRAM_SIZE, I think that can be also found by
parsing the OP-TEE header
=> see : init_mem_usage
the OPTEE should be access to this memory .
and it can change the firewall configuration is it is necessary
for the shared memory for example
=> no need to update first stage boot loader = SPL (with the risk to
brick the device)
when only OP-TEE firmware change
I see your point. It's a packaging issue, which we could solve with FIT,
but not with uImage. Though, how often does an OP-TEE update change the
TZDRAM location?
- Remove optee_verify_bootm_image()
but it is used in
common/bootm_os.c:491: ret =
optee_verify_boot_image(images->os.image_start,
Yes. It only checks if the OP-TEE image fits within some hardcoded, and
potentially wrong, boundaries. Which is contrary to your arguments from
a few paragraphs ago. Just don't call optee_verify_boot_image in bootm_os.c.
Alex
Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
On 9/3/21 10:32 AM, Marek Vasut wrote: On 9/1/21 11:07 AM, Patrick DELAUNAY wrote: On 8/31/21 6:42 PM, Marek Vasut wrote: I would argue that the U-Boot crypto code went through multiple >> independent security reviews, personally I trust that more than code fully controlled and maintained by any one single company, so I am not buying the security constraint argument here. When I talk about security, it is not crypto code, but some security features as isolation between cortex A and Cortex M, key infractructure, trusted environment execution, secure update. I'll offload this answer to Alexandru, however as far as I can tell, SPL is perfectly capable of starting the TEE and setting up the correct execution levels etc. too. There is not a unisex size for security. It's all driven by requirements. My client wants certain pieces of code to be triple-encrypted with a full-body latex suit (and kevlar reinforced). I don't care about A and M separation, clock control and other fancies. In fact it's better that those are handled by linux because that helps with boot time, considerably. I can achieve that by ECDSA-signing the FSBL, and putting that critical code in TEE trusted applications. I don't need to TiVO-ize the device, lock the kernel, etc. Because of that, can incorporate GPL-v3 programs and libraries, and generally have more flexibility in the software architecture. I'm more than happy to deal with the TZC and ETZPC in SPL, because SPL is fast. Again, this is really driven by the totality of requirements. TF-A is a pretty poor solution in this case for a trio of reasons. When I was trying TF-A, I found a small bug in assembly code. I tried to submit a fix, which was immediately rejected. I was required to sign a CLA granting patent rights. I can't do that legally on behalf of my client. This puts TF-A on a take it or leave it basis, such that I can't fix problems with it. TF-A also requires me to use a new file format (FIP). It's a binary format which is already a huge red flag for me. It seems very similar to a concatenation of EFI HOBs and FFS, and it makes no sense why TF-A would not have just gone with those more established formats. Is it secure? I don't know. It's pretty new to start with, and much more inflexible than FIT. Why would I recommend to my client an unproven format, when FIT has already gone through several CVEs and is more widely supported? Definitely not. Then TF-A is one extra codebase in the secure path of the system. Do I want to have to deal with an extra program bringing its own possible flaws on holes? Not really. The official STM wiki seems to document the "how" of security: "Use TF-A and TEE, all others unsupported". It doesn't explain the "why". The "how" is what my client complained it takes two minutes to boot. And those two minutes are the reason I'm working on this. It would help to have better explained options with their corresponding security implications. Even absent that, SPL is perfectly capable of starting a secure system. I think it's also more flexible. Let's get serious. Without SPL, I wouldn't have been able to boot linux in one second, with a secure OS. Alex
Re: [PATCH 1/2] lib: optee: remove the duplicate CONFIG_OPTEE
Hi Patrick
On 9/2/21 4:56 AM, Patrick Delaunay wrote:
The configuration CONFIG_OPTEE is defined 2 times:
1- in lib/optee/Kconfig for support of OPTEE images loaded by bootm command
2- in drivers/tee/optee/Kconfig for support of OP-TEE driver.
It is abnormal to have the same CONFIG define for 2 purpose;
and it is difficult to managed correctly their dependencies.
Moreover CONFIG_SPL_OPTEE is defined in common/spl/Kconfig
to manage OPTEE image load in SPL.
This definition causes an issue with the macro CONFIG_IS_ENABLED(OPTEE)
to test the availability of the OP-TEE driver.
This patch cleans the configuration dependency with:
- CONFIG_OPTEE_IMAGE (renamed) => support of OP-TEE image in U-Boot
- CONFIG_SPL_OPTEE_IMAGE (renamed) => support of OP-TEE image in SPL
- CONFIG_OPTEE (same) => support of OP-TEE driver in U-Boot
- CONFIG_OPTEE_LIB (new) => support of OP-TEE library
After this patch, the macro have the correct behavior:
- CONFIG_IS_ENABLED(OPTEE_IMAGE) => Load of OP-TEE image is supported
- CONFIG_IS_ENABLED(OPTEE) => OP-TEE driver is supported
It seems a little odd to have both OPTEE_LIB and OPTEE_IMAGE, since they
are both used together to support booting with OP-TEE. What also seems
odd is that "OP-TEE driver in U-Boot" does not depend on "OP-TEE library".
Introducing OPTEE_LIB then, makes sense to me, provided that OPTEE
depends on OPTEE_LIB, but I'm not sure about OPTEE_IMAGE.
diff --git a/lib/optee/optee.c b/lib/optee/optee.c
index 672690dc53..5676785cb5 100644
--- a/lib/optee/optee.c
+++ b/lib/optee/optee.c
@@ -20,6 +20,7 @@
"\n\theader lo=0x%08x hi=0x%08x size=0x%08lx arch=0x%08x" \
"\n\tuimage params 0x%08lx-0x%08lx\n"
+#if defined(CONFIG_OPTEE_IMAGE)
int optee_verify_image(struct optee_header *hdr, unsigned long tzdram_start,
unsigned long tzdram_len, unsigned long image_len)
{
@@ -70,6 +71,7 @@ error:
return ret;
}
+#endif
One the idea of having CONFIGs is to include/exclude code via
obj-$(CONFIG_FOO)+=code.c. This prevents the proliferation of #ifdefs.
It's fairly counterintuitive to have a CONFIG_OPTEE_IMAGE in a file
controlled by CONFIG_OPTEE_LIB.
Going to optee_verify_image() itself. It essentially checks against
OPTEE_TZDRAM_(BASE/SIZE). But those are a derived from devicetree, not
Kconfig. So it seems the motivation behing optee_verify_bootm_image() is
flawed. Also the error message is not very helpful.
In fact, the SPL boot path for OP-TEE doesn't use this function. That's
intentional.
Here's what I suggest:
- Remove OPTEE_TZDRAM_BASE and _SIZE
- Remove optee_verify_bootm_image()
- No need for CONFIG_OPTEE_IMAGE
Alex
Re: [PATCH 2/2] board: stm32: Remove the bi_boot_params initialization
On 9/2/21 5:02 AM, Patrick Delaunay wrote:
The stm32 platforms never had to support an ATAGs-based Linux Kernel,
so remove the bi_boot_params initialization.
Signed-off-by: Patrick Delaunay
Tested-by: Alexandru Gagniuc
---
board/dhelectronics/dh_stm32mp1/board.c | 3 ---
board/engicam/stm32mp1/stm32mp1.c| 3 ---
board/st/stm32f429-discovery/stm32f429-discovery.c | 2 --
board/st/stm32f429-evaluation/stm32f429-evaluation.c | 2 --
board/st/stm32f469-discovery/stm32f469-discovery.c | 2 --
board/st/stm32f746-disco/stm32f746-disco.c | 2 --
board/st/stm32h743-disco/stm32h743-disco.c | 1 -
board/st/stm32h743-eval/stm32h743-eval.c | 1 -
board/st/stm32h750-art-pi/stm32h750-art-pi.c | 1 -
board/st/stm32mp1/stm32mp1.c | 3 ---
10 files changed, 20 deletions(-)
diff --git a/board/dhelectronics/dh_stm32mp1/board.c
b/board/dhelectronics/dh_stm32mp1/board.c
index d7c1857c16..765b54a4a4 100644
--- a/board/dhelectronics/dh_stm32mp1/board.c
+++ b/board/dhelectronics/dh_stm32mp1/board.c
@@ -590,9 +590,6 @@ static void board_init_fmc2(void)
/* board dependent setup after realloc */
int board_init(void)
{
- /* address of boot parameters */
- gd->bd->bi_boot_params = STM32_DDR_BASE + 0x100;
-
if (CONFIG_IS_ENABLED(DM_GPIO_HOG))
gpio_hog_probe_all();
diff --git a/board/engicam/stm32mp1/stm32mp1.c b/board/engicam/stm32mp1/stm32mp1.c
index 8bf9c9c67d..20d8603c78 100644
--- a/board/engicam/stm32mp1/stm32mp1.c
+++ b/board/engicam/stm32mp1/stm32mp1.c
@@ -40,9 +40,6 @@ int checkboard(void)
/* board dependent setup after realloc */
int board_init(void)
{
- /* address of boot parameters */
- gd->bd->bi_boot_params = STM32_DDR_BASE + 0x100;
-
if (IS_ENABLED(CONFIG_DM_REGULATOR))
regulators_enable_boot_on(_DEBUG);
diff --git a/board/st/stm32f429-discovery/stm32f429-discovery.c b/board/st/stm32f429-discovery/stm32f429-discovery.c
index 46fcf907fc..5a50e98dd0 100644
--- a/board/st/stm32f429-discovery/stm32f429-discovery.c
+++ b/board/st/stm32f429-discovery/stm32f429-discovery.c
@@ -53,8 +53,6 @@ u32 get_board_rev(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
-
return 0;
}
diff --git a/board/st/stm32f429-evaluation/stm32f429-evaluation.c b/board/st/stm32f429-evaluation/stm32f429-evaluation.c
index 3b6df1f3ab..cf3056163c 100644
--- a/board/st/stm32f429-evaluation/stm32f429-evaluation.c
+++ b/board/st/stm32f429-evaluation/stm32f429-evaluation.c
@@ -47,8 +47,6 @@ u32 get_board_rev(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
-
return 0;
}
diff --git a/board/st/stm32f469-discovery/stm32f469-discovery.c b/board/st/stm32f469-discovery/stm32f469-discovery.c
index c5df9b0d9c..056c9dff2a 100644
--- a/board/st/stm32f469-discovery/stm32f469-discovery.c
+++ b/board/st/stm32f469-discovery/stm32f469-discovery.c
@@ -47,8 +47,6 @@ u32 get_board_rev(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
-
return 0;
}
diff --git a/board/st/stm32f746-disco/stm32f746-disco.c b/board/st/stm32f746-disco/stm32f746-disco.c
index efa38a0e26..2543e2a5f8 100644
--- a/board/st/stm32f746-disco/stm32f746-disco.c
+++ b/board/st/stm32f746-disco/stm32f746-disco.c
@@ -122,8 +122,6 @@ int board_late_init(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
-
#ifdef CONFIG_ETH_DESIGNWARE
const char *phy_mode;
int node;
diff --git a/board/st/stm32h743-disco/stm32h743-disco.c
b/board/st/stm32h743-disco/stm32h743-disco.c
index 4091d5f9fd..e493786f11 100644
--- a/board/st/stm32h743-disco/stm32h743-disco.c
+++ b/board/st/stm32h743-disco/stm32h743-disco.c
@@ -43,6 +43,5 @@ u32 get_board_rev(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
return 0;
}
diff --git a/board/st/stm32h743-eval/stm32h743-eval.c
b/board/st/stm32h743-eval/stm32h743-eval.c
index 4091d5f9fd..e493786f11 100644
--- a/board/st/stm32h743-eval/stm32h743-eval.c
+++ b/board/st/stm32h743-eval/stm32h743-eval.c
@@ -43,6 +43,5 @@ u32 get_board_rev(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
return 0;
}
diff --git a/board/st/stm32h750-art-pi/stm32h750-art-pi.c
b/board/st/stm32h750-art-pi/stm32h750-art-pi.c
index 5785b2e575..bec26465d2 100644
--- a/board/st/stm32h750-art-pi/stm32h750-art-pi.c
+++ b/board/st/stm32h750-art-pi/stm32h750-art-pi.c
@@ -53,6 +53,5 @@ int board_late_init(void)
int board_init(void)
{
- gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
return 0;
}
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index
Re: [PATCH 1/2] arm: stm32: Disable ATAGs support
On 9/2/21 5:02 AM, Patrick Delaunay wrote: These platforms never had to support an ATAGs-based Linux Kernel, so remove the options. Cc: Marek Vasut Signed-off-by: Tom Rini Signed-off-by: Patrick Delaunay Reviewed-by: Alexandru Gagniuc Slowly, but surely, configs/*.h will grow to zero. --- Hi, this patch is a rebased version of the STM32 part [1] of Tom's patchset [2]. I also update the new file include/configs/stm32h750-art-pi.h [1] [13/13] arm: stm32: Disable ATAGs support https://patchwork.ozlabs.org/project/uboot/patch/20210204022415.20589-13-tr...@konsulko.com/ [2] arm: nanopi2: Remove unused code https://patchwork.ozlabs.org/project/uboot/list/?series=227910=* Patrick include/configs/stm32f429-discovery.h | 5 - include/configs/stm32f429-evaluation.h | 5 - include/configs/stm32f469-discovery.h | 5 - include/configs/stm32f746-disco.h | 5 - include/configs/stm32h743-disco.h | 5 - include/configs/stm32h743-eval.h | 5 - include/configs/stm32h750-art-pi.h | 5 - include/configs/stm32mp1.h | 5 - 8 files changed, 40 deletions(-) diff --git a/include/configs/stm32f429-discovery.h b/include/configs/stm32f429-discovery.h index 9d029fbcc6..dbbce49475 100644 --- a/include/configs/stm32f429-discovery.h +++ b/include/configs/stm32f429-discovery.h @@ -27,11 +27,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 /* Timer is clocked at 1MHz */ -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_CBSIZE 1024 #define CONFIG_SYS_MALLOC_LEN (2 << 20) diff --git a/include/configs/stm32f429-evaluation.h b/include/configs/stm32f429-evaluation.h index fefdb2dd15..29a41e8067 100644 --- a/include/configs/stm32f429-evaluation.h +++ b/include/configs/stm32f429-evaluation.h @@ -29,11 +29,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 /* Timer is clocked at 1MHz */ -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_CBSIZE 1024 #define CONFIG_SYS_MALLOC_LEN (1 * 1024 * 1024) diff --git a/include/configs/stm32f469-discovery.h b/include/configs/stm32f469-discovery.h index ba9f05a61b..b9b932c651 100644 --- a/include/configs/stm32f469-discovery.h +++ b/include/configs/stm32f469-discovery.h @@ -29,11 +29,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 /* Timer is clocked at 1MHz */ -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_CBSIZE 1024 #define CONFIG_SYS_MALLOC_LEN (1 * 1024 * 1024) diff --git a/include/configs/stm32f746-disco.h b/include/configs/stm32f746-disco.h index 08d050adfa..b72b989c2c 100644 --- a/include/configs/stm32f746-disco.h +++ b/include/configs/stm32f746-disco.h @@ -36,11 +36,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 /* Timer is clocked at 1MHz */ -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_CBSIZE 1024 #define CONFIG_SYS_MALLOC_LEN (1 * 1024 * 1024) diff --git a/include/configs/stm32h743-disco.h b/include/configs/stm32h743-disco.h index 6e10dbdfe9..e5bb08eec7 100644 --- a/include/configs/stm32h743-disco.h +++ b/include/configs/stm32h743-disco.h @@ -24,11 +24,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_MAXARGS16 #define CONFIG_SYS_MALLOC_LEN (1 * 1024 * 1024) diff --git a/include/configs/stm32h743-eval.h b/include/configs/stm32h743-eval.h index 268d39c7ad..89169f85d5 100644 --- a/include/configs/stm32h743-eval.h +++ b/include/configs/stm32h743-eval.h @@ -24,11 +24,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_MAXARGS16 #define CONFIG_SYS_MALLOC_LEN (1 * 1024 * 1024) diff --git a/include/configs/stm32h750-art-pi.h b/include/configs/stm32h750-art-pi.h index 3fd5461167..a9006e224a 100644 --- a/include/configs/stm32h750-art-pi.h +++ b/include/configs/stm32h750-art-pi.h @@ -24,11 +24,6 @@ #define CONFIG_SYS_HZ_CLOCK 100 -#define CONFIG_CMDLINE_TAG -#define CONFIG_SETUP_MEMORY_TAGS -#define CONFIG_INITRD_TAG -#define CONFIG_REVISION_TAG - #define CONFIG_SYS_MAXARGS16 #define CONFIG_SYS_MALLOC_LEN (1 * 1024 * 1024) diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h index b372838be8..fb4e4fb0bc 100644 --- a/include/configs/stm32mp1.h +++ b/include/configs/stm32mp1.h @@ -33,11 +33,6 @@ #define CONFIG_LOADADDR 0xc200 #define
Re: [PATCH] lib/rsa: Remove support for OpenSSL < 1.1.0 and libressl < 2.7.0
On 9/2/21 12:43 PM, Peter Robinson wrote: On Thu, Sep 2, 2021 at 3:38 PM Tom Rini wrote: On Thu, Sep 02, 2021 at 03:36:43PM +0100, Peter Robinson wrote: On Thu, Sep 2, 2021 at 2:28 PM Tom Rini wrote: On Thu, Jul 29, 2021 at 01:31:21PM -0500, Alexandru Gagniuc wrote: Older OpenSSL and libressl versions have a slightly different API. This require #ifdefs to support. However, we still can't support it because the ECDSA path does not compile with these older versions. These #ifdefs are truly a vestigial appendage. Alternatively, the ECDSA path could be updated for older libraries, but this requires significant extra code, and #ifdefs. Those libraries are over three years old, and there concerns whether it makes sense to build modern software for real world use against such old libraries. Thusly, remove #ifdefs and code for old OpenSSL and LibreSSL support. Signed-off-by: Alexandru Gagniuc Applied to u-boot/next, thanks! According to recent CVE announcements 1.1.0 is out of support [1], does it make sense to just support 1.1.1x and later? [1] https://www.openssl.org/news/secadv/20210824.txt Good question. Are there API changes between 1.1.0 and 1.1.1x ? So outside of the new TLS 1.3 feature the release says "What’s more is that OpenSSL 1.1.1 is API and ABI compliant with OpenSSL 1.1.0" and depending on how we use openssl it may even be API compatible with 3.0 when it comes out any time now. https://www.openssl.org/blog/blog/2018/09/11/release111/ Okay, I don't think it's worth excluding 1.1.0 then. The only way we could do that is a compile time check against OPENSSL_VERSION. That won't prevent someone from compiling with openssl 1.1.1, and then just replacing libcrypto.so with 1.1.0. Alex
Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads
Hi Patrick,
On 9/1/21 10:10 AM, Alex G. wrote:
Hi Patrick,
On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
Hi,
On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
OP-TEE does not take a devicetree for its own use. However, it does
pass the devicetree to the normal world OS. In most cases that will
be some other devicetree-bearing platform, such as linux.
As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
it is required to copy the optee nodes to he target's FDT. Do this as
part of spl_board_prepare_for_optee().
Signed-off-by: Alexandru Gagniuc
---
arch/arm/mach-stm32mp/spl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
index d9fdc5926c..94fbb45cf9 100644
--- a/arch/arm/mach-stm32mp/spl.c
+++ b/arch/arm/mach-stm32mp/spl.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
u32 spl_boot_device(void)
{
@@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
void spl_board_prepare_for_optee(void *fdt)
{
stm32_fdt_setup_mac_addr(fdt);
+ optee_copy_fdt_nodes(fdt);
stm32_init_tzc_for_optee();
}
NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the unsecure
device tree (when CFG_DT is activated)
before to jump to the kernel (that remove the need to have DT
allignement with U-Boot SPL device tree)
=> SPL should not copy the OP-TEE nodes in next stage DT
reference in optee_os = core/arch/arm/kernel/boot.c: add_optee_dt_node()
add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
It is the expected configuration for OP-TEE on STM32MP15 platform.
I agree that if a component modifies the platform, it should be
responsible for updating the devicetree. Two issues though
1. Consistency
The STM32IMAGE boot path expects u-boot to add the optee nodes, while
the FIP path expects OP-TEE to add the nodes. Which one do we stick to?
2. Memory access
I don't understand is how OP-TEE would get past the TZC.
If we look at optee_os => plat-0stm32mp1/plat_tzc400:
"Early boot stage is in charge of configuring memory regions"
The following memory has been set up by SPL (or TF-A for that matter):
Nonsec: c000->ddff
Sec: de00->dfdf
SHMEM: dfe0->dfff
The external DTB will be in the nonsec region, which OP-TEE allegedly
can't access. So how would this get patched?
Let's set aside the above for a bit. I tried the following OP-TEE configs:
"CFG_DT=y CFG_DTB_MAX_SIZE=0x8"
Crashes with 'unhandled pageable abort' somewhere in
core/arch/arm/plat-stm32mp1/ . I don't have much more time to spend
debugging this.
"CFG_DT=y CFG_DTB_MAX_SIZE=0x8 PLATFORM_FLAVOR=157C_DK2"
This adds about 800 ms to the boot time. (power on to kernel printing
the first line). I have an allowance of 1 second to get to the kernel. I
cannot just add 800ms to the boot time and hope nobody notices. However,
calling optee_copy_fdt_nodes() adds no measureable amount of time.
Unless OP-TEE can modify the devicetree without increasing boot time, I
will not go down this path. This is an engineering decision driven by
the product requirements.
That being said, I request that you reconsider your NAK based on this
new data.
Alex
Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads
Hi Patrick,
On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
Hi,
On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
OP-TEE does not take a devicetree for its own use. However, it does
pass the devicetree to the normal world OS. In most cases that will
be some other devicetree-bearing platform, such as linux.
As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
it is required to copy the optee nodes to he target's FDT. Do this as
part of spl_board_prepare_for_optee().
Signed-off-by: Alexandru Gagniuc
---
arch/arm/mach-stm32mp/spl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
index d9fdc5926c..94fbb45cf9 100644
--- a/arch/arm/mach-stm32mp/spl.c
+++ b/arch/arm/mach-stm32mp/spl.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
u32 spl_boot_device(void)
{
@@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
void spl_board_prepare_for_optee(void *fdt)
{
stm32_fdt_setup_mac_addr(fdt);
+ optee_copy_fdt_nodes(fdt);
stm32_init_tzc_for_optee();
}
NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the unsecure
device tree (when CFG_DT is activated)
before to jump to the kernel (that remove the need to have DT
allignement with U-Boot SPL device tree)
=> SPL should not copy the OP-TEE nodes in next stage DT
reference in optee_os = core/arch/arm/kernel/boot.c: add_optee_dt_node()
add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
It is the expected configuration for OP-TEE on STM32MP15 platform.
I agree that if a component modifies the platform, it should be
responsible for updating the devicetree. Two issues though
1. Consistency
The STM32IMAGE boot path expects u-boot to add the optee nodes, while
the FIP path expects OP-TEE to add the nodes. Which one do we stick to?
2. Memory access
I don't understand is how OP-TEE would get past the TZC.
If we look at optee_os => plat-0stm32mp1/plat_tzc400:
"Early boot stage is in charge of configuring memory regions"
The following memory has been set up by SPL (or TF-A for that matter):
Nonsec: c000->ddff
Sec:de00->dfdf
SHMEM: dfe0->dfff
The external DTB will be in the nonsec region, which OP-TEE allegedly
can't access. So how would this get patched?
Alex
Re: [PATCH 07/10] stm32mp1: spl: Configure MAC address when booting OP-TEE
On 8/31/21 12:10 PM, Patrick DELAUNAY wrote:
Hi,
On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
When OP-TEE is booted as the SPL payload, the stage after OP-TEE is
not guaranteed to be u-boot. Thus the FDT patching in u-boot is not
guaranteed to occur. Add this step to SPL.
The patching by stm32_fdt_setup_mac_addr() is done in SPL, and patches
the target FDT directly. This differs is different from
setup_mac_address(), which sets the "ethaddr" env variable, and does
not work in SPL.
Signed-off-by: Alexandru Gagniuc
---
arch/arm/mach-stm32mp/cpu.c | 22 +++
.../arm/mach-stm32mp/include/mach/sys_proto.h | 3 +++
arch/arm/mach-stm32mp/spl.c | 1 +
3 files changed, 26 insertions(+)
diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index 8727de513c..2b8b67bb40 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -646,6 +647,27 @@ __weak int setup_mac_address(void)
return 0;
}
+int stm32_fdt_setup_mac_addr(void *fdt)
+{
+ int ret;
+ uchar enetaddr[ARP_HLEN];
+
+ ret = stm32_read_otp_mac(enetaddr);
+ if (ret < 0)
+ return ret;
+
+ if (!is_valid_ethaddr(enetaddr)) {
+ printf("invalid MAC address in OTP\n");
+ return -EINVAL;
+ }
+
+ ret = fdt_ethernet_set_macaddr(fdt, 0, enetaddr);
+ if (ret)
+ debug("Failed to set mac address from OTP: %d\n", ret);
+
+ return ret;
+}
+
static int setup_serial_number(void)
{
char serial_string[25];
diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
index 4149d3a133..2d24cfee3f 100644
--- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
+++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
@@ -47,7 +47,10 @@ void get_soc_name(char name[SOC_NAME_SIZE]);
/* return boot mode */
u32 get_bootmode(void);
+/* Set 'ethaddr' env variable with MAC from OTP (useful for u-boot
proper) */
int setup_mac_address(void);
+/* Patch the first 'ethernet' node of FDT with MAC from OTP (useful
for SPL) */
+int stm32_fdt_setup_mac_addr(void *fdt);
/* board power management : configure vddcore according OPP */
void board_vddcore_init(u32 voltage_mv);
diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
index 405eff68a3..d9fdc5926c 100644
--- a/arch/arm/mach-stm32mp/spl.c
+++ b/arch/arm/mach-stm32mp/spl.c
@@ -181,6 +181,7 @@ void stm32_init_tzc_for_optee(void)
void spl_board_prepare_for_optee(void *fdt)
{
+ stm32_fdt_setup_mac_addr(fdt);
stm32_init_tzc_for_optee();
}
I think that all this part is not required for falcon mode:
let the Linux driver fallback to nvmem access to read the MAC address in
OTP when the DT is not updated by boot loader
remove also the patch 06/10 and 04/40
PS: it is already working in OpenSTLinux based on the device tree
modification for your board
{
ethernet_mac_address: mac@e4 {
reg = <0xe4 0x6>;
};
{
nvmem-cells = <_mac_address>;
nvmem-cell-names = "mac-address";
};
I was looking at this, following Sean's suggestion from last week. The
linux BSEC driver backends on stm32_bsec_smc(). This requires a
corresponding secure-world callout. which is not currently implemented
for SPL. We'd just get "Can't read data57 (-5)" error from linux, and a
random MAC address.
Is this change as proposed ideal? Probably not. It works with SPL and
the current LTS linux (v5.10), and is likely the least intensive
solution in terms of lines of code.
Alex
PS: this part is not yet upstreamed in Linux
That's not problematic. I can accommodate devicetree changes by using
overlays in the FIT image. This flexibility is also a huge reason why
I've chosen to go with FIT versus other image formats.
Re: [PATCH 04/10] fdt_support: Implement fdt_ethernet_set_macaddr()
Hi Sean,
On 8/26/21 6:35 PM, Sean Anderson wrote:
On 8/26/21 5:42 PM, Alexandru Gagniuc wrote:
Oftentimes we have MAC address information stored in a ROM or OTP. The
way to add that to the FDT would be through the u-boot environment,
and then fdt_fixup_ethernet(). This is not very useful in SPL.
It would be more helpful to be able to "set interface x to MAC y".
This is where fdt_ethernet_set_macaddr() comes in. It is similar in
function to fdt_fixup_ethernet(), but only updates one interface,
without using the u-boot env, and without string processing.
Have you considered adopting the nvmem-cells property for ethernet
controllers (added in Linux commit 0e839df92cf3 ("net: ethernet: provide
nvmem_get_mac_address()"))?
Obviously I haven't. It sounds like a great idea. Thank you for pointing
me to it.
Alex
--Sean
Signed-off-by: Alexandru Gagniuc
---
common/fdt_support.c | 30 ++
include/fdt_support.h | 17 +
2 files changed, 47 insertions(+)
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 4341d84bd5..c4cbd4060e 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -592,6 +592,36 @@ void fdt_fixup_ethernet(void *fdt)
}
}
+int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t
*mac_addr)
+{
+ const char *path, *name;
+ int prop, aliases_node;
+ char eth_name[16] = "ethernet";
+
+ aliases_node = fdt_path_offset(fdt, "/aliases");
+ if (aliases_node < 0)
+ return aliases_node;
+
+ if (ethnum >= 0)
+ sprintf(eth_name, "ethernet%d", ethnum);
+
+ fdt_for_each_property_offset(prop, fdt, aliases_node) {
+ path = fdt_getprop_by_offset(fdt, prop, , NULL);
+ if (!strcmp(name, eth_name))
+ break;
+
+ path = NULL;
+ }
+
+ if (!path)
+ return -FDT_ERR_NOTFOUND;
+
+ do_fixup_by_path(fdt, path, "mac-address", mac_addr, 6, 0);
+ do_fixup_by_path(fdt, path, "local-mac-address", mac_addr, 6, 1);
+
+ return 0;
+}
+
int fdt_record_loadable(void *blob, u32 index, const char *name,
uintptr_t load_addr, u32 size, uintptr_t entry_point,
const char *type, const char *os, const char *arch)
diff --git a/include/fdt_support.h b/include/fdt_support.h
index f6f46bb8e9..3f0bcb5a00 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -119,6 +119,23 @@ static inline int fdt_fixup_memory_banks(void
*blob, u64 start[], u64 size[],
#endif
void fdt_fixup_ethernet(void *fdt);
+
+/**
+ * Set the "mac-address" and "local-mac-address" of ethernet node
+ * The ethernet node is located from the "/aliases" section of the
fdt. When
+ * 'ethnum' is positive, then the name is matched exactly, e.g
"ethernet0".
+ * When ethnum is negative, the first ethernet alias is updated.
+ * Unlike fdt_fixup_ethernet(), this function only updates one
ethernet node,
+ * and soes not use the "ethaddr" from the u-boot environment. This
is useful,
+ * for example, in SPL, when the environment is not initialized or
available.
+ *
+ * @param fdt FDT blob to update
+ * @param ethnum Ethernet device index, or negative for any ethernet
+ * @param mac_addr Pointer to 6-byte array containing the MAC address
+ *
+ * @return 0 if ok, or -FDT_ERR_... on error
+ */
+int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t
*mac_addr);
int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
const void *val, int len, int create);
void fdt_fixup_qe_firmware(void *fdt);
Re: U-Boot ECDSA Software Implementation Status
Hi Ryan, I'm only aware of the work that Tim has published. I don't think anyone is actively working on it. Alex On 8/26/21 9:00 AM, Pabis, Ryan wrote: I see that Tim was working to add a non-platform specific implementation of the ECDSA algorithm to u-boot back in February. I would like to use this feature as well and was wondering if this work has been completed and where I can find the patch. Thanks, Ryan
Massive stm32mp1 breakage with v2021.10-rc2
Hi Patrick,
I'm having issues with some of the recent changes centered around FIP
support and CONFIG_STM32MP15x_STM32IMAGE. and commit f91783edf224 ("arm:
stm32mp: handle the OP-TEE nodes in DT with FIP support")
## Problem description
> +#ifdef CONFIG_STM32MP15x_STM32IMAGE
> + /* only needed for boot with TF-A, witout FIP support */
> firmware {
> optee {
> compatible = "linaro,optee-tz";
> @@ -33,6 +35,7 @@
> no-map;
> };
> };
> +#endif
This removes the "optee" and "reserved-memory" nodes. These nodes are
required by SPL for setting up TZC memory regions, as introduced in
commit 8533263c8512 ("stm32mp1: spl: Configure TrustZone controller for
OP-TEE").
## Further details
We now have several boot flows:
1) BL1 -> SPL -> u-boot
2) BL1 -> SPL -> OP-TEE (this path is now broken)
3) BL1 -> TF-A -> u-boot (use case for STM32IMAGE)
4) BL1 -> TF-A -> OP-TEE (use case for STM32IMAGE)
5) BL1 -> TF-A -> FIP container
So it seems that STM32IMAGE only makes sense for (3) and (4), but
shouldn't affect the others. The fact that OP-TEE is mixed into this is
the first red flag.
> INPUTS-$(CONFIG_STM32MP15x_STM32IMAGE) += u-boot.stm32
> MKIMAGEFLAGS_u-boot.stm32 = -T stm32image ...
This tells me we use _STM32IMAGE just to enable another output image
format. The build could give me both u-boot.img, and u-boot.stm32, and I
would use the correct file. SO in this case, I would expect _STM32IMAGE
to not change the code behavior. This is the second red flag.
> $ git grep -c 'ifdef CONFIG_STM32MP15x_STM32IMAGE'
> arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c:1
> arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c:2
> arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h:1
> arch/arm/mach-stm32mp/include/mach/stm32prog.h:1
We're actually trying to move away from ifdefs, so this intense reliance
on _STM32IMAGE raises the third red flag.
> board/st/common/stm32mp_mtdparts.c:9
I'm not sure I fully understand why there are so many ifdefs in
mtdparts.c. MTD layout seems to me like something that is intended to be
devicetree-driven. This is the fourth red flag.
Let's take a deeper look at one of those:
> stm32mp_mtdparts.c:#ifdef CONFIG_STM32MP15x_STM32IMAGE
> stm32mp_mtdparts.c: tee = >
stm32prog_get_tee_partitions();
> stm32mp_mtdparts.c-#endif
This makes no sense to me. What does OP-TEE presence have to do with the
image format of u-boot? If TF-A requires a different layout in FIP vs
non-FIP mode, then it's the responsibility of TF-A to supply a corrected
devicetree to u-boot. It's not u-boot's role.
## Proposal
I propose we remove CONFIG_STM32MP15x_STM32IMAGE. Always build
u-boot.stm32, and don't mix it with code behavior.
Alex
A mea culpa undefined reference in v2021.10-rc2, padding_algos, linker lists
Hi Simon,
I'm seeing an undefined reference to padding_pkcs_15_verify with
v2021.10-rc2. It happens when enabling FIT_SIGNATURE. I've tracked it
down to the following two commits:
commit 92c960bc1d ("lib: rsa: Remove #ifdefs from rsa.h")
commit 61416fe9df ("Kconfig: FIT_SIGNATURE should not select RSA_VERIFY")
Individually, each commit is fine, but when put together, they cause the
issue, as the static inline padding_pkcs_15_verify() implementation is
removed from rsa.h.
My hypothesis is that moving padding_algos to a linker list will solve
this specific problem. I realize you might be working on the same part
of the code. Should I address this issue, or should I wait for your series?
Alex
Re: [PATCH] mkimage: use environment variable MKIMAGE_SIGN_PIN to set pin for OpenSSL Engine
Hi Marc,
On 7/23/21 3:17 PM, Marc Kleine-Budde wrote:
This patch adds the possibility to pass the PIN the OpenSSL Engine
used during signing via the environment variable MKIMAGE_SIGN_PIN.
This follows the approach used during kernel module
signing ("KBUILD_SIGN_PIN") or UBIFS image
signing ("MKIMAGE_SIGN_PIN").
I think the preferred approach would have been to add a flag to mkimage,
similar to "-N => openssl engine to use for signing". Environment
variables are rarely used to talk to tools, and not very intuitive. I
can only really think of NO_SDL=1 being used by u-boot.
Since this patch already made it, I hope you have the bandwidth to look
at converting this to a mkimage flag.
Alex
Signed-off-by: Marc Kleine-Budde
---
doc/uImage.FIT/signature.txt | 4 ++--
lib/rsa/rsa-sign.c | 11 +++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/doc/uImage.FIT/signature.txt b/doc/uImage.FIT/signature.txt
index 7cb1c15e5e15..61a72db3c74f 100644
--- a/doc/uImage.FIT/signature.txt
+++ b/doc/uImage.FIT/signature.txt
@@ -533,8 +533,8 @@ Generic engine key ids:
or
""
-As mkimage does not at this time support prompting for passwords HSM may need
-key preloading wrapper to be used when invoking mkimage.
+In order to set the pin in the HSM, an environment variable "MKIMAGE_SIGN_PIN"
+can be specified.
The following examples use the Nitrokey Pro using pkcs11 engine. Instructions
for other devices may vary.
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index f4ed11e74a4a..49abec6c83fb 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -338,6 +338,7 @@ static int rsa_init(void)
static int rsa_engine_init(const char *engine_id, ENGINE **pe)
{
+ const char *key_pass;
ENGINE *e;
int ret;
@@ -362,10 +363,20 @@ static int rsa_engine_init(const char *engine_id, ENGINE **pe)
goto err_set_rsa;
}
+ key_pass = getenv("MKIMAGE_SIGN_PIN");
+ if (key_pass) {
+ if (!ENGINE_ctrl_cmd_string(e, "PIN", key_pass, 0)) {
+ fprintf(stderr, "Couldn't set PIN\n");
+ ret = -1;
+ goto err_set_pin;
+ }
+ }
+
*pe = e;
return 0;
+err_set_pin:
err_set_rsa:
ENGINE_finish(e);
err_engine_init:
Re: [PATCH 1/1] lib/ecdsa: Fix LibreSSL before v2.7.0
Hi Artem On 7/29/21 9:52 AM, Artem Panfilov wrote: On 29.07.2021 15:59, Tom Rini wrote: Well yes, this is part of the question now, is there enough interest in the old version to bother with? The other part of the question is what's being built now that wasn't being built before, and is that a bug or a feature (a less CONFIG-dependent set of tools is good for generic distributions). OK, if someone else will report the same issue after u-boot release, then it should be fixed. Currently, I am okay with my local fix by disabling the CONFIG_TOOLS_LIBCRYPTO option. ECDSA signing was not verified against a libcrypto that old. Given that signatures are non-deterministic, I doubt we could have a CI test that says old-libcrypto, known block must equal known signature. When we added ECDSA, there was not a need to consider old libcrypto versions, but I also did not pay attention to the #ifdefs in the much older RSA path. I'm sorry that you had to go through the frustrations of getting a patch rejected which does something the codebase already does. I am going to take a look at cleaning up the RSA path. There's no point in maintaining backwards compatibility if we're not doing it across the board. Alex
Re: [PATCH 1/1] lib/ecdsa: Fix LibreSSL before v2.7.0
Hi Artem,
I'm re-adding the u-boot mailing list to the CC field, as I see your
email contains no sensitive information.
On 7/28/21 2:30 PM, Artem Panfilov wrote:
We have broken CI builds on your bare-metal CentOS 7 servers with latest
master. I think it is good reason to have a support. Our corporate
clients have CentOS 7 too.
I thought we solved problems associated with bare builds on ancient OSes
by using containerized builds. There's is CI infrastructure based on
this on source.denx.de that uses docker. As things change, and we
eventually move to GNU TLS, a lot more things might break for old build
hosts. I believe it's worth looking at containerized builds.
Given that a solution exists for your problem, I think the argument for
this patch quite weak.
There is no nice way to handle openssl difference. You could check other
commits related to openssl compatibility. They all looks ugly.
Another solution is to disable CONFIG_TOOLS_LIBCRYPTO by default that
broke our builds.
Do you need cryptographic features in mkimage? If not just disable
TOOLS_LIBCRYPTO in your builds.
Alex
Best regards,
Artem
ср, 28 июл. 2021 г., 22:16 Alex G. <mailto:mr.nuke...@gmail.com>>:
On 7/28/21 1:10 PM, Artem Panfilov wrote:
> Fix LibreSSL compilation for versions before v2.7.0.
>
> Fix following compilation issue when CONFIG_TOOLS_LIBCRYPTO is
enabled:
> tools/lib/ecdsa/ecdsa-libcrypto.o: In function `prepare_ctx':
> ecdsa-libcrypto.c:(.text+0x94): undefined reference to
> `OPENSSL_init_ssl'
> ecdsa-libcrypto.c:(.text+0x148): undefined reference to
> `EC_GROUP_order_bits'
> tools/lib/ecdsa/ecdsa-libcrypto.o: In function
> `ecdsa_check_signature.isra.0':
> ecdsa-libcrypto.c:(.text+0x32c): undefined reference to
`ECDSA_SIG_set0'
> tools/lib/ecdsa/ecdsa-libcrypto.o: In function `ecdsa_sign':
> ecdsa-libcrypto.c:(.text+0x42c): undefined reference to
`ECDSA_SIG_get0'
> ecdsa-libcrypto.c:(.text+0x443): undefined reference to
`BN_bn2binpad'
> ecdsa-libcrypto.c:(.text+0x455): undefined reference to
`BN_bn2binpad'
> tools/lib/ecdsa/ecdsa-libcrypto.o: In function
`ecdsa_add_verify_data':
> ecdsa-libcrypto.c:(.text+0x5fa): undefined reference to
> `EC_GROUP_order_bits'
> ecdsa-libcrypto.c:(.text+0x642): undefined reference to
> `EC_POINT_get_affine_coordinates'
>
> Signed-off-by: Artem Panfilov mailto:panfilov.art...@gmail.com>>
> ---
> lib/ecdsa/ecdsa-libcrypto.c | 80
-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ecdsa/ecdsa-libcrypto.c
b/lib/ecdsa/ecdsa-libcrypto.c
> index 1757a14562..50aa093acd 100644
> --- a/lib/ecdsa/ecdsa-libcrypto.c
> +++ b/lib/ecdsa/ecdsa-libcrypto.c
> @@ -24,6 +24,70 @@
> #include
> #include
>
> +#if OPENSSL_VERSION_NUMBER < 0x1010L || \
> + (defined(LIBRESSL_VERSION_NUMBER) &&
LIBRESSL_VERSION_NUMBER < 0x0207fL)
Is there a reasonable use case for supporting an external library that
is more than three years old at this point?
Otherwise NAK, as such #ifdefs don't really help with readability. I
think Simon will agree here.
There's also the issue of deciding what version we have at compile
time,
which ignores the dynamic linking nature of .so libs. This leads into
soname versioning territory. Let's not go there.
Alex
> +#include
> +
> +static int EC_GROUP_order_bits(const EC_GROUP *group)
> +{
> + int ret = 0;
> + BIGNUM *order;
> +
> + if (!group)
> + return ret;
> +
> + order = BN_new();
> +
> + if (!order) {
> + ERR_clear_error();
> + return ret;
> + }
> +
> + if (!EC_GROUP_get_order(group, order, NULL)) {
> + ERR_clear_error();
> + BN_free(order);
> + return ret;
> + }
> +
> + ret = BN_num_bits(order);
> + BN_free(order);
> + return ret;
> +}
> +
> +static void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM
**pr, const BIGNUM **ps)
> +{
> + if (pr != NULL)
> + *pr = sig->r;
> + if (ps != NULL)
> + *ps = sig->s;
> +}
> +
> +static int ECDSA_SIG_set0(ECDSA_SIG *sig, BIGNUM *r, BIGNUM *s)
> +{
> + if (r == NULL || s == NULL)
> + return 0;
> + BN_clear_free(sig->r);
> + BN_clea
Re: [PATCH 1/1] lib/ecdsa: Fix LibreSSL before v2.7.0
On 7/28/21 1:10 PM, Artem Panfilov wrote:
Fix LibreSSL compilation for versions before v2.7.0.
Fix following compilation issue when CONFIG_TOOLS_LIBCRYPTO is enabled:
tools/lib/ecdsa/ecdsa-libcrypto.o: In function `prepare_ctx':
ecdsa-libcrypto.c:(.text+0x94): undefined reference to
`OPENSSL_init_ssl'
ecdsa-libcrypto.c:(.text+0x148): undefined reference to
`EC_GROUP_order_bits'
tools/lib/ecdsa/ecdsa-libcrypto.o: In function
`ecdsa_check_signature.isra.0':
ecdsa-libcrypto.c:(.text+0x32c): undefined reference to `ECDSA_SIG_set0'
tools/lib/ecdsa/ecdsa-libcrypto.o: In function `ecdsa_sign':
ecdsa-libcrypto.c:(.text+0x42c): undefined reference to `ECDSA_SIG_get0'
ecdsa-libcrypto.c:(.text+0x443): undefined reference to `BN_bn2binpad'
ecdsa-libcrypto.c:(.text+0x455): undefined reference to `BN_bn2binpad'
tools/lib/ecdsa/ecdsa-libcrypto.o: In function `ecdsa_add_verify_data':
ecdsa-libcrypto.c:(.text+0x5fa): undefined reference to
`EC_GROUP_order_bits'
ecdsa-libcrypto.c:(.text+0x642): undefined reference to
`EC_POINT_get_affine_coordinates'
Signed-off-by: Artem Panfilov
---
lib/ecdsa/ecdsa-libcrypto.c | 80 -
1 file changed, 79 insertions(+), 1 deletion(-)
diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c
index 1757a14562..50aa093acd 100644
--- a/lib/ecdsa/ecdsa-libcrypto.c
+++ b/lib/ecdsa/ecdsa-libcrypto.c
@@ -24,6 +24,70 @@
#include
#include
+#if OPENSSL_VERSION_NUMBER < 0x1010L || \
+ (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <
0x0207fL)
Is there a reasonable use case for supporting an external library that
is more than three years old at this point?
Otherwise NAK, as such #ifdefs don't really help with readability. I
think Simon will agree here.
There's also the issue of deciding what version we have at compile time,
which ignores the dynamic linking nature of .so libs. This leads into
soname versioning territory. Let's not go there.
Alex
+#include
+
+static int EC_GROUP_order_bits(const EC_GROUP *group)
+{
+ int ret = 0;
+ BIGNUM *order;
+
+ if (!group)
+ return ret;
+
+ order = BN_new();
+
+ if (!order) {
+ ERR_clear_error();
+ return ret;
+ }
+
+ if (!EC_GROUP_get_order(group, order, NULL)) {
+ ERR_clear_error();
+ BN_free(order);
+ return ret;
+ }
+
+ ret = BN_num_bits(order);
+ BN_free(order);
+ return ret;
+}
+
+static void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const
BIGNUM **ps)
+{
+ if (pr != NULL)
+ *pr = sig->r;
+ if (ps != NULL)
+ *ps = sig->s;
+}
+
+static int ECDSA_SIG_set0(ECDSA_SIG *sig, BIGNUM *r, BIGNUM *s)
+{
+ if (r == NULL || s == NULL)
+ return 0;
+ BN_clear_free(sig->r);
+ BN_clear_free(sig->s);
+ sig->r = r;
+ sig->s = s;
+ return 1;
+}
+
+int BN_bn2binpad(const BIGNUM *a, unsigned char *to, int tolen)
+{
+ int n = BN_num_bytes(a);
+
+ if (n < 0 || n > tolen)
+ return -1;
+
+ memset(to, 0, tolen - n);
+ if (BN_bn2bin(a, to + tolen - n) < 0)
+ return -1;
+
+ return tolen;
+}
+#endif
+
/* Image signing context for openssl-libcrypto */
struct signer {
EVP_PKEY *evp_key; /* Pointer to EVP_PKEY object */
@@ -34,9 +98,18 @@ struct signer {
static int alloc_ctx(struct signer *ctx, const struct image_sign_info *info)
{
+ int ret = 0;
+
memset(ctx, 0, sizeof(*ctx));
- if (!OPENSSL_init_ssl(0, NULL)) {
+#if OPENSSL_VERSION_NUMBER < 0x1010L || \
+(defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x0207fL)
+ ret = SSL_library_init();
+#else
+ ret = OPENSSL_init_ssl(0, NULL);
+#endif
+
+ if (!ret) {
fprintf(stderr, "Failure to init SSL library\n");
return -1;
}
@@ -285,7 +358,12 @@ static int do_add(struct signer *ctx, void *fdt, const
char *key_node_name)
x = BN_new();
y = BN_new();
point = EC_KEY_get0_public_key(ctx->ecdsa_key);
+#if OPENSSL_VERSION_NUMBER < 0x1010L || \
+(defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x0207fL)
+ EC_POINT_get_affine_coordinates_GFp(group, point, x, y, NULL);
+#else
EC_POINT_get_affine_coordinates(group, point, x, y, NULL);
+#endif
ret = fdt_setprop_string(fdt, key_node, "ecdsa,curve", curve_name);
if (ret < 0)
Re: [PATCH v3 19/19] tools: Use a single target-independent config to enable OpenSSL
On 7/27/21 4:59 AM, Heiko Thiery wrote: Hi all, Am Do., 15. Juli 2021 um 00:09 Uhr schrieb Alexandru Gagniuc : Host tool features, such as mkimage's ability to sign FIT images were enabled or disabled based on the target configuration. However, this misses the point of a target-agnostic host tool. A target's ability to verify FIT signatures is independent of mkimage's ability to create those signatures. In fact, u-boot's build system doesn't sign images. The target code can be successfully built without relying on any ability to sign such code. Conversely, mkimage's ability to sign images does not require that those images will only work on targets which support FIT verification. Linking mkimage cryptographic features to target support for FIT verification is misguided. Without loss of generality, we can say that host features are and should be independent of target features. While we prefer that a host tool always supports the same feature set, we recognize the following - some users prefer to build u-boot without a dependency on OpenSSL. - some distros prefer to ship mkimage without linking to OpenSSL To allow these use cases, introduce a host-only Kconfig which is used to select or deselect libcrypto support. Some mkimage features or some host tools might not be available, but this shouldn't affect the u-boot build. I also considered setting the default of this config based on FIT_SIGNATURE. While it would preserve the old behaviour it's also contrary to the goals of this change. I decided to enable it by default, so that the default build yields the most feature-complete mkimage. Signed-off-by: Alexandru Gagniuc Since this patch was applied to master the build target "flash.bin" for e.g. the imx8mq_evk_defconfig fails. --- 8< --- MKIMAGE u-boot.itb u-boot.its:7.11-15.5: Warning (unit_address_vs_reg): /images/uboot@1: node has a unit name, but no reg property u-boot.its:16.9-21.5: Warning (unit_address_vs_reg): /images/fdt@1: node has a unit name, but no reg property u-boot.its:22.9-31.5: Warning (unit_address_vs_reg): /images/atf@1: node has a unit name, but no reg property u-boot.its:36.12-41.5: Warning (unit_address_vs_reg): /configurations/config@1: node has a unit name, but no reg property ./tools/mkimage: verify_header failed for FIT Image support with exit code 1 make: *** [Makefile:1440: u-boot.itb] Error 1 make: *** Deleting file 'u-boot.itb' make: *** Waiting for unfinished jobs --- 8< --- Does I miss here something? Are you sure it's this patch? I don't see how this change affects this issue, but I did notice invalid FIT node names [1] in your build. Alex [1] https://source.denx.de/u-boot/u-boot/-/commit/3f04db891a353f4b127ed57279279f851c6b4917
Re: [PATCH v5 0/5] stm32mp: Enable OP-TEE and TZC support in SPL
On 7/15/21 2:19 PM, Alexandru Gagniuc wrote: v4 branch was reported to have some issues with SPL becoming too big on some platforms (e.g. imx6dl_mamoj) This is fixed by dropping the call to genimg_get_os_name(). Ping for merge window. Alexandru Gagniuc (5): spl: mmc: Support OP-TEE payloads in Falcon mode spl: Introduce spl_board_prepare_for_optee() hook arm: stm32mp: Implement support for TZC 400 controller stm32mp1: spl: Configure TrustZone controller for OP-TEE ARM: dts: stm32mp: Add OP-TEE reserved memory to SPL dtb arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi | 3 + arch/arm/mach-stm32mp/Makefile | 1 + arch/arm/mach-stm32mp/include/mach/tzc.h | 33 ++ arch/arm/mach-stm32mp/spl.c | 92 +++ arch/arm/mach-stm32mp/tzc400.c | 136 +++ common/spl/spl.c | 5 + common/spl/spl_mmc.c | 4 +- include/spl.h| 14 +++ 8 files changed, 286 insertions(+), 2 deletions(-) create mode 100644 arch/arm/mach-stm32mp/include/mach/tzc.h create mode 100644 arch/arm/mach-stm32mp/tzc400.c
Re: [PATCH v4 1/5] spl: mmc: Support OP-TEE payloads in Falcon mode
On 7/15/21 1:27 PM, Patrick DELAUNAY wrote: Hi, [snip] When I merge this patch on master branch, I get the error: arm: + imx6dl_mamoj +spl/u-boot-spl.bin exceeds file size limit: + limit: 0xefa0 bytes + actual: 0xf41d bytes + excess: 0x47d bytes +make[1]: *** [Makefile:1997: spl/u-boot-spl.bin] Error 1 +make[1]: *** Deleting file 'spl/u-boot-spl.bin' +make: *** [Makefile:177: sub-make] Error 2 This issue need to be solved before I accept and merge the serie. Okay, I'll have to drop the call to genimg_get_os_name(). But I think again about the title of this patch : - spl: mmc: Support OP-TEE payloads in Falcon mode If I unterstood after the serie the sequence for MMC is: ROM code => SPL => TEE (as raw OS) => U-Boot but it is not really the Falcon mode and OP-TEE + falcon mode is not really supported... And the patch title is disturbing. For me the correct sequence is in Falcon mode is : ROM code => SPL => TEE (secure world) => kernel (normal world) This is exactly the use case that this patch intends to support. With the TEE and the kernel loaded by the SPL.. and without falcon mode : (A) ROM code => SPL => TEE (secure world) => U-Boot or (B) ROM code => SPL (TZ) => U-Boot (TZ) execute bootm => TEE (secure world) => kernel what it your expected sequence in spl_load_simple_fit in this serie / in your defconfig ? Today with the normal world address can be: 1/ = spl_image->entry_point (bootm_os.c in U-Boot proprer) 2/ = CONFIG_SYS_TEXT_BASE (hardcoded for SPL in spl_optee.S) for 2/ When U-Boot is not used after SPL = in falcon mode, the LR register shoud be set to kernel entry point. How does SPL know where OP-TEE should jump to? One could parse the FIT image, and try to figure out which of the loadables is the kernel. But what if there's a linux and u-boot, with different "/configurations" nodes? Figuring out where OP-TEE wants to start the normal world is a hard problem that the u=boot infrastructure is not prepared for. The solution I'm using is to build OP-TEE with CFG_NS_ENTRY_ADDR=[linux entry addr] Then LR is irrelevant. Alex
Re: [PATCH] spl: Align device tree blob address at 8-byte boundary
On 7/13/21 1:11 PM, Tom Rini wrote:
On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
On 7/13/21 6:47 PM, Simon Glass wrote:
Hi Marek,
On Tue, 13 Jul 2021 at 08:53, Marek Vasut wrote:
On 7/13/21 4:41 PM, Tom Rini wrote:
On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
On 7/13/21 3:47 PM, Tom Rini wrote:
On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
On 7/12/21 10:15 AM, Tom Rini wrote:
On Mon, Jul 12, 2021 at 01:36:14PM +0800, Bin Meng wrote:
On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle wrote:
I submitted an almost identical patch. See
https://github.com/u-boot/u-boot/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76
This patch eventually had to be reverted
(https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16fd114),
because it was causing issues on some platforms that had FIT on 32 bit
boundary. However I continue to use it in production code, as without it the
boot on my platform aborts.
I don't have time to investigate why this was happening, but you need to check
this code won't just cause exactly the same faults.
Thanks for your information.
+Marek who did the revert
The revert commit message says:
"The commit breaks booting of fitImage by SPL, the system simply
hangs. This is because on arm32, the fitImage and all of its content
can be aligned to 4 bytes and U-Boot expects just that."
I don't understand this. If an address is aligned to 8, it is already
aligned to 4, so how did this commit make the system hang on arm32?
I think this had something to do with embedding contents somewhere in
the image? There is a thread on the ML from then but I don't know how
informative it will end up being.
It's true that the flat devicetree spec requires an 8-byte alignment, even
on 32-bit. The issues here are specific to u-boot.
SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
two cases:
1) u-boot as a FIT (binary and FDT separately loaded)
2) u-boot with embedded FDT
In case (1) SPL must place the FDT at a location where u-boot will find it.
The current logic is
SPL:fdt = ALIGN_4(u_boot + u_boot_size)
u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
In case (2), SPL's view of the FDT is not relevant, but instead the build
system must place the FDT correctly:
build: fdt >> u-boot.bin
u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
We have 3 places that must agree. A correct and complete patch could change
all three, but one has to consider compatibility issues when crossing u-boot
and SPL versions.
I had proposed in the revert discussion that SPL use r2 or similar mechanism
to pass the location of the FDT to u-boot.
I'm not sure that we need to worry too much about mix-and-match
SPL/U-Boot, but documenting what to go change if you must do it
somewhere under doc/ would be good. I think we can just switch to
ALIGN(8) not ALIGN(4) and be done with it?
Remember, there is also falcon boot. And we definitely have to be able to
have old u-boot (SPL) boot new fitImage and vice versa.
I don't follow you, sorry. But since you seem to have the best
understanding of where all of the cases something could go wrong here,
can you perhaps post an RFC patch? That is likely to be clearer than
another long thread here.
I don't follow you, sorry. I believe the revert did the right thing and
new systems should use mkimage -E when generating fitImages, to avoid
the string alignment problem. That is all.
Using -E should be optional and things really should work without it.
See the DTSpec, I don't think that is possible unless you relocate fitImage
components, and if you want fast boot time esp. in SPL, that is not good.
This is why I've asked you to make up some patch to perhaps highlight
the problem. Ensuring that the device tree, which is small, is also
8-byte aligned, shouldn't be a big problem nor performance hit. I'm not
sure where the problem case is that isn't "user put things they control
in a bad spot, fail and tell them why" but I could just be missing a
case.
As far as highlighting the problem, here's an excerpt from the previous
discussion [1].
## SPL:
image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
(gdb) print/x (spl_image->load_addr + spl_image->size)
$19 = 0xc01cf85c
(gdb) print/x image_info->load_addr
$20 = 0xc01cf860
FDT is installed at 0xc01cf860
## u-boot:
__weak void *board_fdt_blob_setup(void)
{
/* FDT is at end of image */
fdt_blob = (ulong *)&_end;
(gdb) print &_end
$22 = (char (*)[]) 0xc01cf85c
FDT is expected at 0xc01cf85c
Alex
[1] https://lists.denx.de/pipermail/u-boot/2020-October/430066.html
Re: [PATCH] spl: Align device tree blob address at 8-byte boundary
On 7/13/21 3:35 PM, Marek Vasut wrote: On 7/13/21 8:11 PM, Tom Rini wrote: On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote: On 7/13/21 6:47 PM, Simon Glass wrote: Hi Marek, On Tue, 13 Jul 2021 at 08:53, Marek Vasut wrote: On 7/13/21 4:41 PM, Tom Rini wrote: On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote: On 7/13/21 3:47 PM, Tom Rini wrote: On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote: On 7/12/21 10:15 AM, Tom Rini wrote: On Mon, Jul 12, 2021 at 01:36:14PM +0800, Bin Meng wrote: On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle wrote: I submitted an almost identical patch. See https://github.com/u-boot/u-boot/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76 This patch eventually had to be reverted (https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16fd114), because it was causing issues on some platforms that had FIT on 32 bit boundary. However I continue to use it in production code, as without it the boot on my platform aborts. I don't have time to investigate why this was happening, but you need to check this code won't just cause exactly the same faults. Thanks for your information. +Marek who did the revert The revert commit message says: "The commit breaks booting of fitImage by SPL, the system simply hangs. This is because on arm32, the fitImage and all of its content can be aligned to 4 bytes and U-Boot expects just that." I don't understand this. If an address is aligned to 8, it is already aligned to 4, so how did this commit make the system hang on arm32? I think this had something to do with embedding contents somewhere in the image? There is a thread on the ML from then but I don't know how informative it will end up being. It's true that the flat devicetree spec requires an 8-byte alignment, even on 32-bit. The issues here are specific to u-boot. SPL and u-boot have to agree where u-boot's FDT is located. We'll look at two cases: 1) u-boot as a FIT (binary and FDT separately loaded) 2) u-boot with embedded FDT In case (1) SPL must place the FDT at a location where u-boot will find it. The current logic is SPL: fdt = ALIGN_4(u_boot + u_boot_size) u-boot: fdt = ALIGN_4(u_boot + u_boot_size) In case (2), SPL's view of the FDT is not relevant, but instead the build system must place the FDT correctly: build: fdt >> u-boot.bin u-boot: fdt = ALIGN_4(u_boot + u_boot_size) We have 3 places that must agree. A correct and complete patch could change all three, but one has to consider compatibility issues when crossing u-boot and SPL versions. I had proposed in the revert discussion that SPL use r2 or similar mechanism to pass the location of the FDT to u-boot. I'm not sure that we need to worry too much about mix-and-match SPL/U-Boot, but documenting what to go change if you must do it somewhere under doc/ would be good. I think we can just switch to ALIGN(8) not ALIGN(4) and be done with it? Remember, there is also falcon boot. And we definitely have to be able to have old u-boot (SPL) boot new fitImage and vice versa. I don't follow you, sorry. But since you seem to have the best understanding of where all of the cases something could go wrong here, can you perhaps post an RFC patch? That is likely to be clearer than another long thread here. I don't follow you, sorry. I believe the revert did the right thing and new systems should use mkimage -E when generating fitImages, to avoid the string alignment problem. That is all. Using -E should be optional and things really should work without it. See the DTSpec, I don't think that is possible unless you relocate fitImage components, and if you want fast boot time esp. in SPL, that is not good. This is why I've asked you to make up some patch to perhaps highlight the problem. Ensuring that the device tree, which is small, is also 8-byte aligned, shouldn't be a big problem nor performance hit. I'm not sure where the problem case is that isn't "user put things they control in a bad spot, fail and tell them why" but I could just be missing a case. The fail case is this: - you update SPL with this 8 byte alignment change - you have older kernel fitImage with embedded DT for falcon mode - system no longer boots because there is off-by-4 error in the DT address passed to the kernel I'm not sure how falcon mode would break the kernel. It passes to the kernel the load address of the fdt. The concern here is loading u-boot. I hope this is clear now.
Re: [PATCH] spl: Align device tree blob address at 8-byte boundary
On 7/12/21 10:15 AM, Tom Rini wrote: On Mon, Jul 12, 2021 at 01:36:14PM +0800, Bin Meng wrote: On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle wrote: I submitted an almost identical patch. See https://github.com/u-boot/u-boot/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76 This patch eventually had to be reverted (https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16fd114), because it was causing issues on some platforms that had FIT on 32 bit boundary. However I continue to use it in production code, as without it the boot on my platform aborts. I don't have time to investigate why this was happening, but you need to check this code won't just cause exactly the same faults. Thanks for your information. +Marek who did the revert The revert commit message says: "The commit breaks booting of fitImage by SPL, the system simply hangs. This is because on arm32, the fitImage and all of its content can be aligned to 4 bytes and U-Boot expects just that." I don't understand this. If an address is aligned to 8, it is already aligned to 4, so how did this commit make the system hang on arm32? I think this had something to do with embedding contents somewhere in the image? There is a thread on the ML from then but I don't know how informative it will end up being. It's true that the flat devicetree spec requires an 8-byte alignment, even on 32-bit. The issues here are specific to u-boot. SPL and u-boot have to agree where u-boot's FDT is located. We'll look at two cases: 1) u-boot as a FIT (binary and FDT separately loaded) 2) u-boot with embedded FDT In case (1) SPL must place the FDT at a location where u-boot will find it. The current logic is SPL:fdt = ALIGN_4(u_boot + u_boot_size) u-boot: fdt = ALIGN_4(u_boot + u_boot_size) In case (2), SPL's view of the FDT is not relevant, but instead the build system must place the FDT correctly: build: fdt >> u-boot.bin u-boot: fdt = ALIGN_4(u_boot + u_boot_size) We have 3 places that must agree. A correct and complete patch could change all three, but one has to consider compatibility issues when crossing u-boot and SPL versions. I had proposed in the revert discussion that SPL use r2 or similar mechanism to pass the location of the FDT to u-boot. Alex
Re: [PATCH v4 0/5] stm32mp: Enable OP-TEE and TZC support in SPL
On 7/6/21 10:45 AM, Tom Rini wrote: On Tue, Jul 06, 2021 at 10:18:44AM -0500, Alex G. wrote: On 5/31/21 12:43 PM, Alexandru Gagniuc wrote: The purpose of this series is to allow booting an OP-TEE image from SPL, by corectly configuring the TrustZone (TZC) memory regions. Any chance we could have this hit the merge window? For clarity, does this series depend on anything that's not already merged? Negative. This series can be applied standalone. Alex
Re: [PATCH v4 0/5] stm32mp: Enable OP-TEE and TZC support in SPL
On 5/31/21 12:43 PM, Alexandru Gagniuc wrote: The purpose of this series is to allow booting an OP-TEE image from SPL, by corectly configuring the TrustZone (TZC) memory regions. Any chance we could have this hit the merge window? Alex
Re: [PATCH] tools: Use a single target-independent config to enable OpenSSL
On 6/22/21 8:31 AM, Simon Glass wrote: [snip] +config TOOLS_USE_LIBCRYPTO would HOST_LIBCRYPTO be better? I had considered a shorter kconfig such as the above. Does it mean (1) The build host has libcrypto available? (2) We use the libcrypto on the host? (3) There is a libcrypto for the target? "tools" is a subset of what we do on the host so it seems more concrete. I realize you could read "tools use libcrypto" as a statement rather than a question, which would seem odd in kconfig. As far as having a verb, I thought it to be important because there isn't a substantially similar use of Kconfig in u-boot. I didn't want the name to be too vague. Hope this clears things up, Alex
Re: Boeing Engineer - Help Needed (URGENT)
Ni Nelson, On 6/17/21 2:21 PM, Su (US), Nelson Z wrote: Hi U-Boot devs, I am an engineer working for Boeing and I need assistance with getting a MicroChip VSC6803 API (https://github.com/microchip-ung/mesa) to work on their VSC7429 Ethernet Switch. From their API, we have a MFI image file built to load onto the Switch and this was instructed to be done with U-Boot. The problem is we have a board with a blank NOR chip, so we have no starting image to even get U-Boot up. How would we get an image with U-Boot onto our NOR chip? We have SPI connector for SPI NOR flash. Our board is a Luton26. We have no idea how to get U-Boot loaded onto our chip. I read on the U-Boot project's README that U-Boot can be installed in a boot ROM, but I have no idea how to do that. Although I am not familiar with this particular chip, it sounds like you might need a hardware flasher, like a DediProg. Please help. This is extremely time critical. I presume you don't have a plane going down right now. You'll be fine, and we'll do what we can to point you in the right direction. Alex Thanks, Nelson Su
Re: [PATCH] tools: Use a single target-independent config to enable OpenSSL
On 6/15/21 6:34 PM, AKASHI Takahiro wrote: A gentle ping. What is the current review status? Who will take care of this patch? Patchwork automatically delegates this to a maintainer [1], but anyone is welcome to comment and review. Alex [1] https://patchwork.ozlabs.org/project/uboot/patch/20210524202317.1492578-1-mr.nuke...@gmail.com/ -Takahiro Akashi On Mon, May 24, 2021 at 03:23:17PM -0500, Alexandru Gagniuc wrote: Host tool features, such as mkimage's ability to sign FIT images were enabled or disabled based on the target configuration. However, this misses the point of a target-agnostic host tool. A target's ability to verify FIT signatures is independent of mkimage's ability to create those signatures. In fact, u-boot's build system doesn't sign images. The target code can be successfully built without relying on any ability to sign such code. Conversely, mkimage's ability to sign images does not require that those images will only work on targets which support FIT verification. Linking mkimage cryptographic features to target support for FIT verification is misguided. Without loss of generality, we can say that host features are and should be independent of target features. While we prefer that a host tool always supports the same feature set, we recognize the following - some users prefer to build u-boot without a dependency on OpenSSL. - some distros prefer to ship mkimage without linking to OpenSSL To allow these use cases, introduce a host-only Kconfig which is used to select or deselect libcrypto support. Some mkimage features or some host tools might not be available, but this shouldn't affect the u-boot build. I also considered setting the default of this config based on FIT_SIGNATURE. While it would preserve the old behaviour it's also contrary to the goals of this change. I decided to enable it by default, so that the default build yields the most feature-complete mkimage. Signed-off-by: Alexandru Gagniuc --- This patch is designed to apply on top of [PATCH v2 00/18] image: Reduce #ifdef abuse in image code tools/Kconfig | 11 +++ tools/Makefile | 46 ++ 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/tools/Kconfig b/tools/Kconfig index b2f5012240..214932ae30 100644 --- a/tools/Kconfig +++ b/tools/Kconfig @@ -9,4 +9,15 @@ config MKIMAGE_DTC_PATH some cases the system dtc may not support all required features and the path to a different version should be given here. +config TOOLS_USE_LIBCRYPTO + bool "Use OpenSSL's libcrypto library for host tools" + default y + help + Cryptographic signature, verification, and encryption of images is + provided by host tools using OpenSSL's libcrypto. Select 'n' here if + you wish to build host tools without OpenSSL. mkimage will not have + the ability to sign images. + This selection does not affect target features, such as runtime FIT + signature verification. + endmenu diff --git a/tools/Makefile b/tools/Makefile index 722355e984..1f30a06cce 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -3,6 +3,25 @@ # (C) Copyright 2000-2006 # Wolfgang Denk, DENX Software Engineering, w...@denx.de. +# A note on target vs host configuration: +# +# Host tools can be used across multiple targets, or different configurations +# of the same target. Thus, host tools must be able to handle any combination +# of target configurations. To prevent having different variations of the same +# tool, the tool build options may not depend on target configuration. +# +# Some linux distributions package these utilities as u-boot-tools, and it +# would be unmaintainable to have a different tool variation for each +# arch or configuration. +# +# A couple of simple rules: +# +# 1) Do not use target CONFIG_* options to enable or disable features in host +#tools. Only use the configs from tools/Kconfig +# 2) It's okay to use target configs to disable building specific tools. +#That's as long as the features of those tools aren't modified. +# + # Enable all the config-independent tools ifneq ($(HOST_TOOLS_ALL),) CONFIG_ARCH_KIRKWOOD = y @@ -53,30 +72,30 @@ hostprogs-y += mkenvimage mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o hostprogs-y += dumpimage mkimage -hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign +hostprogs-$(CONFIG_TOOLS_USE_LIBCRYPTO) += fit_info fit_check_sign hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o common/image-fit-sig.o -FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o +FIT_SIG_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := image-sig-host.o common/image-fit-sig.o +FIT_CIPHER_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := common/image-cipher.o # The
Re: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
On 4/24/21 2:43 AM, Lim, Elly Siew Chin wrote: Add this discussion to denx mailing list. [snip] I can think of two enhancement to fix this: (1) Add separate CONFIG to gate ECDSA algorithm. This enhancement benefits all use cases. I assume not all user need ECDSA algorithm when FIT_SIGNATURE is used. (2) Enhance spl/spl_fit.c to support verification of data integrity based on hash(es) in FIT image instead of based on FIT_SIGNATURE. What do you think? If you agree: For (1), can we ask Alex's help to change it? For (2), who will be the right person to change this kind of common code? FYI, I proposed a change to decouple OpenSSL from FIT_SIGNATURE [1] [1] https://patchwork.ozlabs.org/project/uboot/patch/20210524202317.1492578-1-mr.nuke...@gmail.com/ That would enable you to have FIT_SIGNATURE, but not need OpenSSL support in mkimage. Alex
Re: [PATCH v3 4/5] stm32mp1: spl: Configure TrustZone controller for OP-TEE
On 5/28/21 5:22 AM, Patrick DELAUNAY wrote: Hi, On 4/15/21 6:48 PM, Alexandru Gagniuc wrote: [snip] + fdt_start = ofnode_get_addr_size(node, "reg", size); warning here because size is 'fdt_size_t *' not 'u32*' arch/arm/mach-stm32mp/spl.c:122:48: warning: passing argument 3 of ‘ofnode_get_addr_size’ from inc Fixed in v4 + + tzc_configure(tzc, optee_config); + tzc_dump_config(tzc); Dump is always require, even for nomal boot, or only for debug cases ? Because tzc_dump_config() uses log_info(), only prints when the appropriate log level is enabled. This is by design, so that we don't need extra logic here to invoke dump(). + + dcache_disable(); You disable cache why, it is not supported by OP-TEE ? => if it is a generic issue it should be in spl.c for case IH_OS_TEE or in spl_optee_entry() and not in board specific weak function as it done in bl31_entry for TF-A I don't know if this can be safely generalized. stm32mp is the only platform to enable TZC in SPL, which brings special constraints: We're running in secure mode, and we've touched memory that will be reserved to the normal world, for example the linux devicetree. Once we enable TZC, we can't touch that memory anymore. This could happen later as the CPU is evicting cache lines. To make sure we don't hit a TZC violation as cache lines are evicted, we both flush the dcache, and disable the dcache. Doing this another way is not tested, and I can't guarantee that other variations will work reliably. Alex
Re: [PATCH v3 3/5] arm: stm32mp: Implement support for TZC 400 controller
Hi Patrick, On 5/28/21 4:59 AM, Patrick DELAUNAY wrote: Hi, Any reason to prefer uint16_t and uint32_t ? I use standard C types. u32, _u32, and __u32 are non-standard, and they have different meanings depending on how they're used. I avoid them for this reason. Alex See checkpatch warning arch/arm/mach-stm32mp/include/mach/tzc.h:24: check: Prefer kernel type 'u16' over 'uint16_t' arch/arm/mach-stm32mp/include/mach/tzc.h:25: check: Prefer kernel type 'u16' over 'uint16_t' arch/arm/mach-stm32mp/tzc400.c:41: check: Prefer kernel type 'u16' over 'uint16_t' arch/arm/mach-stm32mp/tzc400.c:52: check: Prefer kernel type 'u32' over 'uint32_t' arch/arm/mach-stm32mp/tzc400.c:81: check: Prefer kernel type 'u32' over 'uint32_t' arch/arm/mach-stm32mp/tzc400.c:82: check: Prefer kernel type 'u32' over 'uint32_t' arch/arm/mach-stm32mp/tzc400.c:93: check: Prefer kernel type 'u32' over 'uint32_t' arch/arm/mach-stm32mp/tzc400.c:94: check: Prefer kernel type 'u32' over 'uint32_t' arch/arm/mach-stm32mp/tzc400.c:113: check: Prefer kernel type 'u32' over 'uint32_t' But except these remarks: Reviewed-by: Patrick Delaunay Thanks Patrick
Re: [PATCH v9 1/1] efi_loader: add PE/COFF image measurement
On 5/25/21 10:09 PM, Masahisa Kojima wrote:
"TCG PC Client Platform Firmware Profile Specification"
requires to measure every attempt to load and execute
a OS Loader(a UEFI application) into PCR[4].
This commit adds the PE/COFF image measurement, extends PCR,
and appends measurement into Event Log.
Acked-by: Ilias Apalodimas
Tested-by: Ilias Apalodimas
Signed-off-by: Masahisa Kojima
---
Changes in v9:
- use original return code from __get_active_pcr_banks()
- return EFI_UNSUPPORTED instead of EFI_INVALID_PARAMETER
if efi_image_parse() fails, it complies with TCG spec
- remove **new_efi parameter from efi_prepare_aligned_image()
to improve the readability
(no changes since v7)
Changes in v7:
- include hash-checksum.h instead of rsa.h
- select HASH_CALCULATE in Kconfig, not to update lib/Makefile
I want to remove HASH_CALCULATE for Kconfig for reasons outlined in (1):
(1)
https://patchwork.ozlabs.org/project/uboot/patch/20210524192857.1486696-2-mr.nuke...@gmail.com/
The root of the problem is that selecting SHA_xxx should compile and
link the hash_calculate() symbol, and this would make the existing
kconfig correct. Unfortunately, the selection doesn't happen
automatically because the SHA code isn't too well organized.
To solve your problem, I would prefer that you take the series in (1) --
there's a second patch after it -- and use "select HASH" here. You're
asking "What's the difference ?". The difference is that "HASH" is an
existing Kconfig symbol, so we don't need to also add "HASH_CALULATE".
Alex
- rebased the base code
Changes in v6:
- update lib/Makefile to add hash-checksum.c as a compilation target
(no changes since v2)
Changes in v2:
- Remove duplicate include
- Remove unnecessary __packed attribute
- Add all EV_EFI_* event definition
- Create common function to prepare 8-byte aligned image
- Add measurement for EV_EFI_BOOT_SERVICES_DRIVER and
EV_EFI_RUNTIME_SERVICES_DRIVER
- Use efi_search_protocol() to get device_path
- Add function comment
include/efi_loader.h | 6 +
include/efi_tcg2.h| 9 ++
include/tpm-v2.h | 18 +++
lib/efi_loader/Kconfig| 1 +
lib/efi_loader/efi_image_loader.c | 62 ++---
lib/efi_loader/efi_tcg2.c | 207 --
6 files changed, 277 insertions(+), 26 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 522696d635..0a9c82a257 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void);
efi_status_t efi_rng_register(void);
/* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
efi_status_t efi_tcg2_register(void);
+/* measure the pe-coff image, extend PCR and add Event Log */
+efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
+ struct efi_loaded_image_obj *handle,
+ struct efi_loaded_image *loaded_image_info);
/* Create handles and protocols for the partitions of a block device */
int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
const char *if_typename, int diskid,
@@ -886,6 +890,8 @@ bool efi_secure_boot_enabled(void);
bool efi_capsule_auth_enabled(void);
+void *efi_prepare_aligned_image(void *efi, u64 *efi_size);
+
bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
WIN_CERTIFICATE **auth, size_t *auth_len);
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index 40e241ce31..bcfb98168a 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -9,6 +9,7 @@
#if !defined _EFI_TCG2_PROTOCOL_H_
#define _EFI_TCG2_PROTOCOL_H_
+#include
#include
#define EFI_TCG2_PROTOCOL_GUID \
@@ -53,6 +54,14 @@ struct efi_tcg2_event {
u8 event[];
} __packed;
+struct uefi_image_load_event {
+ efi_physical_addr_t image_location_in_memory;
+ u64 image_length_in_memory;
+ u64 image_link_time_address;
+ u64 length_of_device_path;
+ struct efi_device_path device_path[];
+};
+
struct efi_tcg2_boot_service_capability {
u8 size;
struct efi_tcg2_version structure_version;
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 7de7d6a57d..247b386967 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -70,6 +70,24 @@ struct udevice;
#define EV_TABLE_OF_DEVICES ((u32)0x000B)
#define EV_COMPACT_HASH ((u32)0x000C)
+/*
+ * event types, cf.
+ * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
+ * rev 1.04, June 3, 2019
+ */
+#define EV_EFI_EVENT_BASE ((u32)0x8000)
+#define EV_EFI_VARIABLE_DRIVER_CONFIG ((u32)0x8001)
+#define EV_EFI_VARIABLE_BOOT ((u32)0x8002)
+#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x8003)
+#define EV_EFI_BOOT_SERVICES_DRIVER
Re: [PATCH RFC 1/2] Revert "lib: introduce HASH_CALCULATE option"
On 5/26/21 11:06 AM, Heinrich Schuchardt wrote: On 5/24/21 9:28 PM, Alexandru Gagniuc wrote: When we think of Kconfig, we usually think of features that we like to enable or not. Ideally, we wouldn't use Kconfig to fix a build issue, although sometimes it might make sense. With Kconfig it's hard to guarantee that the fix is universal. We can only say that it works for the set of tested configurations. In the majority of cases, it's preferable to let the linker figure things out for us. The reverted commit attempted to fix a build issue by adding an invisible Kconfig option. This is wrong in several ways: It invents a new Kconfig variable when CONFIG_HASH already exists for the same purpose. Second, hash-checksum.c makes use of the hash_progressive_lookup_algo() symbol, which is only provided with CONFIG_HASH, but this dependency was not expressed in the reverted patch. It feels like Kconfig is turning into a listing of all available source files, and a buffet to 'select' which ones to compile. The purpose of this revert is to enable the next change to make use of CONFIG_HASH instead of adding to Kconfig. See upcoming patch efi_loader: add PE/COFF image measurement https://patchwork.ozlabs.org/project/uboot/patch/20210526030958.15701-2-masahisa.koj...@linaro.org/ Here we need to compile hash-checksum.o, but don't need FIT image support. You can take the nest patch in this series and "select HASH". Alex Best regards Heinrich This reverts commit 87316da05f2fd49d3709275e64ef0c5980366ade. Signed-off-by: Alexandru Gagniuc --- common/Kconfig.boot| 1 - lib/Kconfig| 3 --- lib/Makefile | 2 +- lib/efi_loader/Kconfig | 2 -- 4 files changed, 1 insertion(+), 7 deletions(-) diff --git a/common/Kconfig.boot b/common/Kconfig.boot index 3c6e77d099..89a3161f1f 100644 --- a/common/Kconfig.boot +++ b/common/Kconfig.boot @@ -80,7 +80,6 @@ config FIT_SIGNATURE select RSA_VERIFY select IMAGE_SIGN_INFO select FIT_FULL_CHECK - select HASH_CALCULATE help This option enables signature verification of FIT uImages, using a hash signed and verified using RSA. If diff --git a/lib/Kconfig b/lib/Kconfig index d675ab1d82..15019d2c65 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -439,9 +439,6 @@ config CRC32C config XXHASH bool -config HASH_CALCULATE - bool - endmenu menu "Compression Support" diff --git a/lib/Makefile b/lib/Makefile index 0835ea292c..6825671955 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -61,7 +61,7 @@ endif obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/ obj-$(CONFIG_$(SPL_)MD5) += md5.o obj-$(CONFIG_$(SPL_)RSA) += rsa/ -obj-$(CONFIG_HASH_CALCULATE) += hash-checksum.o +obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o obj-$(CONFIG_SHA1) += sha1.o obj-$(CONFIG_SHA256) += sha256.o obj-$(CONFIG_SHA512_ALGO) += sha512.o diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index eb5c4d6f29..c259abe033 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -174,7 +174,6 @@ config EFI_CAPSULE_AUTHENTICATE select PKCS7_MESSAGE_PARSER select PKCS7_VERIFY select IMAGE_SIGN_INFO - select HASH_CALCULATE default n help Select this option if you want to enable capsule @@ -343,7 +342,6 @@ config EFI_SECURE_BOOT select X509_CERTIFICATE_PARSER select PKCS7_MESSAGE_PARSER select PKCS7_VERIFY - select HASH_CALCULATE default n help Select this option to enable EFI secure boot support.
Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
On 5/21/21 2:39 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 18:07, Alex G. wrote:
On 5/20/21 6:17 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote:
From: Simon Glass
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1)
directly in the code shared with the host build, so we can drop the
unnecessary indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
Signed-off-by: Alexandru Gagniuc
---
common/image-fit.c | 2 +-
include/image.h| 8
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe3..24e92a8e92 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
CHUNKSZ_CRC32);
*((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
*value_len = 4;
- } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
+ } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise
this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not
in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define
IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why
I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how
we can get hash algos in linker lists, like we're proposing to do for
crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to
be going to extreme lengths to avoid it, but it seems like the
simplest solution, easy to understand, no effect on code size and
scalable to the future.
It's easy for the short term in terms if the goal is to get something
merged. It just hides more fundamental issues with the code. For
ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
Because one resets the watchdog after every CHUNK bytes and the other
doesn't?
Well hash_calculate() is used for hashing parts of a devicetree, so is
quite a different function.
I was under the impression that we were agreed on the combination of
patches. I won't try to defend your patch from yourself. I'll drop the
hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I
think we are going to end up with a Kconfig solution at some point.
What do you think?
I think it's possible and reasonable to have common code without host
Kconfig. coreboot did it.
There is very little code shared between target and tools there. I am
sure there is some, but can't think of anything except some library
functions. There is also no equivalent of CONFIG_IS_ENABLED(), but
instead a log of ENV_... macros and entirely separate rules in the
Makefile.
Can you point to another way to do this? I think your approach is
valuable in untangling code that does not need to be shared, but I
still think that the host Kconfig thing is a great idea for everything
else. It isn't my idea, but Rasmus' (now on cc).
I have a couple of ideas to start, but nothing submittable.
Let's start with hash_calculate(). It's just a big switch() with string
processing. But we've already done part of the work in
fit_image_check_hash(). So instead of stopping at a "char *algo", why
not get an actual "struct hash_algo *" with the correct ops to begin
with, and not need a huge switch() function() ?
We have "struct hash_algo" and "struct checksum_algo" that seem to serve
very similar purposes. Would it actually make sense to merge them?
And if that is done, you open the gates to significantly reducing the
CONFIG_IS_ENABLED() use for hashes, as well as really simplify the hash
selection in Kconfig.
In order to do this, we are reducing the occurrence of IS_ENABLED(),
which is just an eye-candy version of #ifdef. This leads to the natural
conclusion of eliminating IS_ENABLED() from common code.
Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
On 5/20/21 6:17 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote:
From: Simon Glass
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1)
directly in the code shared with the host build, so we can drop the
unnecessary indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
Signed-off-by: Alexandru Gagniuc
---
common/image-fit.c | 2 +-
include/image.h| 8
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe3..24e92a8e92 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
CHUNKSZ_CRC32);
*((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
*value_len = 4;
- } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
+ } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise
this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not
in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define
IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why
I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how
we can get hash algos in linker lists, like we're proposing to do for
crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to
be going to extreme lengths to avoid it, but it seems like the
simplest solution, easy to understand, no effect on code size and
scalable to the future.
It's easy for the short term in terms if the goal is to get something
merged. It just hides more fundamental issues with the code. For
ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
Because one resets the watchdog after every CHUNK bytes and the other
doesn't?
I was under the impression that we were agreed on the combination of
patches. I won't try to defend your patch from yourself. I'll drop the
hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I
think we are going to end up with a Kconfig solution at some point.
What do you think?
I think it's possible and reasonable to have common code without host
Kconfig. coreboot did it.
Alex
Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote:
From: Simon Glass
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1)
directly in the code shared with the host build, so we can drop the
unnecessary indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
Signed-off-by: Alexandru Gagniuc
---
common/image-fit.c | 2 +-
include/image.h| 8
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe3..24e92a8e92 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
CHUNKSZ_CRC32);
*((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
*value_len = 4;
- } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
+ } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise
this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not
in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define
IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why
I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how
we can get hash algos in linker lists, like we're proposing to do for
crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to
be going to extreme lengths to avoid it, but it seems like the
simplest solution, easy to understand, no effect on code size and
scalable to the future.
It's easy for the short term in terms if the goal is to get something
merged. It just hides more fundamental issues with the code. For
ecample, why is there hash_calculate() and clacultae_hash()
I was under the impression that we were agreed on the combination of
patches. I won't try to defend your patch from yourself. I'll drop the
hash changes from v2 if it helps get things moving along.
Alex
Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote:
From: Simon Glass
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1)
directly in the code shared with the host build, so we can drop the
unnecessary indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
Signed-off-by: Alexandru Gagniuc
---
common/image-fit.c | 2 +-
include/image.h| 8
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe3..24e92a8e92 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
CHUNKSZ_CRC32);
*((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
*value_len = 4;
- } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
+ } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise
this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not
in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define
IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why
I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how
we can get hash algos in linker lists, like we're proposing to do for
crytpo algos. Or I could just drop this change in v2.
Alex
Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote:
From: Simon Glass
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1)
directly in the code shared with the host build, so we can drop the
unnecessary indirection.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
Signed-off-by: Alexandru Gagniuc
---
common/image-fit.c | 2 +-
include/image.h| 8
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe3..24e92a8e92 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const
char *algo,
CHUNKSZ_CRC32);
*((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
*value_len = 4;
- } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
+ } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise
this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not
in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define
IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why
I think this change is an improvement, and part of this series.
Alex
sha1_csum_wd((unsigned char *)data, data_len,
(unsigned char *)value, CHUNKSZ_SHA1);
*value_len = 20;
diff --git a/include/image.h b/include/image.h
index 887a3270bd..8c718adba0 100644
--- a/include/image.h
+++ b/include/image.h
@@ -68,13 +68,9 @@ struct fdt_region;
# ifdef CONFIG_SPL_MD5
# define IMAGE_ENABLE_MD51
# endif
-# ifdef CONFIG_SPL_FIT_SHA1
-# define IMAGE_ENABLE_SHA1 1
-# endif
# else
# define IMAGE_ENABLE_CRC32 1
# define IMAGE_ENABLE_MD5 1
-# define IMAGE_ENABLE_SHA11
# endif
#ifndef IMAGE_ENABLE_CRC32
@@ -85,10 +81,6 @@ struct fdt_region;
#define IMAGE_ENABLE_MD5 0
#endif
-#ifndef IMAGE_ENABLE_SHA1
-#define IMAGE_ENABLE_SHA1 0
-#endif
-
#if defined(CONFIG_FIT_SHA256) || \
defined(CONFIG_SPL_FIT_SHA256)
#define IMAGE_ENABLE_SHA2561
--
2.31.1
Regards,
Simon
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/17/21 8:23 PM, AKASHI Takahiro wrote: On Mon, May 17, 2021 at 05:29:44PM -0500, Alex G. wrote: On 5/12/21 12:14 PM, Tom Rini wrote: On Wed, May 12, 2021 at 11:19:52AM -0500, Alex G. wrote: On 5/12/21 10:52 AM, Simon Glass wrote: [snip] We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? It could be a config option instead of an environment variable. I think it can be independent of target options, since we don't sign images in the buildsystem anyway -- we can enable FIT verification, but mkimage without openssl. As people point out from time to time, "NO_SDL" is very non-obvious and doesn't fit with how the rest of U-Boot is configured. So I would rather not see NO_SSL added. FYI, I have a proof-of-concept for the NO_SSL idea using Kconfig [1] instead of environment variahles. It's not yet ready for publication. [1] https://github.com/mrnuke/u-boot/commit/c054c546a8de54e41d3802fe60ad9389095e673b FYI, I have posted a patch[1] for a similar *signing* tool using OpenSSL. Basically, I'd like to follow the way agreed here about how OpenSSL be handled in host tools. So please keep in mind that there can be another use case of this kind of host Kconfig option. [1] https://lists.denx.de/pipermail/u-boot/2021-May/449572.html I can't ask you to change your patch based on my ideas, as I my changes have not yet been submitted for review. However, should you want to anticipate, make sure that there's one and only one variable that determines if OpenSSL is linked. I also suspect Tom would be quite thrilled if your patch started using gnutls instead of openssl. I'm not sure how sane things would look having both gnutls and openssl dependencies; however, I suspect it might be acceptable as long as it's temporary. These decisions haven't been made yet. I don't want to send you on a wild goose refactoring chase, only to have the rug pulled from under you later. I think it's okay to continue with your patch as submitted. I'll update my patch accordingly when yours gets merged first -- looks easy enough. Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/12/21 12:14 PM, Tom Rini wrote: On Wed, May 12, 2021 at 11:19:52AM -0500, Alex G. wrote: On 5/12/21 10:52 AM, Simon Glass wrote: [snip] We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? It could be a config option instead of an environment variable. I think it can be independent of target options, since we don't sign images in the buildsystem anyway -- we can enable FIT verification, but mkimage without openssl. As people point out from time to time, "NO_SDL" is very non-obvious and doesn't fit with how the rest of U-Boot is configured. So I would rather not see NO_SSL added. FYI, I have a proof-of-concept for the NO_SSL idea using Kconfig [1] instead of environment variahles. It's not yet ready for publication. [1] https://github.com/mrnuke/u-boot/commit/c054c546a8de54e41d3802fe60ad9389095e673b Frankly, given everything else that's needed to build today, I don't think just enabling the support for verified boot in mkimage by default and making it a bit odd to turn off is a problem. But given: https://lists.denx.de/pipermail/u-boot/2017-December/313742.html I would really like to see the switch to gnutls or some other clearly compatibly licensed library first. Might be interesting to switch to gnutls, even if only because it doesn't burn your eyes looking at function names and variable types. I wouldn't mind looking into this, but I just don't have the bandwidth nowadays. Alex
Re: [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file
On 5/17/21 11:38 AM, Alexandru Gagniuc wrote: image-sig.c is used to map a hash or crypto algorithm name to a handler of that algorithm. There is some similarity between the host and target variants, with the differences worked out by #ifdefs. The purpose of this change is to remove those ifdefs. First, copy the file to a host-only version, and remove target specific code. Although it looks like we are duplicating code, subsequent patches will change the way target algorithms are searched. Besides we are only duplicating three string to struct mapping functions. This isn't something to fuss about. Signed-off-by: Alexandru Gagniuc --- tools/Makefile | 5 +- tools/image-sig-host.c | 133 + 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tools/image-sig-host.c diff --git a/tools/Makefile b/tools/Makefile index d020c55d66..e39006b6f6 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -57,8 +57,9 @@ hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include -FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_OBJS-$(CONFIG_FIT) := image-sig-host.o fit_common.o fit_image.o \ + image-host.o common/image-fit.o +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o This may cause a build failure with FIT_SIGNATURE disabled. I will have this fixed in v2. The correction is trivial. Correct diff below for reference: FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o common/image-fit-sig.o
Re: [PATCH RFC 01/10] common: Move host-only logic in image-sig.c to separate file
On 5/15/21 10:20 AM, Simon Glass wrote:
Hi Alexandru,
On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc wrote:
image-sig.c is used to map a hash or crypto algorithm name to a
handler of that algorithm. There is some similarity between the host
and target variants, with the differences worked out by #ifdefs. The
purpose of this change is to remove those ifdefs.
First, copy the file to a host-only version, and remove target
specific code. Although it looks like we are duplicating code,
subsequent patches will change the way target algorithms are searched.
Besides we are only duplicating three string to struct mapping
functions. This isn't something to fuss about.
---
common/image-sig-host.c | 134
tools/Makefile | 2 +-
2 files changed, 135 insertions(+), 1 deletion(-)
create mode 100644 common/image-sig-host.c
Will we never support signing in the board code? So far it is true,
but I wonder if it will remain so, as things get more and more
complicated. For example, we may want to sign the devicetree (somehow)
after fix-ups. The current code structure makes it possible to add
signing if needed. If we decided we wanted to sign on the board, how
would we refactor things with this approach?
We'd have the logistics of keeping private keys available to firmware
and only to firmware, but those are orthogonal to the problem. Assuming
we implemented a device-side *_sign(), then we would add it to the
linker list, via the proposed U_BOOT_CRYPTO_ALGO():
int rsa_device_side_sign(...)
{
if (!CONFIG_IS_ENABLED(RSA_SIGN_ON_DEVICE))
return -EIEIO;
return do_rsa_device_side_sign(...);
}
U_BOOT_CRYPTO_ALGO(rsa2048) = {
.name = "rsa2048",
.key_len = RSA2048_BYTES,
.verify = rsa_verify,
.sign = rsa_device_side_sign,
};
If this is host code, can we move it to tools/ ?
Definitely!
Re: [PATCH v2 47/50] image: Remove some #ifdefs from image-fit and image-fit-sig
On 5/6/21 9:24 AM, Simon Glass wrote:
Drop the #ifdefs which are easy to remove without refactoring.
Signed-off-by: Simon Glass
---
(no changes since v1)
common/Kconfig.boot| 10 ++
common/image-fit-sig.c | 8 ++--
common/image-fit.c | 7 ---
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 03a6e6f214f..a31d9847124 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -191,6 +191,16 @@ config SPL_FIT_SIGNATURE
select SPL_IMAGE_SIGN_INFO
select SPL_FIT_FULL_CHECK
+config SPL_FIT_SIGNATURE_MAX_SIZE
+ hex "Max size of signed FIT structures in SPL"
+ depends on SPL_FIT_SIGNATURE
+ default 0x1000
+ help
+ This option sets a max size in bytes for verified FIT uImages.
+ A sane value of 256MB protects corrupted DTB structures from
overlapping
+ device memory. Assure this size does not extend past expected storage
+ space.
+
I can't find an argument of why we'd want a separate
FIT_SIGNATURE_MAX_SIZE for SPL. This also seems unrelated to the commit
message of reducing ifdefs.
config SPL_LOAD_FIT
bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
select SPL_FIT
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 12a6745c642..22f89861048 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -49,10 +49,8 @@ struct image_region *fit_region_make_list(const void *fit,
* Use malloc() except in SPL (to save code size). In SPL the caller
* must allocate the array.
*/
-#ifndef CONFIG_SPL_BUILD
- if (!region)
+ if (!IS_ENABLED(CONFIG_SPL_BUILD) && !region)
region = calloc(sizeof(*region), count);
-#endif
if (!region)
return NULL;
for (i = 0; i < count; i++) {
@@ -72,12 +70,10 @@ static int fit_image_setup_verify(struct image_sign_info
*info,
char *algo_name;
const char *padding_name;
-#ifndef USE_HOSTCC
- if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) {
+ if (fdt_totalsize(fit) > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE)) {
*err_msgp = "Total size too large";
return 1;
}
-#endif
if (fit_image_hash_get_algo(fit, noffset, _name)) {
*err_msgp = "Can't get hash algo property";
return -1;
diff --git a/common/image-fit.c b/common/image-fit.c
index f8aa61fc99d..882e872144f 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1993,9 +1993,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
int type_ok, os_ok;
ulong load, load_end, data, len;
uint8_t os, comp;
-#ifndef USE_HOSTCC
- uint8_t os_arch;
-#endif
const char *prop_name;
int ret;
@@ -2087,8 +2084,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
}
#ifndef USE_HOSTCC
+ {
+ uint8_t os_arch;
+
fit_image_get_arch(fit, noffset, _arch);
images->os.arch = os_arch;
+ }
#endif
bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
Re: [PATCH v2 42/50] image: Drop #ifdefs for fit_print_contents()
On 5/6/21 9:24 AM, Simon Glass wrote:
Use a simple return to drop the unwanted code.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
(no changes since v1)
common/image-fit.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index 3ee306143b3..f8aa61fc99d 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -165,7 +165,6 @@ int fit_get_subimage_count(const void *fit, int
images_noffset)
return count;
}
-#if CONFIG_IS_ENABLED(FIT_PRINT)
/**
* fit_image_print_data() - prints out the hash node details
* @fit: pointer to the FIT format image header
@@ -375,6 +374,9 @@ void fit_print_contents(const void *fit)
const char *p;
time_t timestamp;
+ if (!CONFIG_IS_ENABLED(FIT_PRINT))
+ return;
+
/* Indent string is defined in header image.h */
p = IMAGE_INDENT_STRING;
@@ -477,6 +479,9 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
int ndepth;
int ret;
+ if (!CONFIG_IS_ENABLED(FIT_PRINT))
+ return;
+
/* Mandatory properties */
ret = fit_get_desc(fit, image_noffset, );
printf("%s Description: ", p);
@@ -570,10 +575,6 @@ void fit_image_print(const void *fit, int image_noffset,
const char *p)
}
}
}
-#else
-void fit_print_contents(const void *fit) { }
-void fit_image_print(const void *fit, int image_noffset, const char *p) { }
-#endif /* CONFIG_IS_ENABLED(FIT_PRINT) */
/**
* fit_get_desc - get node description property
Re: [PATCH v2 41/50] image: Drop unnecessary #ifdefs from image.h
On 5/6/21 9:24 AM, Simon Glass wrote:
This file has a lot of conditional code and much of it is unnecessary.
Clean this up to reduce the number of build combinations.
Signed-off-by: Simon Glass
---
[snip]
@@ -523,12 +520,9 @@ enum fit_load_op {
int boot_get_setup(bootm_headers_t *images, uint8_t arch, ulong *setup_start,
ulong *setup_len);
-#ifndef USE_HOSTCC
/* Image format types, returned by _get_format() routine */
#define IMAGE_FORMAT_INVALID 0x00
-#if defined(CONFIG_LEGACY_IMAGE_FORMAT)
#define IMAGE_FORMAT_LEGACY 0x01/* legacy image_header based format */
-#endif
#define IMAGE_FORMAT_FIT 0x02/* new, libfdt based format */
#define IMAGE_FORMAT_ANDROID 0x03/* Android boot image */
I've hit build errors in stm32 before because IMAGE_FORMAT_LEGACY wasn't
defined. I'm with you on this one.
[snip]
/**
* board_fit_config_name_match() - Check for a matching board name
diff --git a/include/u-boot/hash-checksum.h b/include/u-boot/hash-checksum.h
index 54e6a73744e..7f16b37a9ab 100644
--- a/include/u-boot/hash-checksum.h
+++ b/include/u-boot/hash-checksum.h
@@ -7,11 +7,12 @@
#define _RSA_CHECKSUM_H
#include
-#include
#include
#include
#include
+struct image_region;
+
/**
* hash_calculate() - Calculate hash over the data
*
@@ -23,7 +24,7 @@
* @return 0 if OK, < 0 if error
*/
int hash_calculate(const char *name,
- const struct image_region region[], int region_count,
+ const struct image_region *region, int region_count,
uint8_t *checksum);
This doesn't have to do anything with ifdefs. Should id be a separate
change?
#endif
diff --git a/lib/hash-checksum.c b/lib/hash-checksum.c
index d732ecc38fd..8f2a42f9a08 100644
--- a/lib/hash-checksum.c
+++ b/lib/hash-checksum.c
@@ -17,7 +17,7 @@
#include
int hash_calculate(const char *name,
- const struct image_region region[],
+ const struct image_region *region,
int region_count, uint8_t *checksum)
{
struct hash_algo *algo;
Ditto
Re: [PATCH v2 38/50] image: Drop IMAGE_ENABLE_BEST_MATCH
On 5/6/21 9:24 AM, Simon Glass wrote:
This is not needed with Kconfig, since we can use IS_ENABLED() easily
enough. Drop it.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
(no changes since v1)
common/image-fit.c | 2 +-
include/image.h| 5 -
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index e81a0858dc1..a0987fd52c8 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -2026,7 +2026,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
* fit_conf_get_node() will try to find default config node
*/
bootstage_mark(bootstage_id + BOOTSTAGE_SUB_NO_UNIT_NAME);
- if (IMAGE_ENABLE_BEST_MATCH && !fit_uname_config) {
+ if (IS_ENABLED(CONFIG_FIT_BEST_MATCH) && !fit_uname_config) {
cfg_noffset = fit_conf_find_compat(fit, gd_fdt_blob());
} else {
cfg_noffset = fit_conf_get_node(fit,
diff --git a/include/image.h b/include/image.h
index 12043abd049..b388684cbdc 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1157,11 +1157,6 @@ void image_set_host_blob(void *host_blob);
# define gd_fdt_blob()(gd->fdt_blob)
#endif
-#ifdef CONFIG_FIT_BEST_MATCH
-#define IMAGE_ENABLE_BEST_MATCH1
-#else
-#define IMAGE_ENABLE_BEST_MATCH0
-#endif
#endif /* FIT */
/*
Re: [PATCH v2 29/50] Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5
On 5/6/21 9:24 AM, Simon Glass wrote: Drop the _SUPPORT suffix so we can use CONFIG_IS_ENABLED() with this option. Signed-off-by: Simon Glass Reviewed-by: Alexandru Gagniuc --- (no changes since v1) common/spl/Kconfig | 2 +- include/image.h| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 8cd41eb1b29..e6d00caaa85 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -418,7 +418,7 @@ config SPL_CRC32 for detected accidental image corruption. For secure applications you should consider SHA1 or SHA256. -config SPL_MD5_SUPPORT +config SPL_MD5 bool "Support MD5" depends on SPL_FIT help diff --git a/include/image.h b/include/image.h index e68c2cbf621..e1e4bf6806f 100644 --- a/include/image.h +++ b/include/image.h @@ -47,7 +47,7 @@ struct fdt_region; #include #include # ifdef CONFIG_SPL_BUILD -# ifdef CONFIG_SPL_MD5_SUPPORT +# ifdef CONFIG_SPL_MD5 # define IMAGE_ENABLE_MD5 1 # endif # ifdef CONFIG_SPL_FIT_SHA1
Re: [PATCH v2 27/50] Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32
On 5/6/21 9:24 AM, Simon Glass wrote: Drop the _SUPPORT suffix so we can use CONFIG_IS_ENABLED() with this option. Signed-off-by: Simon Glass Reviewed-by: Alexandru Gagniuc --- (no changes since v1) common/spl/Kconfig| 4 ++-- configs/axm_defconfig | 2 +- configs/chromebit_mickey_defconfig| 2 +- configs/chromebook_jerry_defconfig| 2 +- configs/chromebook_minnie_defconfig | 2 +- configs/chromebook_speedy_defconfig | 2 +- configs/evb-px30_defconfig| 2 +- configs/firefly-px30_defconfig| 2 +- configs/imxrt1020-evk_defconfig | 2 +- configs/imxrt1050-evk_defconfig | 2 +- configs/odroid-go2_defconfig | 2 +- configs/px30-core-ctouch2-px30_defconfig | 2 +- configs/px30-core-edimm2.2-px30_defconfig | 2 +- configs/socfpga_agilex_atf_defconfig | 2 +- configs/socfpga_agilex_vab_defconfig | 2 +- configs/socfpga_stratix10_atf_defconfig | 2 +- configs/taurus_defconfig | 2 +- include/image.h | 2 +- 18 files changed, 19 insertions(+), 19 deletions(-) diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 2570b220891..39fc260566b 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -204,7 +204,7 @@ config SPL_LEGACY_IMAGE_SUPPORT config SPL_LEGACY_IMAGE_CRC_CHECK bool "Check CRC of Legacy images" depends on SPL_LEGACY_IMAGE_SUPPORT - select SPL_CRC32_SUPPORT + select SPL_CRC32 help Enable this to check the CRC of Legacy images. While this increases reliability, it affects both code size and boot duration. @@ -407,7 +407,7 @@ config SYS_MMCSD_RAW_MODE_EMMC_BOOT_PARTITION the eMMC EXT_CSC_PART_CONFIG selection should be overridden in SPL by user defined partition number. -config SPL_CRC32_SUPPORT +config SPL_CRC32 bool "Support CRC32" default y if SPL_LEGACY_IMAGE_SUPPORT help diff --git a/configs/axm_defconfig b/configs/axm_defconfig index 0bfd7548b09..4e776fd6953 100644 --- a/configs/axm_defconfig +++ b/configs/axm_defconfig @@ -32,7 +32,7 @@ CONFIG_BOOTCOMMAND="run flash_self" CONFIG_BOARD_EARLY_INIT_F=y # CONFIG_SPL_LEGACY_IMAGE_SUPPORT is not set CONFIG_SPL_SYS_MALLOC_SIMPLE=y -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_NAND_SUPPORT=y CONFIG_SPL_NAND_DRIVERS=y CONFIG_SPL_NAND_ECC=y diff --git a/configs/chromebit_mickey_defconfig b/configs/chromebit_mickey_defconfig index c09b63b9462..2b664e118cf 100644 --- a/configs/chromebit_mickey_defconfig +++ b/configs/chromebit_mickey_defconfig @@ -25,7 +25,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/chromebook_jerry_defconfig b/configs/chromebook_jerry_defconfig index 692b630174d..a757d259f58 100644 --- a/configs/chromebook_jerry_defconfig +++ b/configs/chromebook_jerry_defconfig @@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/chromebook_minnie_defconfig b/configs/chromebook_minnie_defconfig index ae55842e3bf..353aa01ea9e 100644 --- a/configs/chromebook_minnie_defconfig +++ b/configs/chromebook_minnie_defconfig @@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/chromebook_speedy_defconfig b/configs/chromebook_speedy_defconfig index 4b460ee6a9e..c5be5597b10 100644 --- a/configs/chromebook_speedy_defconfig +++ b/configs/chromebook_speedy_defconfig @@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/evb-px30_defconfig b/configs/evb-px30_defconfig index d2fdfef2938..55e2702a172 100644 --- a/configs/evb-px30_defconfig +++ b/configs/evb-px30_defconfig @@ -29,7 +29,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_ATF=y # CONFIG_TPL_FRAMEWORK is not set # CONFIG_CMD_BOOTD
Re: [PATCH v2 37/50] image: Drop IMAGE_ENABLE_SIGN/VERIFY defines
On 5/14/21 3:44 PM, Simon Glass wrote: Hi Alex, On Fri, 14 May 2021 at 14:38, Alex G. wrote: On 5/6/21 9:24 AM, Simon Glass wrote: Add host Kconfigs for FIT_SIGN and RSA_VERIFY. With these we can use CONFIG_IS_ENABLED() directly in the host build, so drop the unnecessary indirections IMAGE_ENABLE_SIGN and HOST_RSA_VERIFY. Also drop FIT_IMAGE_ENABLE_VERIFY which is not actually used. Leave IMAGE_ENABLE_VERIFY_ECDSA along since this feature is incomplete and needs to be integrated with RSA. Signed-off-by: Simon Glass --- (no changes since v1) common/image-fit.c | 6 +++--- common/image-sig.c | 10 +- include/image.h| 13 ++--- include/u-boot/ecdsa.h | 2 +- include/u-boot/rsa.h | 4 ++-- tools/Kconfig | 10 ++ tools/image-host.c | 4 ++-- 7 files changed, 25 insertions(+), 24 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index c13ff6bba24..e81a0858dc1 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1301,7 +1301,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, int ret; /* Verify all required signatures */ - if (FIT_IMAGE_ENABLE_VERIFY && + if (CONFIG_IS_ENABLED(RSA_VERIFY) && NAK. Having verification depend directly on CONFIG_RSA_VERIFY will make adding ECDSA support that much more convoluted. Let me counter-NAK. The ECDSA needs to be integrated into the RSA stuff, as we have done with hashing. E.g. CONFIG_VERIFY that enables the feature, with a driver to select which methods are supported. Then why not add a CONFIG_(SPL_)VERIFY to this patch instead of replacing a common define with an algo-secific CONFIG? I think I mentioned that in the original review. You did. Integrating ECDSA with RSA is orthogonal to ECDSA verification. I like the motivation behind this cosmetic series, but it is creating unnecessary complications to adding the ECDSA features. "It is relatively straightforward to add new algorithms if required. [...] If another algorithm is needed (such as DSA) then it can be placed alongside rsa.c, and its functions added to the table in image-sig.c also." That's from doc/uImage.FIT/signature.txt. Seems like we're changing goal posts as the balls are already in the air. I want to tone down this series, pick a few patches that I really like, combine them with some of my changes and submit a co-authored series with the uncontroversial changes. I posted a parallel series which eliminates IMAGE_ENABLE_VERIFY_ECDSA, and is far less intrusive. I was already trying to combine it with some patches in this series. Let's see how that goes Alex
Re: [PATCH v2 37/50] image: Drop IMAGE_ENABLE_SIGN/VERIFY defines
On 5/6/21 9:24 AM, Simon Glass wrote:
Add host Kconfigs for FIT_SIGN and RSA_VERIFY. With these we can use
CONFIG_IS_ENABLED() directly in the host build, so drop the unnecessary
indirections IMAGE_ENABLE_SIGN and HOST_RSA_VERIFY. Also drop
FIT_IMAGE_ENABLE_VERIFY which is not actually used.
Leave IMAGE_ENABLE_VERIFY_ECDSA along since this feature is incomplete
and needs to be integrated with RSA.
Signed-off-by: Simon Glass
---
(no changes since v1)
common/image-fit.c | 6 +++---
common/image-sig.c | 10 +-
include/image.h| 13 ++---
include/u-boot/ecdsa.h | 2 +-
include/u-boot/rsa.h | 4 ++--
tools/Kconfig | 10 ++
tools/image-host.c | 4 ++--
7 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index c13ff6bba24..e81a0858dc1 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1301,7 +1301,7 @@ int fit_image_verify_with_data(const void *fit, int
image_noffset,
int ret;
/* Verify all required signatures */
- if (FIT_IMAGE_ENABLE_VERIFY &&
+ if (CONFIG_IS_ENABLED(RSA_VERIFY) &&
NAK. Having verification depend directly on CONFIG_RSA_VERIFY will make
adding ECDSA support that much more convoluted.
fit_image_verify_required_sigs(fit, image_noffset, data, size,
gd_fdt_blob(), _all)) {
err_msg = "Unable to verify required signature";
@@ -1323,7 +1323,7 @@ int fit_image_verify_with_data(const void *fit, int
image_noffset,
_msg))
goto error;
puts("+ ");
- } else if (FIT_IMAGE_ENABLE_VERIFY && verify_all &&
+ } else if (CONFIG_IS_ENABLED(RSA_VERIFY) && verify_all &&
!strncmp(name, FIT_SIG_NODENAME,
strlen(FIT_SIG_NODENAME))) {
ret = fit_image_check_sig(fit, noffset, data,
@@ -2045,7 +2045,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
if (image_type == IH_TYPE_KERNEL)
images->fit_uname_cfg = fit_base_uname_config;
- if (FIT_IMAGE_ENABLE_VERIFY && images->verify) {
+ if (CONFIG_IS_ENABLED(RSA_VERIFY) && images->verify) {
puts(" Verifying Hash Integrity ... ");
if (fit_config_verify(fit, cfg_noffset)) {
puts("Bad Data Hash\n");
diff --git a/common/image-sig.c b/common/image-sig.c
index bbc6bb3b1e3..74ca96a39e9 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -29,7 +29,7 @@ struct checksum_algo checksum_algos[] = {
.checksum_len = SHA1_SUM_LEN,
.der_len = SHA1_DER_LEN,
.der_prefix = sha1_der_prefix,
-#if IMAGE_ENABLE_SIGN
+#if CONFIG_IS_ENABLED(FIT_SIGN)
.calculate_sign = EVP_sha1,
#endif
.calculate = hash_calculate,
@@ -39,7 +39,7 @@ struct checksum_algo checksum_algos[] = {
.checksum_len = SHA256_SUM_LEN,
.der_len = SHA256_DER_LEN,
.der_prefix = sha256_der_prefix,
-#if IMAGE_ENABLE_SIGN
+#if CONFIG_IS_ENABLED(FIT_SIGN)
.calculate_sign = EVP_sha256,
#endif
.calculate = hash_calculate,
@@ -50,7 +50,7 @@ struct checksum_algo checksum_algos[] = {
.checksum_len = SHA384_SUM_LEN,
.der_len = SHA384_DER_LEN,
.der_prefix = sha384_der_prefix,
-#if IMAGE_ENABLE_SIGN
+#if CONFIG_IS_ENABLED(FIT_SIGN)
.calculate_sign = EVP_sha384,
#endif
.calculate = hash_calculate,
@@ -62,7 +62,7 @@ struct checksum_algo checksum_algos[] = {
.checksum_len = SHA512_SUM_LEN,
.der_len = SHA512_DER_LEN,
.der_prefix = sha512_der_prefix,
-#if IMAGE_ENABLE_SIGN
+#if CONFIG_IS_ENABLED(FIT_SIGN)
.calculate_sign = EVP_sha512,
#endif
.calculate = hash_calculate,
@@ -122,7 +122,7 @@ struct checksum_algo *image_get_checksum_algo(const char
*full_name)
struct checksum_algo *algo = _algos[i];
MANUAL_RELOC(algo->name);
-#if IMAGE_ENABLE_SIGN
+#if CONFIG_IS_ENABLED(FIT_SIGN)
MANUAL_RELOC(algo->calculate_sign);
#endif
MANUAL_RELOC(algo->calculate);
diff --git a/include/image.h b/include/image.h
index 64866c609f4..12043abd049 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1139,22 +1139,13 @@ int calculate_hash(const void *data, int data_len,
const char *algo,
*/
#if defined(USE_HOSTCC)
# if defined(CONFIG_FIT_SIGNATURE)
-# define IMAGE_ENABLE_SIGN1
-# define IMAGE_ENABLE_VERIFY 1
# define IMAGE_ENABLE_VERIFY_ECDSA 1
-# define FIT_IMAGE_ENABLE_VERIFY 1
# include
#
Re: [PATCH v2 21/50] hash: Drop some #ifdefs in hash.c
On 5/6/21 9:24 AM, Simon Glass wrote:
We can use the __maybe_unused attribute to avoid some of the #ifdefs in
this file. Update the functions accordingly.
What is __maybe_unused? Does u-boot support booting quantum computers?
Note: The actual hashing interface is still a mess, with four separate
combinations and lots of #ifdefs. This should really use a driver
approach, e.g. as is done with partition drivers.
Signed-off-by: Simon Glass
---
(no changes since v1)
common/hash.c | 54 ---
1 file changed, 25 insertions(+), 29 deletions(-)
diff --git a/common/hash.c b/common/hash.c
index 987d238c66c..1ccc62f162d 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -24,6 +24,7 @@
#include
#else
#include "mkimage.h"
+#include
#include
#include
#endif /* !USE_HOSTCC*/
@@ -42,8 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
static void reloc_update(void);
-#if CONFIG_IS_ENABLED(SHA1) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
-static int hash_init_sha1(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp)
{
sha1_context *ctx = malloc(sizeof(sha1_context));
sha1_starts(ctx);
@@ -51,15 +51,16 @@ static int hash_init_sha1(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_sha1(struct hash_algo *algo, void *ctx, const void *buf,
- unsigned int size, int is_last)
+static int __maybe_unused hash_update_sha1(struct hash_algo *algo, void *ctx,
+ const void *buf, unsigned int size,
+ int is_last)
{
sha1_update((sha1_context *)ctx, buf, size);
return 0;
}
-static int hash_finish_sha1(struct hash_algo *algo, void *ctx, void *dest_buf,
- int size)
+static int __maybe_unused hash_finish_sha1(struct hash_algo *algo, void *ctx,
+ void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -68,10 +69,8 @@ static int hash_finish_sha1(struct hash_algo *algo, void
*ctx, void *dest_buf,
free(ctx);
return 0;
}
-#endif
-#if CONFIG_IS_ENABLED(SHA256) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
-static int hash_init_sha256(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_sha256(struct hash_algo *algo, void **ctxp)
{
sha256_context *ctx = malloc(sizeof(sha256_context));
sha256_starts(ctx);
@@ -79,15 +78,16 @@ static int hash_init_sha256(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_sha256(struct hash_algo *algo, void *ctx,
- const void *buf, unsigned int size, int is_last)
+static int __maybe_unused hash_update_sha256(struct hash_algo *algo, void *ctx,
+const void *buf, uint size,
+int is_last)
{
sha256_update((sha256_context *)ctx, buf, size);
return 0;
}
-static int hash_finish_sha256(struct hash_algo *algo, void *ctx, void
- *dest_buf, int size)
+static int __maybe_unused hash_finish_sha256(struct hash_algo *algo, void *ctx,
+void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -96,10 +96,8 @@ static int hash_finish_sha256(struct hash_algo *algo, void
*ctx, void
free(ctx);
return 0;
}
-#endif
-#if CONFIG_IS_ENABLED(SHA384) && !CONFIG_IS_ENABLED(SHA_PROG_HW_ACCEL)
-static int hash_init_sha384(struct hash_algo *algo, void **ctxp)
+static int __maybe_unused hash_init_sha384(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
sha384_starts(ctx);
@@ -107,15 +105,16 @@ static int hash_init_sha384(struct hash_algo *algo, void
**ctxp)
return 0;
}
-static int hash_update_sha384(struct hash_algo *algo, void *ctx,
- const void *buf, unsigned int size, int is_last)
+static int __maybe_unused hash_update_sha384(struct hash_algo *algo, void *ctx,
+const void *buf, uint size,
+int is_last)
{
sha384_update((sha512_context *)ctx, buf, size);
return 0;
}
-static int hash_finish_sha384(struct hash_algo *algo, void *ctx, void
- *dest_buf, int size)
+static int __maybe_unused hash_finish_sha384(struct hash_algo *algo, void *ctx,
+void *dest_buf, int size)
{
if (size < algo->digest_size)
return -1;
@@ -124,10 +123,8 @@ static int hash_finish_sha384(struct hash_algo *algo, void
*ctx, void
free(ctx);
return 0;
}
-#endif
-#if
