Re: [PATCH] Revert "spl: Drop bd_info in the data section"
Hi Simon On 4/8/21 6:55 PM, Simon Glass wrote: Hi Alexandru, On Fri, 9 Apr 2021 at 04:56, Alexandru Gagniuc wrote: This reverts commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75. struct global_data contains a pointer to the bd_info structure. This pointer was populated spl_set_bd() to a pre-allocated bd_info in the ".data" section. The referenced commit replaced this mechanism to one that uses malloc(). That new mechanism is only used if SPL_ALLOC_BD=y. which very few boards do. The result is that (struct global_data)->bd is NULL in SPL on most platforms. This breaks falcon mode, since arch_fixup_fdt() tries to access (struct global_data)->bd and set the "/memory" node in the devicetree. The result is that the "/memory" node contains garbage values, causing linux to panic() as it sets up the page table. Instead of trying to fix the mess, potentially causing other issues, revert to the code that worked, while this change is reworked. The goal here is to drop a feature that few boards use and reduce memory usage in SPL. It has been in place for two releases now. If Falcon mode needs it, perhaps you could add an 'imply' in the Kconfig for that feature? Is there one? Or perhaps CONFIG_ARCH_FIXUP_FDT_MEMORY ? One option would be to return an error in arch_fixup_fdt(). In general, fixups make things tricky because there is no way to determine when they are used but at least this one has a CONFIG. The argument that this has been in place for two releases is incorrect. Commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75 was only introduced with the v2021.04 release. It definitely was not in 2021.01. It's only in the last release, which is four days old t the time of this writing. Although I was able to find one example, the reality is that we don't know the full extent of the breakage. The prudent thing at this point is to revert. The knowledge of how to init the platform is in the devicetree and code. Why should kconfig also be involved in storing this knowledge? By this model, as the number of boards increases without bounds, every "if" predicate tends to be Kconfig driven. That is not maintainable, and why I think the original change --and the proposed fixes-- are broken by design. Furthermore, I'm happy to discuss what to do about Falcon mode, and if we should kill it entirely (I have an alternative proposal). But we shouldn't have that discussion in a manner holding my platform hostage. To be fair to you, I don't think reverting a 64-byte savings in .data will hold your platform hostage either. Alex
Re: [PATCH] Revert "spl: Drop bd_info in the data section"
On 4/12/21 8:25 AM, Tom Rini wrote:
On Fri, Apr 09, 2021 at 05:29:36PM -0700, Tim Harvey wrote:
On Fri, Apr 9, 2021 at 1:53 PM Tom Rini wrote:
On Fri, Apr 09, 2021 at 03:24:41PM -0500, Adam Ford wrote:
On Fri, Apr 9, 2021 at 2:20 PM Alex G. wrote:
Hi Simon
On 4/8/21 6:55 PM, Simon Glass wrote:
Hi Alexandru,
On Fri, 9 Apr 2021 at 04:56, Alexandru Gagniuc wrote:
This reverts commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75.
struct global_data contains a pointer to the bd_info structure. This
pointer was populated spl_set_bd() to a pre-allocated bd_info in the
".data" section. The referenced commit replaced this mechanism to one
that uses malloc(). That new mechanism is only used if SPL_ALLOC_BD=y.
which very few boards do.
The result is that (struct global_data)->bd is NULL in SPL on most
platforms. This breaks falcon mode, since arch_fixup_fdt() tries to
access (struct global_data)->bd and set the "/memory" node in the
devicetree. The result is that the "/memory" node contains garbage
values, causing linux to panic() as it sets up the page table.
Instead of trying to fix the mess, potentially causing other issues,
revert to the code that worked, while this change is reworked.
The goal here is to drop a feature that few boards use and reduce
memory usage in SPL. It has been in place for two releases now.
If Falcon mode needs it, perhaps you could add an 'imply' in the
Kconfig for that feature? Is there one? Or perhaps
CONFIG_ARCH_FIXUP_FDT_MEMORY ?
One option would be to return an error in arch_fixup_fdt(). In
general, fixups make things tricky because there is no way to
determine when they are used but at least this one has a CONFIG.
The argument that this has been in place for two releases is incorrect.
Commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75 was only introduced with
the v2021.04 release. It definitely was not in 2021.01. It's only in the
last release, which is four days old t the time of this writing.
Yes. But another way of saying that is that we're 4 days in to the
merge window. That's a bit early to say we must revert the change. If
this was just before the release, yes, revert would be the right answer.
It's also the case the original commit fixes some cases while also
saving size, if I read it right. So a strict revert isn't right, we'd
need to also probably also default y SPL_ALLOC_BD in some cases.
Although I was able to find one example, the reality is that we don't
know the full extent of the breakage. The prudent thing at this point is
to revert.
The knowledge of how to init the platform is in the devicetree and code.
Why should kconfig also be involved in storing this knowledge? By this
model, as the number of boards increases without bounds, every "if"
predicate tends to be Kconfig driven. That is not maintainable, and why
I think the original change --and the proposed fixes-- are broken by design.
Furthermore, I'm happy to discuss what to do about Falcon mode, and if
we should kill it entirely (I have an alternative proposal). But we
shouldn't have that discussion in a manner holding my platform hostage.
To be fair to you, I don't think reverting a 64-byte savings in .data
will hold your platform hostage either.
That original patch broke a bunch of OMAP boards, but enabling
SPL_ALLOC_BD was the solution to fix the issue.
Can you try enabling SPL_ALLOC_BD and see if that fixes it? Maybe we
can make falcon mode imply it.
It would be "select" since it needs it rather than imply.
I just ran into this as well finding that commit 38d6b7ebdaee ("spl:
Drop bd_info in the data section") breaks Gateworks Ventana and
defining SPL_ALLOC_BD does resolve it. In this case, Falcon mode is
not being used and the breakage is because arch/arm/mach-imx/spl.c
dram_init_banksize() accesses gd->bd which is NULL.
So I would guess that this probably broke a whole lot of IMX based
boards that use SPL.
I don't quite know what the best solution is... we now have a v2021.04
that is broken for several or many boards and I dont' know if its
clear what cases break.
Looking forward, I think we need to rework this. Simon, I gather you
have some platforms where we need to set gd->bd to something that we
malloc() ? So perhaps spl_set_bd() should have been __weak so that
architectures / platforms could override as needed, but also move it
just past mem_malloc_init().
Let's try and avoid making new weak functions. Why not introduce a
spl_platform_alloc_bd(), that each plat- *must* implement? No diversion
to Kconfig and no __weak__ required.
Alex
Re: [PATCH] Revert "spl: Drop bd_info in the data section"
On 4/12/21 9:40 AM, Tom Rini wrote:
On Mon, Apr 12, 2021 at 08:51:11AM -0500, Alex G. wrote:
On 4/12/21 8:25 AM, Tom Rini wrote:
On Fri, Apr 09, 2021 at 05:29:36PM -0700, Tim Harvey wrote:
On Fri, Apr 9, 2021 at 1:53 PM Tom Rini wrote:
On Fri, Apr 09, 2021 at 03:24:41PM -0500, Adam Ford wrote:
On Fri, Apr 9, 2021 at 2:20 PM Alex G. wrote:
Hi Simon
On 4/8/21 6:55 PM, Simon Glass wrote:
Hi Alexandru,
On Fri, 9 Apr 2021 at 04:56, Alexandru Gagniuc wrote:
This reverts commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75.
struct global_data contains a pointer to the bd_info structure. This
pointer was populated spl_set_bd() to a pre-allocated bd_info in the
".data" section. The referenced commit replaced this mechanism to one
that uses malloc(). That new mechanism is only used if SPL_ALLOC_BD=y.
which very few boards do.
The result is that (struct global_data)->bd is NULL in SPL on most
platforms. This breaks falcon mode, since arch_fixup_fdt() tries to
access (struct global_data)->bd and set the "/memory" node in the
devicetree. The result is that the "/memory" node contains garbage
values, causing linux to panic() as it sets up the page table.
Instead of trying to fix the mess, potentially causing other issues,
revert to the code that worked, while this change is reworked.
The goal here is to drop a feature that few boards use and reduce
memory usage in SPL. It has been in place for two releases now.
If Falcon mode needs it, perhaps you could add an 'imply' in the
Kconfig for that feature? Is there one? Or perhaps
CONFIG_ARCH_FIXUP_FDT_MEMORY ?
One option would be to return an error in arch_fixup_fdt(). In
general, fixups make things tricky because there is no way to
determine when they are used but at least this one has a CONFIG.
The argument that this has been in place for two releases is incorrect.
Commit 38d6b7ebdaee3e0e8426ef1b9df88bdce8ae2e75 was only introduced with
the v2021.04 release. It definitely was not in 2021.01. It's only in the
last release, which is four days old t the time of this writing.
Yes. But another way of saying that is that we're 4 days in to the
merge window. That's a bit early to say we must revert the change. If
this was just before the release, yes, revert would be the right answer.
It's also the case the original commit fixes some cases while also
saving size, if I read it right. So a strict revert isn't right, we'd
need to also probably also default y SPL_ALLOC_BD in some cases.
Although I was able to find one example, the reality is that we don't
know the full extent of the breakage. The prudent thing at this point is
to revert.
The knowledge of how to init the platform is in the devicetree and code.
Why should kconfig also be involved in storing this knowledge? By this
model, as the number of boards increases without bounds, every "if"
predicate tends to be Kconfig driven. That is not maintainable, and why
I think the original change --and the proposed fixes-- are broken by design.
Furthermore, I'm happy to discuss what to do about Falcon mode, and if
we should kill it entirely (I have an alternative proposal). But we
shouldn't have that discussion in a manner holding my platform hostage.
To be fair to you, I don't think reverting a 64-byte savings in .data
will hold your platform hostage either.
That original patch broke a bunch of OMAP boards, but enabling
SPL_ALLOC_BD was the solution to fix the issue.
Can you try enabling SPL_ALLOC_BD and see if that fixes it? Maybe we
can make falcon mode imply it.
It would be "select" since it needs it rather than imply.
I just ran into this as well finding that commit 38d6b7ebdaee ("spl:
Drop bd_info in the data section") breaks Gateworks Ventana and
defining SPL_ALLOC_BD does resolve it. In this case, Falcon mode is
not being used and the breakage is because arch/arm/mach-imx/spl.c
dram_init_banksize() accesses gd->bd which is NULL.
So I would guess that this probably broke a whole lot of IMX based
boards that use SPL.
I don't quite know what the best solution is... we now have a v2021.04
that is broken for several or many boards and I dont' know if its
clear what cases break.
Looking forward, I think we need to rework this. Simon, I gather you
have some platforms where we need to set gd->bd to something that we
malloc() ? So perhaps spl_set_bd() should have been __weak so that
architectures / platforms could override as needed, but also move it
just past mem_malloc_init().
Let's try and avoid making new weak functions. Why not introduce a
spl_platform_alloc_bd(), that each plat- *must* implement? No diversion to
Kconfig and no __weak__ required.
Well, who needs something different than what we had before exactly?
I'm not sure. From reading the commit message of the broken change, it
seems the main driver was to sa
Getting rid of falcon mode
## Introduction Today we use "falcon mode" to mean "boot linux straight from SPL". This designation makes sense, since falcons "fly at high speed and change direction rapidly" according to Wikipedia. The way we implement falcon mode is to reserve two areas of storage: * kernel area/partition * dtb area/partition By using some "special cases", and "spl export", SPL can more or less figure out how to skip u-boot. ## The plot twist People familiar with FIT, will have recognized that the above is achievable with a very basic FIT image. With some advantages: - No "special cases" in SPL code - Signed kernel images - Signed kernel devicetree - Devicetree overlays - Automatic selection of correct devicetree ## The problems The advantages of FIT are not obvious by looking at SPL code. A noticeable amount of SPL code is hidden under #ifdef CONFIG_SPL_OS_BOOT, leading one to believe that SPL_OS_BOOT is very important. It must be since it takes up so much code. Enabling falcon mode is not well documented, and requires a lot of trial and error. I've had to define 7 macros, and one new function to get it working on my board -- and vividly remember the grief. This is an antiquated way of doing things, and completely ignores the u-boot devicetree -- we could just as well have defined those seven values in the dtb. SPL assumes that it must load u-boot, unless in instances under CONFIG_SPL_OS_BOOT. This has cause me a huge amount of grief and confusion over the past several months. I have no less than three patch series trying to address shortfalls there. It's awful. ## The proposal I propose we drop falcon mode support for legacy images. - Drop CONFIG_SPL_OS_BOOT. Support for this is implied by SPL_FIT - Drop the "dtb area/partition". The dtb is derived from the FIT - Drop "spl export" How do we deal with devicetrees in the FIT then? The options are to use a modified devicetree which has the desired "/chosen" node, or use DTB overlays. What are the reasons why we shouldn't go this route? Alex
Re: Getting rid of falcon mode
Hi Maxime, On 4/13/21 3:56 AM, Maxime Ripard wrote: Hi, On Mon, Apr 12, 2021 at 04:32:49PM -0500, Alex G. wrote: ## Introduction Today we use "falcon mode" to mean "boot linux straight from SPL". This designation makes sense, since falcons "fly at high speed and change direction rapidly" according to Wikipedia. The way we implement falcon mode is to reserve two areas of storage: * kernel area/partition * dtb area/partition By using some "special cases", and "spl export", SPL can more or less figure out how to skip u-boot. ## The plot twist People familiar with FIT, will have recognized that the above is achievable with a very basic FIT image. With some advantages: - No "special cases" in SPL code - Signed kernel images - Signed kernel devicetree - Devicetree overlays - Automatic selection of correct devicetree ## The problems The advantages of FIT are not obvious by looking at SPL code. A noticeable amount of SPL code is hidden under #ifdef CONFIG_SPL_OS_BOOT, leading one to believe that SPL_OS_BOOT is very important. It must be since it takes up so much code. Enabling falcon mode is not well documented, and requires a lot of trial and error. I've had to define 7 macros, and one new function to get it working on my board -- and vividly remember the grief. This is an antiquated way of doing things, and completely ignores the u-boot devicetree -- we could just as well have defined those seven values in the dtb. SPL assumes that it must load u-boot, unless in instances under CONFIG_SPL_OS_BOOT. This has cause me a huge amount of grief and confusion over the past several months. I have no less than three patch series trying to address shortfalls there. It's awful. ## The proposal I propose we drop falcon mode support for legacy images. - Drop CONFIG_SPL_OS_BOOT. Support for this is implied by SPL_FIT - Drop the "dtb area/partition". The dtb is derived from the FIT - Drop "spl export" How do we deal with devicetrees in the FIT then? The options are to use a modified devicetree which has the desired "/chosen" node, or use DTB overlays. What are the reasons why we shouldn't go this route? I can see at least two, that are mainly due to a FIT image being essentially a compiled device tree: - Not all platforms have enough head-space in their SPL to have libfdt in addition to what is already there. Do we know which platforms fall in this category? We can investigate if it might be possible to disable just enough of the legacy support to make room for libfdt. - Parsing a DT is fairly slow too. Last time I checked booting a FIT image took around 150ms more than a legacy image on a Cortex-A7. If one is using the Falcon mode in the first place chances are it's to improve the boot-time, so this seems like a step backward. I'm skeptical of the 150ms delta, as I also did heavy measurements on this a few months back. But maybe there's something that causes certain platforms to bog down, (caching issues ?). I've used grabserial (e.g. "grabserial -d /dev/ttyACM0 -b 200 -m"U-Boot SPL" -t") to time the boot. If you have similar logs, maybe there's something there that tells us why parsing the FIT bogs down. Alex
Re: [PATCH] Revert "spl: Drop bd_info in the data section"
On 4/16/21 6:16 PM, Adam Ford wrote: On Fri, Apr 16, 2021 at 3:41 PM Tim Harvey wrote: On Mon, Apr 12, 2021 at 11:44 AM Simon Glass wrote: Hi Tom, On Tue, 13 Apr 2021 at 06:38, Tom Rini wrote: On Tue, Apr 13, 2021 at 06:26:08AM +1200, Simon Glass wrote: Hi Tom, On Tue, 13 Apr 2021 at 06:18, Tom Rini wrote: On Tue, Apr 13, 2021 at 05:49:19AM +1200, Simon Glass wrote: [snip] As to a weak function, what would the default behaviour be? If we can define that, then it would be better IMO. When we try to refactor things, weak functions and undefined (or arch-specific behaviour) can really make things tough. Well, what was the problem you were trying to solve here? I assumed there was a case where things actively broke, with the previous design, and it's not just the 64byte savings in some cases. But is it? Yes the region of memory is not writable, so must be allocated at runtime. Where, on x86? Some ARM cases? That's one of the other things to sort out here. Yes, on coral and likely newer things to come...For ARM I don't know of any such problems. I'm not sure I understand if there is agreement on a solution to this patch breaking several or many boards? I count 58 IMX6 boards using SPL and none of them currently define CONFIG_SPL_ALLOC_BD=y. It sounds like Adam said OMAP boards were broken as well and I'm not clear if those boards are fixed yet either. I have already submitted a patch for the OMAP boards that I maintain to address the issue. I wonder if it would make sense to make these architectures select CONFIG_SPL_ALLOC_BD automatically instead of having a bunch of individual boards enable it. I have not checked any of the IMX8M or IMX6 boards that I maintain, but I am watching this thread. I can test the CONFIG_SPL_ALLOC_BD on my boards if people think there is value. I've been thinking about what Simon said -- that the memory region is not writable on x86. But that memory is in .data, which should be writable. It doesn't make sense. Alex
Re: [PATCH v4 2/6] lib: ecdsa: Add skeleton to implement ecdsa verification in u-boot
On 4/21/21 2:15 AM, Simon Glass wrote: Hi Alexandru, On Fri, 16 Apr 2021 at 08:07, Alexandru Gagniuc wrote: Prepare the source tree for accepting implementations of the ECDSA algorithm. This patch deals with the boring aspects of Makefiles and Kconfig files. Signed-off-by: Alexandru Gagniuc --- include/image.h | 10 +- include/u-boot/rsa.h | 2 +- lib/Kconfig | 1 + lib/Makefile | 1 + lib/ecdsa/Kconfig| 23 +++ lib/ecdsa/Makefile | 1 + lib/ecdsa/ecdsa-verify.c | 13 + 7 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 lib/ecdsa/Kconfig create mode 100644 lib/ecdsa/Makefile create mode 100644 lib/ecdsa/ecdsa-verify.c Reviewed-by: Simon Glass nit below diff --git a/include/image.h b/include/image.h index 3ff3c035a7..9b95f6783b 100644 --- a/include/image.h +++ b/include/image.h @@ -1224,20 +1224,20 @@ 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_RSA 1 # define IMAGE_ENABLE_VERIFY_ECDSA1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include # else # define IMAGE_ENABLE_SIGN0 -# define IMAGE_ENABLE_VERIFY 0 +# define IMAGE_ENABLE_VERIFY_RSA 0 # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(RSA_VERIFY) -# define IMAGE_ENABLE_VERIFY_ECDSA 0 +# define IMAGE_ENABLE_VERIFY_RSA CONFIG_IS_ENABLED(RSA_VERIFY) +# define IMAGE_ENABLE_VERIFY_ECDSA CONFIG_IS_ENABLED(ECDSA_VERIFY) Since we are using Kconfig now, can we drop this IMAGE_... stuff and just use CONFIG_IS_ENABLED() in the code? CONFIG_IS_ENABLED() doesn't work for host tools. Alex
Re: [PATCH v4 2/6] lib: ecdsa: Add skeleton to implement ecdsa verification in u-boot
On 4/23/21 11:56 PM, Simon Glass wrote: Hi Tom, Alex, On Fri, 23 Apr 2021 at 12:47, Tom Rini wrote: On Fri, Apr 23, 2021 at 11:55:57AM +1200, Simon Glass wrote: Hi Alex, On Thu, 22 Apr 2021 at 07:30, Alex G. wrote: On 4/21/21 2:15 AM, Simon Glass wrote: Hi Alexandru, On Fri, 16 Apr 2021 at 08:07, Alexandru Gagniuc wrote: Prepare the source tree for accepting implementations of the ECDSA algorithm. This patch deals with the boring aspects of Makefiles and Kconfig files. Signed-off-by: Alexandru Gagniuc --- include/image.h | 10 +- include/u-boot/rsa.h | 2 +- lib/Kconfig | 1 + lib/Makefile | 1 + lib/ecdsa/Kconfig| 23 +++ lib/ecdsa/Makefile | 1 + lib/ecdsa/ecdsa-verify.c | 13 + 7 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 lib/ecdsa/Kconfig create mode 100644 lib/ecdsa/Makefile create mode 100644 lib/ecdsa/ecdsa-verify.c Reviewed-by: Simon Glass nit below diff --git a/include/image.h b/include/image.h index 3ff3c035a7..9b95f6783b 100644 --- a/include/image.h +++ b/include/image.h @@ -1224,20 +1224,20 @@ 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_RSA 1 # define IMAGE_ENABLE_VERIFY_ECDSA1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include # else # define IMAGE_ENABLE_SIGN0 -# define IMAGE_ENABLE_VERIFY 0 +# define IMAGE_ENABLE_VERIFY_RSA 0 # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(RSA_VERIFY) -# define IMAGE_ENABLE_VERIFY_ECDSA 0 +# define IMAGE_ENABLE_VERIFY_RSA CONFIG_IS_ENABLED(RSA_VERIFY) +# define IMAGE_ENABLE_VERIFY_ECDSA CONFIG_IS_ENABLED(ECDSA_VERIFY) Since we are using Kconfig now, can we drop this IMAGE_... stuff and just use CONFIG_IS_ENABLED() in the code? CONFIG_IS_ENABLED() doesn't work for host tools. I wonder if that and IS_ENABLED() can be fixed? Not super easily? Some sort of seeing about cleaning up the code we share with userspace would be nice, yes. But it should also probably means that for the user side of things we always enable a bunch of stuff so that in the end we end up with (nearly) target-agnostic tools. (just to be clear, this discussion should not hold up this patch IMO) Yes and in fact at present we allow some things to be disabled in tools where we probably should not. My original question was about CONFIG_IS_ENABLED(). I wonder if it doesn't work because the CONFIG is not enabled or because of some other reason? CONFIG_IS_ENABLED() macro isn't available when compiling host tools. I suspect nobody implemented it host-side? Alex
Re: [PATCH v4 2/6] lib: ecdsa: Add skeleton to implement ecdsa verification in u-boot
On 5/4/21 11:58 AM, Simon Glass wrote: Hi Alex, On Thu, 29 Apr 2021 at 10:10, Simon Glass wrote: Hi Alex, On Mon, 26 Apr 2021 at 07:21, Alex G. wrote: On 4/23/21 11:56 PM, Simon Glass wrote: Hi Tom, Alex, On Fri, 23 Apr 2021 at 12:47, Tom Rini wrote: On Fri, Apr 23, 2021 at 11:55:57AM +1200, Simon Glass wrote: Hi Alex, On Thu, 22 Apr 2021 at 07:30, Alex G. wrote: On 4/21/21 2:15 AM, Simon Glass wrote: Hi Alexandru, On Fri, 16 Apr 2021 at 08:07, Alexandru Gagniuc wrote: Prepare the source tree for accepting implementations of the ECDSA algorithm. This patch deals with the boring aspects of Makefiles and Kconfig files. Signed-off-by: Alexandru Gagniuc --- include/image.h | 10 +- include/u-boot/rsa.h | 2 +- lib/Kconfig | 1 + lib/Makefile | 1 + lib/ecdsa/Kconfig| 23 +++ lib/ecdsa/Makefile | 1 + lib/ecdsa/ecdsa-verify.c | 13 + 7 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 lib/ecdsa/Kconfig create mode 100644 lib/ecdsa/Makefile create mode 100644 lib/ecdsa/ecdsa-verify.c Reviewed-by: Simon Glass nit below diff --git a/include/image.h b/include/image.h index 3ff3c035a7..9b95f6783b 100644 --- a/include/image.h +++ b/include/image.h @@ -1224,20 +1224,20 @@ 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_RSA 1 # define IMAGE_ENABLE_VERIFY_ECDSA1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include # else # define IMAGE_ENABLE_SIGN0 -# define IMAGE_ENABLE_VERIFY 0 +# define IMAGE_ENABLE_VERIFY_RSA 0 # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(RSA_VERIFY) -# define IMAGE_ENABLE_VERIFY_ECDSA 0 +# define IMAGE_ENABLE_VERIFY_RSA CONFIG_IS_ENABLED(RSA_VERIFY) +# define IMAGE_ENABLE_VERIFY_ECDSA CONFIG_IS_ENABLED(ECDSA_VERIFY) Since we are using Kconfig now, can we drop this IMAGE_... stuff and just use CONFIG_IS_ENABLED() in the code? CONFIG_IS_ENABLED() doesn't work for host tools. I wonder if that and IS_ENABLED() can be fixed? Not super easily? Some sort of seeing about cleaning up the code we share with userspace would be nice, yes. But it should also probably means that for the user side of things we always enable a bunch of stuff so that in the end we end up with (nearly) target-agnostic tools. (just to be clear, this discussion should not hold up this patch IMO) Yes and in fact at present we allow some things to be disabled in tools where we probably should not. My original question was about CONFIG_IS_ENABLED(). I wonder if it doesn't work because the CONFIG is not enabled or because of some other reason? CONFIG_IS_ENABLED() macro isn't available when compiling host tools. I suspect nobody implemented it host-side? I think it should map to IS_ENABLED(). But also, do we include kconfig.h in the tools? Just a note that I sent a series to enable CONFIG_IS_ENABLED on the host. Do I need to rebase on your series? Regards, Simon
Re: [PATCH v4 2/6] lib: ecdsa: Add skeleton to implement ecdsa verification in u-boot
On 5/5/21 1:43 PM, Simon Glass wrote: Hi Alex, On Wed, 5 May 2021 at 11:49, Alex G. wrote: On 5/4/21 11:58 AM, Simon Glass wrote: Hi Alex, On Thu, 29 Apr 2021 at 10:10, Simon Glass wrote: Hi Alex, On Mon, 26 Apr 2021 at 07:21, Alex G. wrote: On 4/23/21 11:56 PM, Simon Glass wrote: Hi Tom, Alex, On Fri, 23 Apr 2021 at 12:47, Tom Rini wrote: On Fri, Apr 23, 2021 at 11:55:57AM +1200, Simon Glass wrote: Hi Alex, On Thu, 22 Apr 2021 at 07:30, Alex G. wrote: On 4/21/21 2:15 AM, Simon Glass wrote: Hi Alexandru, On Fri, 16 Apr 2021 at 08:07, Alexandru Gagniuc wrote: Prepare the source tree for accepting implementations of the ECDSA algorithm. This patch deals with the boring aspects of Makefiles and Kconfig files. Signed-off-by: Alexandru Gagniuc --- include/image.h | 10 +- include/u-boot/rsa.h | 2 +- lib/Kconfig | 1 + lib/Makefile | 1 + lib/ecdsa/Kconfig| 23 +++ lib/ecdsa/Makefile | 1 + lib/ecdsa/ecdsa-verify.c | 13 + 7 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 lib/ecdsa/Kconfig create mode 100644 lib/ecdsa/Makefile create mode 100644 lib/ecdsa/ecdsa-verify.c Reviewed-by: Simon Glass nit below diff --git a/include/image.h b/include/image.h index 3ff3c035a7..9b95f6783b 100644 --- a/include/image.h +++ b/include/image.h @@ -1224,20 +1224,20 @@ 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_RSA 1 # define IMAGE_ENABLE_VERIFY_ECDSA1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include # else # define IMAGE_ENABLE_SIGN0 -# define IMAGE_ENABLE_VERIFY 0 +# define IMAGE_ENABLE_VERIFY_RSA 0 # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(RSA_VERIFY) -# define IMAGE_ENABLE_VERIFY_ECDSA 0 +# define IMAGE_ENABLE_VERIFY_RSA CONFIG_IS_ENABLED(RSA_VERIFY) +# define IMAGE_ENABLE_VERIFY_ECDSA CONFIG_IS_ENABLED(ECDSA_VERIFY) Since we are using Kconfig now, can we drop this IMAGE_... stuff and just use CONFIG_IS_ENABLED() in the code? CONFIG_IS_ENABLED() doesn't work for host tools. I wonder if that and IS_ENABLED() can be fixed? Not super easily? Some sort of seeing about cleaning up the code we share with userspace would be nice, yes. But it should also probably means that for the user side of things we always enable a bunch of stuff so that in the end we end up with (nearly) target-agnostic tools. (just to be clear, this discussion should not hold up this patch IMO) Yes and in fact at present we allow some things to be disabled in tools where we probably should not. My original question was about CONFIG_IS_ENABLED(). I wonder if it doesn't work because the CONFIG is not enabled or because of some other reason? CONFIG_IS_ENABLED() macro isn't available when compiling host tools. I suspect nobody implemented it host-side? I think it should map to IS_ENABLED(). But also, do we include kconfig.h in the tools? Just a note that I sent a series to enable CONFIG_IS_ENABLED on the host. Do I need to rebase on your series? Normally the series that is reviewed first is applied first, then it is up to the subsequent series (i.e. mine) to rebase on that. It gets a bit out of hand if people send a patch, it is reviewed, then it has to be reworked later after someone does another patch that didn't exist then! I am sure it happens sometimes, though. It's up to Tom. Having said that, if you can do a fix-up patch on top of my series I think it would be handy. I'd rather work with you than race condition against you. If you're willing to plow through review on your series, and get it merged soon, I'll take the red pill and rebase my series. Alex Regards, Simon
Re: [PATCH v4 2/5] psci: add features/reset2 support
On 4/20/21 9:21 AM, Tom Rini wrote: On Thu, Apr 01, 2021 at 02:01:53AM +0300, Igor Opaniuk wrote: From: Igor Opaniuk Adds support for: * PSCI_FEATURES, which was introduced in PSCI 1.0. This provides API that allows discovering whether a specific PSCI function is implemented and its features. * SYSTEM_RESET2, which was introduced in PSCI 1.1, which extends existing SYSTEM_RESET. It provides support for vendor-specific resets, providing reset_type as an additional param. For additional details visit [1]. Implementations of some functions were borrowed from Linux PSCI driver code [2]. [1] https://developer.arm.com/documentation/den0022/latest/ [2] drivers/firmware/psci/psci.c Signed-off-by: Igor Opaniuk Applied to u-boot/master, thanks! I'm seeing a build failure on stm32mp1 from this patch: drivers/firmware/psci.c:69:12: error: conflicting types for 'psci_features' 69 | static int psci_features(u32 psci_func_id) |^ In file included from drivers/firmware/psci.c:23: ./arch/arm/include/asm/system.h:548:5: note: previous declaration of 'psci_features' was here 548 | s32 psci_features(u32 function_id, u32 psci_fid); | ^ make[2]: *** [scripts/Makefile.build:266: drivers/firmware/psci.o] Error 1 make[1]: *** [scripts/Makefile.build:419: drivers/firmware] Error 2
Re: [RESEND PATCH v1] psci: rename psci_features function
On 5/5/21 3:54 PM, Igor Opaniuk wrote:
From: Igor Opaniuk
s/psci_features/psci_features_req/g for the case when both
ARCH_SUPPORT_PSCI=y and ARM_PSCI_FW=y, that leads to these
compilation issues:
drivers/firmware/psci.c:69:12: error: conflicting types for 'psci_features'
69 | static int psci_features(u32 psci_func_id)
|^
In file included from drivers/firmware/psci.c:23:
./arch/arm/include/asm/system.h:548:5: note: previous declaration of
'psci_features' was heremailto:igor.opan...@foundries.io
548 | s32 psci_features(u32 function_id, u32 psci_fid);
| ^
Signed-off-by: Igor Opaniuk
Reported-by: Alexandru Gagniuc
Tested-by: Alexandru Gagniuc
---
drivers/firmware/psci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index be57552aba..f1841a1835 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -66,7 +66,7 @@ unsigned long __efi_runtime invoke_psci_fn
return res.a0;
}
-static int psci_features(u32 psci_func_id)
+static int psci_features_req(u32 psci_func_id)
{
return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
psci_func_id, 0, 0);
@@ -85,7 +85,7 @@req(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
static bool psci_is_system_reset2_supported(void)
ver = psci_0_2_get_version();
if (PSCI_VERSION_MAJOR(ver) >= 1) {
- ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
+ ret = psci_features_req(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
Can you include a verb in the function name? For example
request_psci_features(), or call_psci_features()
Alex
Re: [PATCH 7/8] spl: fit: Replace #ifdef blocks with more readable constructs
On 12/18/20 8:29 PM, Simon Glass wrote: On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc wrote: Use the IS_ENABLED() macro to control code flow, instead of the caveman approach of sprinkling #ifdefs. Code size is not affected, as the linker garbage-collects unused functions. However, readability is improved significantly. Signed-off-by: Alexandru Gagniuc --- common/spl/spl_fit.c | 53 1 file changed, 24 insertions(+), 29 deletions(-) I am trying to imagine stick drawings with #ifdefs #ifdef #if #if #if #if #if #if #if #if #endif #if#if#if #if #if #if #if#if#if #if #if #if #ifdef #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #endif #ifdef #if #if #if #if #if #if #if #if #endif #endif Reviewed-by: Simon Glass
Re: [PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct
On 12/18/20 8:28 PM, Simon Glass wrote:
Hi Alexandru,
On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc wrote:
The logical steps in spl_load_simple_fit() are difficult to follow.
I think the long comments, ifdefs, and ungodly number of variables
seriously affect the readability. In particular, it violates section 6
of the coding style, paragraphs (3), and (4).
The purpose of this patch is to improve the situation by
- Factoring out initialization and parsing to separate functions
- Reduce the number of variables by using a context structure
This change introduces no functional changes.
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl_fit.c | 87 ++--
1 file changed, 60 insertions(+), 27 deletions(-)
This certainly looks a lot better although your email address does not
inspire confidence...
Don't worry. It doesn't bite.
Do you think you could look at creating a sandbox SPL test for this?
It should be possible to write it in C, set up a bit of data, call
your function and check the results.
I can look at it. I can't promise anything though, since this is the
first time I heard of the sandbox. Maybe doc knows more.
Alex
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 1b4a7f6b15..a6f85b6f9d 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
#define CONFIG_SYS_BOOTM_LEN (64 << 20)
#endif
+struct spl_fit_info {
+ const void *fit;
+ size_t ext_data_offset;
+ int images_node;
+};
struct comments
Re: [PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct
On 12/21/20 2:23 PM, Simon Glass wrote:
Hi Alex,
On Mon, 21 Dec 2020 at 12:28, Alex G. wrote:
On 12/18/20 8:28 PM, Simon Glass wrote:
Hi Alexandru,
On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc wrote:
The logical steps in spl_load_simple_fit() are difficult to follow.
I think the long comments, ifdefs, and ungodly number of variables
seriously affect the readability. In particular, it violates section 6
of the coding style, paragraphs (3), and (4).
The purpose of this patch is to improve the situation by
- Factoring out initialization and parsing to separate functions
- Reduce the number of variables by using a context structure
This change introduces no functional changes.
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl_fit.c | 87 ++--
1 file changed, 60 insertions(+), 27 deletions(-)
This certainly looks a lot better although your email address does not
inspire confidence...
Don't worry. It doesn't bite.
Do you think you could look at creating a sandbox SPL test for this?
It should be possible to write it in C, set up a bit of data, call
your function and check the results.
I can look at it. I can't promise anything though, since this is the
first time I heard of the sandbox. Maybe doc knows more.
Yes, see doc/arch/sandbox.rst
test/dm/Makefile shows that only one test file is enabled for SPL, but
you can add more. Let me know if you need pointers.
These aliases might help, if you build into /tmp/b/ :
# Run a pytest on sandbox
# $1: Name of test to run (optional, else run all)
function pyt {
test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
}
# Run a pytest on sandbox_spl
# $1: Name of test to run (optional, else run all SPL tests)
function pytspl {
local run=$1
[[ -z "$run" ]] && run=spl
test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
}
You're thinking way ahead of where I am. I know how to build a board,
but I've never used the test infrastructure. After some fumbling, I
figured I'd try sandbox_spl:
$ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
As you can imagine, it kept complaining about SDL. I've never used
environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
But then why would we need SDL for testing an SPL build anyway? 'swig'
was missing too, but that was an easy fix.
Second try:
$ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
--build
Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
normal?
What I seem to be missing is how to connect this python to calling
spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
FIT image -- boots, okay.
Alex
Regards,
Simon
Alex
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 1b4a7f6b15..a6f85b6f9d 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
#define CONFIG_SYS_BOOTM_LEN (64 << 20)
#endif
+struct spl_fit_info {
+ const void *fit;
+ size_t ext_data_offset;
+ int images_node;
+};
struct comments
Regards,
Simon
Re: [PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct
On 12/28/20 9:33 PM, Simon Glass wrote:
Hi Alex,
On Mon, 21 Dec 2020 at 15:24, Alex G. wrote:
On 12/21/20 2:23 PM, Simon Glass wrote:
Hi Alex,
On Mon, 21 Dec 2020 at 12:28, Alex G. wrote:
On 12/18/20 8:28 PM, Simon Glass wrote:
Hi Alexandru,
On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc wrote:
The logical steps in spl_load_simple_fit() are difficult to follow.
I think the long comments, ifdefs, and ungodly number of variables
seriously affect the readability. In particular, it violates section 6
of the coding style, paragraphs (3), and (4).
The purpose of this patch is to improve the situation by
- Factoring out initialization and parsing to separate functions
- Reduce the number of variables by using a context structure
This change introduces no functional changes.
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl_fit.c | 87 ++--
1 file changed, 60 insertions(+), 27 deletions(-)
This certainly looks a lot better although your email address does not
inspire confidence...
Don't worry. It doesn't bite.
Do you think you could look at creating a sandbox SPL test for this?
It should be possible to write it in C, set up a bit of data, call
your function and check the results.
I can look at it. I can't promise anything though, since this is the
first time I heard of the sandbox. Maybe doc knows more.
Yes, see doc/arch/sandbox.rst
test/dm/Makefile shows that only one test file is enabled for SPL, but
you can add more. Let me know if you need pointers.
These aliases might help, if you build into /tmp/b/ :
# Run a pytest on sandbox
# $1: Name of test to run (optional, else run all)
function pyt {
test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
}
# Run a pytest on sandbox_spl
# $1: Name of test to run (optional, else run all SPL tests)
function pytspl {
local run=$1
[[ -z "$run" ]] && run=spl
test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
}
You're thinking way ahead of where I am. I know how to build a board,
but I've never used the test infrastructure. After some fumbling, I
figured I'd try sandbox_spl:
$ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
As you can imagine, it kept complaining about SDL. I've never used
environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
But then why would we need SDL for testing an SPL build anyway? 'swig'
was missing too, but that was an easy fix.
Second try:
$ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
--build
Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
normal?
What I seem to be missing is how to connect this python to calling
spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
FIT image -- boots, okay.
Here's a suggestoin
- Write a function that calls the function to load a fit and does some
checks that it worked correct, e.g. by looking in memory
- put a call to that function in an SPL C test (as mentioned ealler)
I suppose you could also boot it, perhaps by switching sandbox to use
FIT to boot?
Hi Simon,
There seems to be a lot more to wrapping around spl_load_simple_fit().
We need populated spl_image_info spl_load_info structure. I'm not even
sure if the test code runs in SPL, or how to run it in SPL.
There are examples, and unfocused documentation on how to connect this
into u-boot proper. The current documentation and exampples are not
helping with what I was trying to accomplish. Unfortunately, I've spent
a week on this, and wasn't able to make any progress. I'm one guy who's
getting paid to ship a product. This test infrastructure is more tedious
than I anticipated, and I need to move on.
ALex
Re: [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
On 1/7/21 6:35 AM, Simon Glass wrote:
Hi Alexandru,
Hi Simon,
(pun alert!) A lot of your comments have to do with comments. I use
comments as a tool to add something of value to code. When the code is
self-documenting, comments don't help much.
See kernel coding style chapter 8.
What comments can do, is increase code size and break code flow -- there
is a cost to having them. I'm certain you've seen functions that need to
be scrolled up and down because of a huge comment smack in the middle.
I'll address individual comment comments below.
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc wrote:
mkimage supports rsa2048, and rsa4096 signatures. With newer silicon
now supporting hardware-accelerated ECDSA, it makes sense to expand
signing support to elliptic curves.
Implement host-side ECDSA signing and verification with libcrypto.
Device-side implementation of signature verification is beyond the
scope of this patch.
Signed-off-by: Alexandru Gagniuc
---
common/image-sig.c | 14 +-
include/u-boot/ecdsa.h | 27
lib/ecdsa/ecdsa-libcrypto.c | 300
tools/Makefile | 3 +
4 files changed, 342 insertions(+), 2 deletions(-)
create mode 100644 include/u-boot/ecdsa.h
create mode 100644 lib/ecdsa/ecdsa-libcrypto.c
diff --git a/common/image-sig.c b/common/image-sig.c
index 21dafe6b91..2548b3eb0f 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -15,6 +15,7 @@
DECLARE_GLOBAL_DATA_PTR;
#endif /* !USE_HOSTCC*/
#include
+#include
#include
#include
@@ -82,8 +83,17 @@ struct crypto_algo crypto_algos[] = {
.sign = rsa_sign,
.add_verify_data = rsa_add_verify_data,
.verify = rsa_verify,
- }
-
+ },
+#if defined(USE_HOSTCC)
+ /* Currently, only host support exists for ECDSA */
+ {
+ .name = "ecdsa256",
+ .key_len = ECDSA256_BYTES,
+ .sign = ecdsa_sign,
+ .add_verify_data = ecdsa_add_verify_data,
+ .verify = ecdsa_verify,
+ },
+#endif
};
struct padding_algo padding_algos[] = {
diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h
new file mode 100644
index 00..a13a7267e1
--- /dev/null
+++ b/include/u-boot/ecdsa.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Alexandru Gagniuc .
+ */
+
+#ifndef _ECDSA_H
+#define _ECDSA_H
+
+#include
+#include
+
+/**
+ * crypto_algo API impementation for ECDSA;
+ * @see "struct crypt_algo"
+ * @{
+ */
+int ecdsa_sign(struct image_sign_info *info, const struct image_region
region[],
+ int region_count, uint8_t **sigp, uint *sig_len);
+int ecdsa_verify(struct image_sign_info *info,
+const struct image_region region[], int region_count,
+uint8_t *sig, uint sig_len);
+int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest);
Please always add full comments when you export functions, or have a
non-trivial static function.
I disagree. These functions implement and are designed to be used via
the crypt_algo API. One should understand the crypt_algo API, and any
deviations in behavior would be a bug. So there is no scenario in which
comments here would be useful.
+/** @} */
+
+#define ECDSA256_BYTES (256 / 8)
+
+#endif
diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c
new file mode 100644
index 00..ff491411d0
--- /dev/null
+++ b/lib/ecdsa/ecdsa-libcrypto.c
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ECDSA image signing implementation using libcrypto backend
+ *
+ * The signature is a binary representation of the (R, S) points, padded to the
+ * key size. The signature will be (2 * key_size_bits) / 8 bytes.
+ *
+ * Deviations from behavior of RSA equivalent:
+ * - Verification uses private key. This is not technically required, but a
+ *limitation on how clumsy the openssl API is to use.
I'm not quite sure what the implications are on this. If this is
public key crypto, the private key is not available, so how can you
verify with it?
I presume this is fixable, but only as an academic exercise. This file
is for mkimage, which doesn't do standalone verification. The way you
verify is in u-boot. That is the subject of another series.
+ * - Handling of keys and key paths:
+ *- The '-K' key directory option must contain path to the key file,
+ * instead of the key directory.
If we make this change we should update RSA to do the same.
Of course, but is there agreement as to this direction? There seem to be
some hidden subtleties to key-name-hint that I don't fully understand yet.
+ *- No assumptions are made about the file extension of the key
+ *- The 'key-name-hint' property is only used for naming devicetree nodes,
+ * but is not used for looking up keys on the filesystem.
+ *
+ * Copyright (c) 2020, Alexandru Gagni
Re: [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
On 1/7/21 6:35 AM, Simon Glass wrote:
Hi Alexandru,
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc wrote:
Add a test to make sure that the ECDSA signatures generated by
mkimage can be verified successfully. pyCryptodomex was chosen as the
crypto library because it integrates much better with python code.
Using openssl would have been unnecessarily painful.
Signed-off-by: Alexandru Gagniuc
---
test/py/tests/test_fit_ecdsa.py | 111
1 file changed, 111 insertions(+)
create mode 100644 test/py/tests/test_fit_ecdsa.py
This test looks fine but the functions need full comments. I do think
it might be worth putting the code in test_vboot, particularly when
you get to the sandbox implementation.
test_vboot seems to be testing the bootm command, while with this test
I'm only looking to test the host-side (mkimage). In the next series, I
won't have a software implementation of ECDSA, like RSA_MOD_EXP. I will
use the ROM on the stm32mp. So there won't be somthing testable in the
sandbox.
For installing the Python library, you might need to update the docs
in test/ and perhaps install things in .gitlab-ci.yml and .azure...
Sure, let me look into this.
diff --git a/test/py/tests/test_fit_ecdsa.py b/test/py/tests/test_fit_ecdsa.py
new file mode 100644
index 00..957964d329
--- /dev/null
+++ b/test/py/tests/test_fit_ecdsa.py
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (c) 2020, Alexandru Gagniuc
+
+"""
+Test ECDSA signing of FIT images
+
+This test uses mkimage to sign an existing FIT image with an ECDSA key. The
+signature is then extracted, and verified against pyCryptodome.
+This test doesn't run the sandbox. It only checks the host tool 'mkimage'
+"""
+
+import pytest
+import u_boot_utils as util
+from Cryptodome.Hash import SHA256
+from Cryptodome.PublicKey import ECC
+from Cryptodome.Signature import DSS
+
+class SignableFitImage(object):
+""" Helper to manipulate a FIT image on disk """
+def __init__(self, cons, file_name):
+self.fit = file_name
+self.cons = cons
+self.signable_nodes = set()
+
+def __fdt_list(self, path):
+return util.run_and_log(self.cons, f'fdtget -l {self.fit} {path}')
+
+def __fdt_set(self, node, **prop_value):
+for prop, value in prop_value.items():
+util.run_and_log(self.cons, f'fdtput -ts {self.fit} {node} {prop}
{value}')
+
+def __fdt_get_binary(self, node, prop):
+numbers = util.run_and_log(self.cons, f'fdtget -tbi {self.fit} {node}
{prop}')
+
+bignum = bytearray()
+for little_num in numbers.split():
+bignum.append(int(little_num))
+
+return bignum
+
+def find_signable_image_nodes(self):
+for node in self.__fdt_list('/images').split():
+image = f'/images/{node}'
+if 'signature' in self.__fdt_list(image):
+self.signable_nodes.add(image)
+
+return self.signable_nodes
+
+def change_signature_algo_to_ecdsa(self):
+for image in self.signable_nodes:
+self.__fdt_set(f'{image}/signature', algo='sha256,ecdsa256')
+
+def sign(self, mkimage, key_file):
+util.run_and_log(self.cons, [mkimage, '-F', self.fit, f'-k{key_file}'])
+
+def check_signatures(self, key):
+for image in self.signable_nodes:
+raw_sig = self.__fdt_get_binary(f'{image}/signature', 'value')
+raw_bin = self.__fdt_get_binary(image, 'data')
+
+sha = SHA256.new(raw_bin)
+verifier = DSS.new(key, 'fips-186-3')
+verifier.verify(sha, bytes(raw_sig))
+
+
+@pytest.mark.buildconfigspec('fit_signature')
+@pytest.mark.requiredtool('dtc')
+@pytest.mark.requiredtool('fdtget')
+@pytest.mark.requiredtool('fdtput')
+def test_fit_ecdsa(u_boot_console):
+"""TODO: Document me
+"""
+def generate_ecdsa_key():
+return ECC.generate(curve='prime256v1')
+
+def assemble_fit_image(dest_fit, its, destdir):
+dtc_args = f'-I dts -O dtb -i {destdir}'
+util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f', its, dest_fit])
+
+def dtc(dts):
+dtb = dts.replace('.dts', '.dtb')
+util.run_and_log(cons, f'dtc {datadir}/{dts} -O dtb -o
{tempdir}/{dtb}')
+
+cons = u_boot_console
+mkimage = cons.config.build_dir + '/tools/mkimage'
+datadir = cons.config.source_dir + '/test/py/tests/vboot/'
+tempdir = cons.config.result_dir
+key_file = f'{tempdir}/ecdsa-test-key.pem'
+fit_file = f'{tempdir}/test.fit'
+dtc('sandbox-kernel.dts')
+
+key = generate_ecdsa_key()
+
+# Create a number kernel image with zeroes
+with open(f'{tempdir}/test-kernel.bin', 'w') as fd:
+fd.write(500 * chr(0))
+
+with open(key_file, 'w') as f:
+f.write(key.export_key(format='PEM'))
+
+assemble_fit_image(fit_file, f'{datadir}/sign-images-sha256.its', tempdir)
+
+fit = SignableFitImage(cons, fit_file
Re: [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
On 1/7/21 11:31 AM, Simon Glass wrote: Hi Alex, On Thu, 7 Jan 2021 at 09:44, Alex G. wrote: On 1/7/21 6:35 AM, Simon Glass wrote: Hi Alexandru, On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc wrote: Add a test to make sure that the ECDSA signatures generated by mkimage can be verified successfully. pyCryptodomex was chosen as the crypto library because it integrates much better with python code. Using openssl would have been unnecessarily painful. Signed-off-by: Alexandru Gagniuc --- test/py/tests/test_fit_ecdsa.py | 111 1 file changed, 111 insertions(+) create mode 100644 test/py/tests/test_fit_ecdsa.py This test looks fine but the functions need full comments. I do think it might be worth putting the code in test_vboot, particularly when you get to the sandbox implementation. test_vboot seems to be testing the bootm command, while with this test It also runs fit_check_sign to check the signature. Hmm, it backends on tools/check_fit_sign. Would be an interesting execrise to extend it ecdsa signatures, but that would take significantly more effort than the simple test I am proposing here. I'm only looking to test the host-side (mkimage). In the next series, I won't have a software implementation of ECDSA, like RSA_MOD_EXP. I will use the ROM on the stm32mp. So there won't be somthing testable in the sandbox. I'm not sure that is a good idea. With driver model you'll end up creating a ECDSA driver I suppose, so implementing it for sandbox should be possible. Is it a complicated algorithm? A software implementation of ECDSA is outside the scope of my project. Without that, I'm not even sure how fit_check_sign could work? It uses the ops->verify in ecdsa-libcrypto, does it not? [..] Regards, Simon
Re: [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
On 1/7/21 11:29 AM, Simon Glass wrote: Hi Alex, On Thu, 7 Jan 2021 at 09:27, Alex G. wrote: On 1/7/21 6:35 AM, Simon Glass wrote: Hi Alexandru, Hi Simon, (pun alert!) A lot of your comments have to do with comments. I use comments as a tool to add something of value to code. When the code is self-documenting, comments don't help much. See kernel coding style chapter 8. What comments can do, is increase code size and break code flow -- there is a cost to having them. I'm certain you've seen functions that need to be scrolled up and down because of a huge comment smack in the middle. I'll address individual comment comments below. Don't get me started on comments in the kernel...there seems to almost be a ban on them :-) We used to follow the same approach but now we have comments for non-trivial code. Comments and tests are closely related. - if there is no comment, we don't know what the function is supposed to do so we can't change it (there is no contract), we are not sure if line 5 is a bug or a real quirk, casual readers can't understand what is going on, the automated docs don't produce anything, no one wants to refactor it, etc. - if there is no test, presumably the code doesn't work now, if it ever did I will add each of the comments you are requesting, but not for the reasons quoted. The comments that I could write won't add anything of value -- they would just make the code larger, and increase the cognitive load. You are the maintainer, and starting an argument would be pointless. In the end, comments don't get compiled, and the code works just the same :) [snip] Again, this is not Linux and people don't have as much time to cogitate on code. I resent that statement. It takes me longer to do a task in u-boot than it would take me to do a similar task in linux. In the context of comments (note I intentionally did not say 'documentation'), does plastering the same information over and over in a way that hides the essence really help? I think the current path is misguided, and will only aggravate the problem. (I'll add the comments, though) They are just trying to get their device working. That's true for linux too. [snip] All other comments will be addressed in v3
Re: [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
On 1/7/21 11:25 AM, Tom Rini wrote: On Thu, Jan 07, 2021 at 10:27:50AM -0600, Alex G. wrote: On 1/7/21 6:35 AM, Simon Glass wrote: Hi Alexandru, Hi Simon, (pun alert!) A lot of your comments have to do with comments. I use comments as a tool to add something of value to code. When the code is self-documenting, comments don't help much. See kernel coding style chapter 8. Comments for comments sake are bad. Comments so that we can also have reasonable generated documentation are good. Function prototypes fall in to that second category to me, with few exceptions (and that we have lots of bad examples isn't a good reason). The function names may well make it obvious what's going on but the comments allow for generated documentation to include that when explaining the not so obvious parts. Thanks! The problem with generated documentation is that it's not very useful. People add comments for the sake of comments to have something generated. Most often you end up with a detailed description of all the details, but almost never the big picture. Nowadays, I don't even waste my time reading generated documentation. I am getting ready to send the new series with the goodies requested by Simon. I don't find the new comments to be useful, and I find some to spread out the code such that it hurts readability. They will be there because you and Simon asked nicely, although I think you're wrong. Alex
Re: [PATCH] lib: rsa: rsa-verify: don't look for keys in the FIT image
On 1/12/21 12:18 PM, Philippe Reynes wrote:
Hi Philippe,
In the function rsa_verify_hash, if the "main" key doesn't
work, u-boot try others keys. But it searches those keys
in the FIT image instead of the u-boot device tree.
Signed-off-by: Philippe Reynes
---
lib/rsa/rsa-verify.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index 0ab0f629d0..e34d3293d1 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -522,10 +522,10 @@ int rsa_verify_hash(struct image_sign_info *info,
return ret;
/* No luck, so try each of the keys in turn */
- for (ndepth = 0, noffset = fdt_next_node(info->fit, sig_node,
+ for (ndepth = 0, noffset = fdt_next_node(blob, sig_node,
&ndepth);
(noffset >= 0) && (ndepth > 0);
-noffset = fdt_next_node(info->fit, noffset, &ndepth)) {
+noffset = fdt_next_node(blob, noffset, &ndepth)) {
if (ndepth == 1 && noffset != node) {
ret = rsa_verify_with_keynode(info, hash,
sig, sig_len,
I was looking at this code ot too long ago and didn't notice the
inconsistency. I think it would be better to use 'info->fdt_blob' and
get rid of 'blob' completely.
Alex
Re: [PATCH v3 3/6] lib: Add support for ECDSA image signing
On 1/13/21 10:10 AM, Simon Glass wrote: On Thu, 7 Jan 2021 at 15:34, Alexandru Gagniuc wrote: mkimage supports rsa2048, and rsa4096 signatures. With newer silicon now supporting hardware-accelerated ECDSA, it makes sense to expand signing support to elliptic curves. Implement host-side ECDSA signing and verification with libcrypto. Device-side implementation of signature verification is beyond the scope of this patch. Signed-off-by: Alexandru Gagniuc --- common/image-sig.c | 11 +- include/image.h | 3 + include/u-boot/ecdsa.h | 94 +++ lib/ecdsa/ecdsa-libcrypto.c | 306 tools/Makefile | 3 + 5 files changed, 415 insertions(+), 2 deletions(-) create mode 100644 include/u-boot/ecdsa.h create mode 100644 lib/ecdsa/ecdsa-libcrypto.c Reviewed-by: Simon Glass But you should check the return value of do_sign(). Why do you call ecdsa_check_signature() afterwards? Can you not trust the library? You are right that it's a redundant step. It's an automatic test for the ecdsa_sig_encode_raw()/ecdsa_sig_from_raw() pair. Alex Regards, Simon
Re: [PATCH 1/5] dm: crypto: Define UCLASS API for ECDSA signature verification
On 1/13/21 10:10 AM, Simon Glass wrote: Hi Alexandru, On Mon, 11 Jan 2021 at 08:41, Alexandru Gagniuc wrote: Define a UCLASS API for verifying ECDSA signatures. Unlike UCLASS_MOD_EXP, which focuses strictly on modular exponentiation, the ECDSA class focuses on verification. This is done so that it better aligns with mach-specific implementations, such as stm32mp. Signed-off-by: Alexandru Gagniuc --- include/crypto/ecdsa-uclass.h | 39 +++ include/dm/uclass-id.h| 1 + 2 files changed, 40 insertions(+) create mode 100644 include/crypto/ecdsa-uclass.h This needs a test, as do all uclasses in U-Boot. If it isn't easy to implement the algorithm then I suppose you could fake it by using an easy algorithm like md5, but it does need a test. I agree. I couldn't find a test for UCLASS_MOD_EXP (for guidance), so I'm not sure where to even start. Alex Regards, Simon
Re: [PATCH v3 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()
On 1/15/21 8:33 PM, Tom Rini wrote:
On Wed, Dec 23, 2020 at 08:44:05AM -0600, Alexandru Gagniuc wrote:
The size is derived from the FIT image itself. Any alignment
requirements are machine-specific and known by the board code. Thus
the total length can be derived from the FIT image and knowledge of
the platform. The 'length' argument is redundant. Remove it.
Signed-off-by: Alexandru Gagniuc
Reviewed-by: Peng Fan
Reviewed-by: Simon Glass
---
arch/arm/mach-imx/spl.c | 5 +++--
common/spl/spl_fit.c| 4 ++--
include/spl.h | 4 ++--
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index aa2686bb92..11255798d3 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
DECLARE_GLOBAL_DATA_PTR;
@@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
return size;
}
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+void board_spl_fit_post_load(const void *fit)
{
- u32 offset = length - CONFIG_CSF_SIZE;
+ u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
if (imx_hab_authenticate_image(load_addr,
offset + IVT_SIZE + CSF_PAD_SIZE,
OK, this is a problem right here. First, the code no longer compiles as
we don't pass in "load_addr", which is what
imx_hab_authenticate_image() takes to know where in DDR the image is to
validate. While I could probably work out that value from what we use
now for offset, I would rather someone that can test the code do so.
Thanks!
Hi Tom,
I'm sorry I missed that. I seemed to have, again, solved the hard
problem and choked on something simple. Being able to eliminate the
'length' argument is essential simplifying the FIT code, and the rest of
this series. Fixing the compilation issue is trivial, but how do we get
this tested? Do you know someone with the hardware who'd be willing to
give it a shot?
Alex
Re: [PATCH] mmc: stm32_sdmmc2: Use mmc_of_parse() to read host capabilities
On 9/10/20 11:04 AM, Patrick DELAUNAY wrote: Hi Alexandru, Hi [snip] + cfg->f_max = 5200; + mmc_of_parse(dev, cfg); Result of mmc_of_parse is not tested ? I proposed: + ret = mmc_of_parse(dev, cfg); + if (ret) + return ret; You're right. I'll get that updated. I test this patch with a trace in stm32_sdmmc2_probe on STM32MP157C-EV1 [snip] I think that it is a issue U-Boot, in mmc uclass in mmc_of_parse(): if (dev_read_bool(dev, "cap-mmc-highspeed")) - cfg->host_caps |= MMC_CAP(MMC_HS); + cfg->host_caps |= MMC_CAP(MMC_HS) | MMC_CAP(MMC_HS_52); In U-Boot this capability is splitted in 2 bits = one for 26MHz one for 52MHz And for card side it is managed on card side by mmc_get_capabilities() I agree. I am preparing a patch to address this. [snip] 2) some host caps can be added in device tree (they are supported by SOC and by Linux driver) but they are not supported by U-Boot driver, for example: #define MMC_MODE_DDR_52MHz MMC_CAP(MMC_DDR_52) #define MMC_MODE_HS200 MMC_CAP(MMC_HS_200) #define MMC_MODE_HS400 MMC_CAP(MMC_HS_400) #define MMC_MODE_HS400_ES MMC_CAP(MMC_HS_400_ES) I afraid (I don't sure) that this added caps change the mmc core behavior in U-Boot = U-Boot try to select the high speed mode even if not supported by driver Two issues here. The first is when devicetree enables an unsupported mode, say "mmc-hs400-1_2v". That's a devicetree bug, and not something we should fix in the code. Hypothetical exam: DT says bus-width = <8>; but only four lines are routed on the board. The second issue is what happens when somebody plugs in a UHS SD card? UHS support is not enabled by default in the stm32mp1 defconfigs, so the mmc core won't try to run it at UHS. Now if somebody were to enable UHS manually: CONFIG_MMC_IO_VOLTAGE=y CONFIG_MMC_UHS_SUPPORT=y Then yes, the UHS switch will be attempted, fail, and the card will not be detected. To work around that, one could implement a .wait_dat0() that returns an error other than -ENOSYS. This would cause mmc_switch_voltage() to fail, making the mmc core to leave the card at default speeds. My argument is that it takes conscious effort to break things in the first place, so it's not a situation we should worry about. The host_caps bitfield should be limited by the supported features in the driver (or remove the unsupported features) [snip] cfg->host_caps &= SDMMC_SUPPORTED_CAPS; or cfg->host_caps |= ~SDMMC_UNSUPPORTED_CAPS; I think this sort of playing with the flags will cause more confusion. People would expect this to come from DT. For example, somebody sets "sd-uhs-ddr52" in devicetree. It's more confusing to check host_caps, and find that MMC_MODE_DDR_52MHz is not set than it is to see the driver trying to place the card in DDR52 and failing. Alex
Re: [PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing
On 3/9/21 5:55 PM, Farhan Ali wrote:
This change adds a callback for preprocessing the FIT header before
it is parsed. There are 3 main reasons for this callback:
(1) If a vulnerability is discovered in the FIT parsing/loading code,
or libfdt, this callback allows users to scan the FIT header for
specific exploit signatures and prevent flashing/booting of the image
(2) If users want to implement a single signature which covers the
entire FIT header, which is then appended to the end of the header,
then this callback can be used to authenticate that signature.
(3) If users want to check the FIT header contents against specific
metadata stored outside the FIT header, then this callback allows
them to do that.
Hi Fahran,
This patch describes "how" you're trying to achieve it, but "what" you
want to achieve. I'll get later into why I think the "how" is
fundamentally flawed.
There should be at least a use case implemented in this series. When
someone notices an empty function that isn't used, the first instinct
would be to submit a patch to remove it. But more importantly, seeing an
actual example of "what" you are trying to achieve will help others
suggest a better way on "how" to achieve it.
Second issue is that spl_simple_fit_read() is intended to bring a FIT
image to memory. If you need to make decisions on the content of that
image, then spl_simple_fit_read() is the wrong place to do it. A better
place might be spl_simple_fit_parse().
The third issue is that whatever the end goal is, you're trying to
achieve it with weak functions. Weak functions aren't always bad. There
are a limited number of cases where they work very well for the purpose
-- I've even used them before. But to introduce a weak function, a
really strong argument is needed. Maybe you have it, but that argument
needs to be made clear.
Let's assume board 'c' implements this. Then later someone with board
'd' implements this at the SOC level. Does board 'c' get the old
implementation, or the new? Things become ambiguous in everything but
the simplest of cases.
A more elegant way would be to have a proper interface to hook into the
FIT processing. That could be done by a function pointer, ops structure,
or perhaps new UCLASS (Simon?).
Alex
Signed-off-by: Farhan Ali
Cc: Simon Glass
Cc: Alexandru Gagniuc
Cc: Marek Vasut
Cc: Michal Simek
Cc: Philippe Reynes
Cc: Samuel Holland
---
Changes for v2:
- Callback now returns a value
- Added a log message on failure
---
common/spl/spl_fit.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 75c8ff0..01aee1c 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -43,6 +43,14 @@ __weak ulong board_spl_fit_size_align(ulong size)
return size;
}
+__weak int board_spl_fit_pre_load(struct spl_load_info *load_info,
+ const void *fit,
+ ulong start_sector,
+ ulong loaded_sector_count)
+{
+ return 0;
+}
+
static int find_node_from_desc(const void *fit, int node, const char *str)
{
int child;
@@ -527,6 +535,7 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx,
unsigned long count, size;
int sectors;
void *buf;
+ int rc = 0;
/*
* For FIT with external data, figure out where the external images
@@ -552,7 +561,18 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx,
debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu,
size=0x%lx\n",
sector, sectors, buf, count, size);
- return (count == 0) ? -EIO : 0;
+ if (count) {
+ /* preprocess loaded fit header before parsing and loading
binaries */
+ rc = board_spl_fit_pre_load(info, fit_header, sector, sectors);
+ if (rc) {
+ debug("%s: fit header pre processing failed. rc=%d\n",
+ __func__, rc);
+ }
+ } else {
+ rc = -EIO;
+ }
+
+ return rc;
}
static int spl_simple_fit_parse(struct spl_fit_info *ctx)
Re: [PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing
On 3/10/21 2:49 PM, Farhan Ali wrote:
On Wed, Mar 10, 2021 at 11:38 AM Alex G. This patch describes "how" you're trying to achieve it, but "what" you
want to achieve. I'll get later into why I think the "how" is
fundamentally flawed.
The 'what' is basically this: I want to be able to parse the fit header
for correctness before
any image loading takes place. This 'correctness' will be user defined
I'd expect such code to be part of this series. Having a function that a
"user" might define sounds a lot like a vendor-specific hook with no
upstream code, hence the skepticism. This series should include a useful
implementation of board_spl_fit_pre_load().
The main use case for us is two folds:
(1) Customers are worried about our reliance on libfdt for FIT parsing
and want to prescan the FIT header to
check for any future exploits
(2) We implement a signature on the entire FIT header ( instead of
individual nodes ).
Do you believe the current FIT signing scheme is inappropriate for your
needs? Have you looked at signed configs? Is there a reason why they are
not appropriate?
There was a potential issue where a bad FIT could place itself anywhere
in memory. This was fixed in commit 03f1f78a9b ("spl: fit: Prefer a
malloc()'d buffer for loading images"). Keep in mind that, in this case,
checking the FIT header would not have guarded against the exploit.
Second issue is that spl_simple_fit_read() is intended to bring a FIT
image to memory. If you need to make decisions on the content of that
image, then spl_simple_fit_read() is the wrong place to do it. A better
place might be spl_simple_fit_parse().
spl_simple_fit_parse() parses the 'contents' of the fit using standard
APIs. We need to check
the FIT header for correctness BEFORE its contents are parsed, using a
user defined 'safe'
parsing function. The standard FIT loading flow checks for only a few
things ( hashes/configuration etc),
there can be a lot of other USER defined checks which may need to be
checked. This callback will achieve this
This patch is calling board_spl_fit_pre_load() after the FIT is read. On
a FIT with embedded data, you've also loaded all the binaries. It seems
that checking a header now is a moot point.
If you need to make sure that the FIT wasn't tampered, the signed
configs were designed exactly for that. You mentioned earlier that you
want to sign the FIT header. What is the FIT header in this case? Is it
the FDT of a FIT with external data? Is it struct fdt_header?
The reason I used a weak function was to mirror the already
upstreamed board_spl_fit_post_load(),
I see why you'd think it was a good idea. board_spl_fit_pre_load()
sneaks in a dependency on arch-specific code (CONFIG_IMX_HAB). I don't
really like the way it's implemented, and I don't know if it would work
with SPL_LOAD_FIT_FULL or bootm.
Alex
"SPL image too big" not that helpful
I've recently hit that message in a yocto build. I can't figure out the exact root cause. On the one hand, I don't know how "big" SPL is. On the other hand, I can't objdump -h the SPL elf because one wasn't created. Alex
Re: [PATCH 4/7] spl: fit: Warn if FIT contains "fpga" property in config node
On 3/18/21 1:44 AM, Simon Glass wrote:
Hi Alexandru,
On Thu, 11 Mar 2021 at 07:04, Alexandru Gagniuc wrote:
Commit 4afc4f37c70e ("doc: FIT image: Clarify format and simplify
syntax") requires that FPGA images be referenced through the
"loadables" in the config node. This means that "fpga" properties in
config nodes are deprecated.
Given that there are likely FIT images which use "fpga", let's not
break those right away. Print a warning message that such use is
deprecated, and give users a couple of releases to update their
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl_fit.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 55fca9f399..68f29c0026 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -526,6 +526,13 @@ __weak bool spl_load_simple_fit_skip_processing(void)
return false;
}
+static void warn_deprecated(const char *msg)
+{
+ printf("DEPRECATED: %s\n", msg);
+ printf("\tThis will stop working in a future u-boot release\n");
+ printf("\tSee doc/uImage.FIT/source_file_format.txt\n");
That is a lot of text to add...can it be shorter?
Sure. The idea was that we'd remove this message in a couple of releases
anyway, and we want it very loud until then. If I remove the middle
printf(), will thgat work ?
Alex
+}
+
static int spl_fit_upload_fpga(struct spl_fit_info *ctx, int node,
struct spl_image_info *fpga_image)
{
@@ -558,6 +565,8 @@ static int spl_fit_load_fpga(struct spl_fit_info *ctx,
if (node < 0)
return node;
+ warn_deprecated("'fpga' property in config node. Use 'loadables'");
+
/* Load the image and set up the fpga_image structure */
ret = spl_load_fit_image(info, sector, ctx, node, &fpga_image);
if (ret) {
--
2.26.2
Regards,
Simon
Re: "SPL image too big" not that helpful
On 3/19/21 9:27 AM, Tom Rini wrote: On Wed, Mar 17, 2021 at 06:42:55PM -0500, Alex G. wrote: I've recently hit that message in a yocto build. I can't figure out the exact root cause. On the one hand, I don't know how "big" SPL is. On the other hand, I can't objdump -h the SPL elf because one wasn't created. So that comes from arch/arm/cpu/u-boot-spl.lds and I'm not sure how to make it more helpful, aside from having something under perhaps doc/build/ that explains the error more fully and have the build-time error point to that file. Is there a way to print "SPL too big: _so-may_ bytes, only _max_ possible" ? In a local build, one could modify the source to increase SPL max size, rebuild, and objdump the SPL elf, see how much SPL overflows. That is much more tedious to do in a remote/yocto build. Alex
Re: [PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing
On 3/22/21 9:27 AM, Philippe REYNES wrote: Hi all, Le 11/03/2021 à 00:10, Alex G a écrit : [snip] I reach the same issue, my customers are also worried with the actual signature check scheme on u-boot. The fit data/node are parsed before being checked : data should be used only after being checked, not before. The code become quite complex for a signature, and the more complex the code is more risk to have/introduce a bug or security issue. [snip] The reason I used a weak function was to mirror the already upstreamed board_spl_fit_post_load(), I see why you'd think it was a good idea. board_spl_fit_pre_load() sneaks in a dependency on arch-specific code (CONFIG_IMX_HAB). I don't really like the way it's implemented, and I don't know if it would work with SPL_LOAD_FIT_FULL or bootm. As I reach the same issue, I was also thinking strongly about adding a "hook" before the fit image is launched/analyzed. In my mind this "pre load" function should be able to do some check/update to the fit image, but also modify the beginning of the fit image (to remove a header for example). Such function/feature may allow to: - check a signature for the full fit (without parsing the node) - cipher the full fit (even the node) - compress the full fit - probably that users will find a lot of others ideas . I think that this feature pre load should be implemented in spl and bootm command. I have understood the feedback about a useful implementation/usage of pre_load. I propose to sent an example soon (probably based on signature check). So "what" you want to do is verify untrusted metadata before using it. That's a very logical and reasonable thing to do. "How" you are trying to do this is by (1) adding a weak function (2) allowing each board to have a completely different implementation Those are two terrible ideas. I agree that there is a deficiency in the way FIT images are signed. Can we stick the signature between the fdt_header and before dt_struct? Alex
Re: [PATCH 5/7] spl: fit: Support loading FPGA images from list of "loadables"
On 3/29/21 2:43 AM, Simon Glass wrote:
Hi Alexandru,
On Thu, 11 Mar 2021 at 07:04, Alexandru Gagniuc wrote:
Commit 4afc4f37c70e ("doc: FIT image: Clarify format and simplify
syntax") and delegated FPGA images to be added via the list of
"loadables" in lieu of the "fpga" property. Now actually implement
this in code.
Note that the "compatible" property is ignored for the time being, as
implementing "compatible" loading is beyond the scope of this change.
However, "u-boot,fpga-legacy" is accepted without warning.
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl_fit.c | 24
1 file changed, 24 insertions(+)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 68f29c0026..ca6be6a839 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -477,6 +477,20 @@ static int spl_fit_record_loadable(const struct
spl_fit_info *ctx, int index,
return ret;
}
+static int spl_fit_image_is_fpga(const void *fit, int node)
+{
+ const char *type;
+
+ if (!IS_ENABLED(CONFIG_SPL_FPGA))
+ return 0;
+
+ type = fdt_getprop(fit, node, FIT_TYPE_PROP, NULL);
+ if (!type)
+ return 0;
+
+ return !strcmp(type, "fpga");
+}
+
static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
{
if (!CONFIG_IS_ENABLED(FIT_IMAGE_TINY) || CONFIG_IS_ENABLED(OS_BOOT))
@@ -536,11 +550,18 @@ static void warn_deprecated(const char *msg)
static int spl_fit_upload_fpga(struct spl_fit_info *ctx, int node,
struct spl_image_info *fpga_image)
{
+ const char *compatible;
int ret;
debug("FPGA bitstream at: %x, size: %x\n",
(u32)fpga_image->load_addr, fpga_image->size);
+ compatible = fdt_getprop(ctx->fit, node, "compatible", NULL);
+ if (!compatible)
+ warn_deprecated("'fpga' image without 'compatible' property");
+ else if (strcmp(compatible, "u-boot,fpga-legacy"))
+ printf("Ignoring compatible = %s property\n", compatible);
+
ret = fpga_load(0, (void *)fpga_image->load_addr, fpga_image->size,
BIT_FULL);
if (ret) {
@@ -739,6 +760,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
return ret;
}
+ if (spl_fit_image_is_fpga(ctx.fit, node))
+ spl_fit_upload_fpga(&ctx, node, &image_info);
+
Given the code-size increase I think a new Kconfig option for SPL FPGA
FIT support would be useful.
The "if !IS_ENABLED(CONFIG_SPL_FPGA))" makes spl_fit_image_is_fpga() a
no-op, with no size increase when "# CONFIG_SPL_FPGA is not set". This
is also confirmed by comparing the size of .text ( "objdump -h
spl/u-boot-spl" ). So there isn't a size increase for non-FPGA use cases.
Also, FPGA support in SPL is only possible through FIT images. So
SPL_FPGA vs SPL_FIT_FPGA distinction doesn't make sense.
Alex
if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
debug("Loadable is %s\n", genimg_get_os_name(os_type));
--
2.26.2
Regards,
Simon
Re: [PATCH 3/6] spl: LOAD_FIT_FULL: Relocate FDT for u-boot payloads
On 3/29/21 2:43 AM, Simon Glass wrote:
Hi Alexandru,
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 8f6c8dba6f..e63f05bb33 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -201,6 +201,7 @@ static int spl_load_fit_image(struct spl_image_info
*spl_image,
{
bootm_headers_t images;
const char *fit_uname_config = NULL;
+ uintptr_t fdt_hack;
const char *uname;
ulong fw_data = 0, dt_data = 0, img_data = 0;
ulong fw_len = 0, dt_len = 0, img_len = 0;
@@ -233,9 +234,18 @@ static int spl_load_fit_image(struct spl_image_info
*spl_image,
ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
- if (ret >= 0)
+ if (ret >= 0) {
spl_image->fdt_addr = (void *)dt_data;
+ if (spl_image->os == IH_OS_U_BOOT) {
+ /* HACK: U-boot expects FDT at a specific address */
+ fdt_hack = spl_image->load_addr + spl_image->size;
+ fdt_hack = (fdt_hack + 3) & ~3;
I don't think this is needed and it just confuses things. If U-Boot is
not an aligned sign, it can't boot because the DT ends up in the wrong
place. The build system makes sure this doesn't happen.
The correct alignment for an FDT is 8 bytes. For a while, this alignment
was implemented [1], and that worked fine with everything but u-boot.
Now to the build system argument. I don't think It's wise the assume the
conventions of the SPL build and of the u-boot build are the same. Even
assuming the branch building the SPL is perfect, the FIT image could
have been built from a buggy u-boot branch, or even assembled manually
outside the u-boot build.
Consequently, we can't assume the FIT image will have a spl_image->size
of the correct alignment. Thus, aligning things by hand is quite necessary.
The problem with [1] is that it broke booting u-boot with FIT. It had to
be reverted [2]. There was a lengthy email discussion about, where I
included an example of the failure [3]. Now, wish the actual problem
could be fixed, but I don't have the bandwidth. The best I can do is
document the problem.
Alex
[1]
https://source.denx.de/u-boot/u-boot/-/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76
[2]
https://source.denx.de/u-boot/u-boot/-/commit/5675ed7cb645f5ec13958726992daeeed16fd114
[3] https://lists.denx.de/pipermail/u-boot/2020-October/430058.html
Re: [PATCH 4/6] spl: LOAD_FIT_FULL: Support 'kernel' and 'firmware' properties
On 3/29/21 2:43 AM, Simon Glass wrote:
HI Alexandru,
On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc wrote:
The 'firmware' property of a config node takes precedence over the
'kernel' property. 'standalone' is deprecated. However, give users a
couple of releases where 'standalone' still works, but warns loudly.
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl.c b/common/spl/spl.c
index e63f05bb33..da4751b4ac 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -214,7 +214,24 @@ static int spl_load_fit_image(struct spl_image_info
*spl_image,
ret = fit_image_load(&images, (ulong)header,
NULL, &fit_uname_config,
IH_ARCH_DEFAULT, IH_TYPE_STANDALONE, -1,
-FIT_LOAD_REQUIRED, &fw_data, &fw_len);
+FIT_LOAD_OPTIONAL, &fw_data, &fw_len);
+ if (ret >= 0) {
+ printf("DEPRECATED: 'standalone = ' property.");
+ printf("Please use either 'firmware =' or 'kernel ='\n");
+ } else {
+ ret = fit_image_load(&images, (ulong)header, NULL,
+&fit_uname_config, IH_ARCH_DEFAULT,
+IH_TYPE_FIRMWARE, -1, FIT_LOAD_OPTIONAL,
+&fw_data, &fw_len);
+ }
+
+ if (ret < 0) {
+ ret = fit_image_load(&images, (ulong)header, NULL,
+&fit_uname_config, IH_ARCH_DEFAULT,
+IH_TYPE_KERNEL, -1, FIT_LOAD_OPTIONAL,
+&fw_data, &fw_len);
Should this only be for Falcon mode?
I don't know. Falcon mode relies on u-boot proper preparing an FDT and
saving it to storage. Then the falcon boot reads the kernel image, and
the "raw" FDT (see "spl export").
On the one hand, yes, because falcon mode loads kernels directly.
On the other hand, no, because we're not using this prepared FDT.
There's a distinction between falcon mode and kernel boot from FIT. And
it's very muddy. I'm hoping that people will stop using falcon mode with
legacy images. Then and only then, we won't have to make this distinction.
Alex
+ }
+
if (ret < 0)
return ret;
--
2.26.2
Regards,
Simon
Re: [PATCH] efi_loader: convert void* to u8* on the tcg eventlog buffer
Hi Ilias,
On 3/29/21 8:59 AM, Ilias Apalodimas wrote:
Although ptr arithmetics are allowed with extensions in gcc, they
are not allowed by the C spec. So switch the 'void *' containing our
eventlog buffer into 'u8 *'
NAK. This patch is in my opinion wrong.
In C, void * can point to anything. The same is not true of a u8*. You
can take a generic pointer, assign it to a void pointer, then assign
back to the original type. u8* has an alignment of 1. While the compiler
can now do math on u8*, it only has to worry about an alignment of 1. As
you can imagine, this can have unintended consequences when the data
behind the pointer is not actually an array of u8.
The correct way in C to do pointer arithmetic is to go via uintptr_t.
You still have to manually make sure you keep alignment and other
constraints, but this and only this is portable.
You can use either void * or the a pointer to the actual type. It's up
to you whether you want to use a uintptr_t in struct event_log_buffer,
or cast to uintptr_t when you do the actual pointer math, then cast back
to void *.
Signed-off-by: Ilias Apalodimas
---
lib/efi_loader/efi_tcg2.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 09046844c723..d5213586cb9c 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -23,8 +23,8 @@
#include
struct event_log_buffer {
- void *buffer;
- void *final_buffer;
+ u8 *buffer;
+ u8 *final_buffer;
size_t pos; /* eventlog position */
size_t final_pos; /* final events config table position */
size_t last_event_size;
@@ -990,12 +990,12 @@ static efi_status_t create_final_event(void)
* EFI_CONFIGURATION_TABLE
*/
ret = efi_allocate_pool(EFI_ACPI_MEMORY_NVS, TPM2_EVENT_LOG_SIZE,
- &event_log.final_buffer);
+ (void **)&event_log.final_buffer);
Now this sort of cast I would rather avoid. Double pointer casts rarely
do what you want them to do. For once, if someone ever changes
final_buffer to be a const pointer, you've lost that in this cast. The
compiler likely won't warn you. So you've just exposed this code to
possible silent failures.
BTW, without looking at stack overflow would that be a (const void **),
(void * const *), or (void ** const) ?
if (ret != EFI_SUCCESS)
goto out;
memset(event_log.final_buffer, 0xff, TPM2_EVENT_LOG_SIZE);
- final_event = event_log.final_buffer;
+ final_event = (struct efi_tcg2_final_events_table
*)event_log.final_buffer;
This sort of cast is also undefined. Why not make event_log.final_buffer
a struct efi_tcg2_final_events_table * in the first place?
final_event->number_of_events = 0;
final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION;
event_log.final_pos = sizeof(*final_event);
Re: [PATCH v2 6/6] test: dm: Add test for ECDSA UCLASS support
+ Tim On 3/29/21 2:43 AM, Simon Glass wrote: Hi Alexandru, On Tue, 16 Mar 2021 at 13:24, Alexandru Gagniuc wrote: This test verifies that ECDSA_UCLASS is implemented, and that ecdsa_verify() works as expected. The definition of "expected" is "does not find a device, and returns -ENODEV". The lack of a hardware-independent ECDSA implementation prevents us from having one in the sandbox, for now. Yes we do need a software impl at some point. Any update on that? I don't have any updates from Tim that you don't. I assume he's still silently hacking at it. Alex
Re: [PATCH v2 1/6] dm: crypto: Define UCLASS API for ECDSA signature verification
On 3/29/21 2:43 AM, Simon Glass wrote:
Hi Alexandru,
On Tue, 16 Mar 2021 at 13:24, Alexandru Gagniuc wrote:
Define a UCLASS API for verifying ECDSA signatures. Unlike
UCLASS_MOD_EXP, which focuses strictly on modular exponentiation,
the ECDSA class focuses on verification. This is done so that it
better aligns with mach-specific implementations, such as stm32mp.
Signed-off-by: Alexandru Gagniuc
---
include/crypto/ecdsa-uclass.h | 39 +++
include/dm/uclass-id.h| 1 +
2 files changed, 40 insertions(+)
create mode 100644 include/crypto/ecdsa-uclass.h
diff --git a/include/crypto/ecdsa-uclass.h b/include/crypto/ecdsa-uclass.h
new file mode 100644
index 00..189843820a
--- /dev/null
+++ b/include/crypto/ecdsa-uclass.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Alexandru Gagniuc
+ */
+
+#include
+
+/**
+ * struct ecdsa_public_key - ECDSA public key properties
+ *
+ * The struct has pointers to the (x, y) curve coordinates to an ECDSA public
+ * key, as well as the name of the ECDSA curve. The size of the key is inferred
+ * from the 'curve_name'
+ */
+struct ecdsa_public_key {
+ const char *curve_name; /* Name of curve, e.g. "prime256v1" */
+ const void *x; /* x coordinate of public key */
+ const void *y; /* y coordinate of public key */
+ unsigned int size_bits; /* key size in bits, derived from curve name */
+};
+
+struct ecdsa_ops {
+ /**
+* Verify signature of hash against given public key
+*
+* @dev:ECDSA Device
+* @pubkey: ECDSA public key
+* @hash: Hash of binary image
+* @hash_len: Length of hash in bytes
+* @signature: Signature in a raw (R, S) point pair
What is the format of this? I think a better API would be to have a struct here.
It's the raw format, which is the R, and S points. It's the same format
used by pycryptodomex, and inside the FIT. see ecdsa_sig_encode_raw() in
the other series ("v6: Add support for ECDSA image signing")
I don't know if it's a good idea to split up the R,S points since the
verify step(both pycryptodomex and stm32) uses this concatenated format.
Then the implementation would have to memcpy R and S to a buffer.
Alex
+* @sig_len:Length of signature in bytes
+*
+* This function verifies that the 'signature' of the given 'hash' was
+* signed by the private key corresponding to 'pubkey'.
+*/
+ int (*verify)(struct udevice *dev, const struct ecdsa_public_key
*pubkey,
+ const void *hash, size_t hash_len,
+ const void *signature, size_t sig_len);
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index d75de368c5..f335f4e5a4 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -47,6 +47,7 @@ enum uclass_id {
UCLASS_DSI_HOST,/* Display Serial Interface host */
UCLASS_DMA, /* Direct Memory Access */
UCLASS_DSA, /* Distributed (Ethernet) Switch Architecture
*/
+ UCLASS_ECDSA, /* Elliptic curve cryptographic device */
UCLASS_EFI, /* EFI managed devices */
UCLASS_ETH, /* Ethernet device */
UCLASS_ETH_PHY, /* Ethernet PHY device */
--
2.26.2
Regards,
Simon
Re: [RFC][PATCH 1/2] cmd: bootm: add a stage pre-load
Hi Phillipe,
On 3/30/21 11:26 AM, Philippe Reynes wrote:
This commit adds a stage pre-load that could
check or modify the image provided to the bootm
command.
For the moment, only a header with a signature is
supported. This header has this format:
- magic : 4 bytes
- image size : 4 bytes
- sig size : 4 bytes
- signature : n bytes
- padding : up to header size
The stage use a node /bootm/pre-load/sig to
get some information:
- header-size (mandatory) : size of the header
- algo-name (mandatory) : name of the algo used to sign
- padding-name : name of padding used to sign
- mandatory : set to yes if this sig is mandatory
- public-key : value of the public key
Before running the image, the stage pre-load check
the signature provided in the header.
This is an initial support, later we could add the
support of:
- ciphering
- uncompressing
- ...
You're on the right path of what I had in mind.
One thing that we could improve is dropping the dependency on bootm. A
FIT image could also be loaded with CONFIG_SPL_LOAD_FIT or
CONFIG_SPL_LOAD_FIT_FULL. It would be nice to have the signature
verification code shared with image-fit-sig.c
The decision to verify the "header signature" is done at kconfig time.
For distinguishing between image or config node signing, the "required"
property in the u-boot FDT is used. So it seems odd to introduce another
mechanism instead of leveraging "required".
A nice to have: how does mkimage insert this header signature into the FIT?
Alex
Signed-off-by: Philippe Reynes
---
cmd/Kconfig | 9 ++
cmd/bootm.c | 2 +-
common/bootm.c | 258
include/image.h | 1 +
4 files changed, 269 insertions(+), 1 deletion(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig
index eff238cb38..086d2b7b74 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -194,6 +194,15 @@ config CMD_BOOTM
help
Boot an application image from the memory.
+config CMD_BOOTM_PRE_LOAD
+ bool "enable pre-load on bootm"
+ depends on CMD_BOOTM
+ default n
+ help
+ Enable support of stage pre-load for the bootm command.
+This stage allow to check of modifty the image provided
+to the bootm command.
+
config BOOTM_EFI
bool "Support booting UEFI FIT images"
depends on CMD_BOOTEFI && CMD_BOOTM && FIT
diff --git a/cmd/bootm.c b/cmd/bootm.c
index 81c6b93978..7a6299d8d8 100644
--- a/cmd/bootm.c
+++ b/cmd/bootm.c
@@ -126,7 +126,7 @@ int do_bootm(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
}
return do_bootm_states(cmdtp, flag, argc, argv, BOOTM_STATE_START |
- BOOTM_STATE_FINDOS | BOOTM_STATE_FINDOTHER |
+ BOOTM_STATE_FINDOS | BOOTM_STATE_PRE_LOAD |
BOOTM_STATE_FINDOTHER |
BOOTM_STATE_LOADOS |
#ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH
BOOTM_STATE_RAMDISK |
diff --git a/common/bootm.c b/common/bootm.c
index defaed8426..37b1340023 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -42,6 +42,12 @@
#define IH_INITRD_ARCH IH_ARCH_DEFAULT
+#define BOOTM_PRE_LOAD_SIG_MAGIC 0x55425348
+#define BOOTM_PRE_LOAD_SIG_OFFSET_MAGIC0
+#define BOOTM_PRE_LOAD_SIG_OFFSET_IMG_LEN 4
+#define BOOTM_PRE_LOAD_SIG_OFFSET_SIG_LEN 8
+#define BOOTM_PRE_LOAD_SIG_OFFSET_SIG 12
+
#ifndef USE_HOSTCC
DECLARE_GLOBAL_DATA_PTR;
@@ -87,6 +93,255 @@ static int bootm_start(struct cmd_tbl *cmdtp, int flag, int
argc,
return 0;
}
+static ulong bootm_data_addr(int argc, char *const argv[])
+{
+ ulong addr;
+
+ if (argc > 0)
+ addr = simple_strtoul(argv[0], NULL, 16);
+ else
+ addr = image_load_addr;
+
+ return addr;
+}
+
+struct bootm_sig_header {
+ u32 magic;
+ u32 size;
+ u8 *sig;
+};
+
+struct bootm_sig_info {
+ ulong header_size;
+ char *algo_name;
+ char *padding_name;
+ u8 *key;
+ int key_len;
+ int mandatory;
+};
+
+/*
+ * This function gather information about the signature check
+ * that could be done in the pre-load stage of bootm.
+ *
+ * return:
+ * -1 => an error has occurred
+ * 0 => OK
+ * 1 => no setup
+ */
+static int bootm_pre_load_sig_setup(struct bootm_sig_info *info)
+{
+ const void *algo_name, *padding_name, *key, *mandatory;
+ const u32 *header_size;
+ int key_len;
+ int node, ret = 0;
+
+ if (!info) {
+ printf("ERROR: info is NULL for bootm pre-load sig check\n");
+ ret = -1;
+ goto out;
+ }
+
+ memset(info, 0, sizeof(*info));
+
+ node = fdt_path_offset(gd->fdt_blob, "/bootm/pre-load/sig");
+ if (node < 0) {
+ printf("INFO: no info for bootm pre-load sig check\n");
+ ret = 1;
+ goto out;
+ }
+
+ header_size = fdt_getprop(gd->fdt_blob, node, "header-size", NULL);
+ if (!header_si
Re: [PATCH 2/6] spl: LOAD_FIT_FULL: Do not hard-code os to IH_OS_U_BOOT
On 3/29/21 2:43 AM, Simon Glass wrote: On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc wrote: The information on the OS should be contained in the FIT, as the self-explanatory "os" property of a node under /images. Hard-coding this to U_BOOT might send us down the wrong path later in the boot process. Signed-off-by: Alexandru Gagniuc --- common/spl/spl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/common/spl/spl.c b/common/spl/spl.c index 986cfbc6fd..8f6c8dba6f 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -220,8 +220,9 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, spl_image->size = fw_len; spl_image->entry_point = fw_data; spl_image->load_addr = fw_data; - spl_image->os = IH_OS_U_BOOT; +8 bytes. Down in the noise Alex - spl_image->name = "U-Boot"; + if (!fit_image_get_os(header, ret, &spl_image->os)) + spl_image->os = IH_OS_INVALID; + spl_image->name = genimg_get_os_name(spl_image->os); debug(SPL_TPL_PROMPT "payload image: %32s load addr: 0x%lx size: %d\n", spl_image->name, spl_image->load_addr, spl_image->size); -- 2.26.2 Regards, Simon
Re: [PATCH v2] efi_loader: Change ptr arithmetics tcg eventlog buffer
Hi Ilias,
On 3/29/21 4:42 PM, Ilias Apalodimas wrote:
Although ptr arithmetics are allowed with extensions in gcc, they
are not allowed by the C spec. So switch to (void *)(uintptr_t) instead
Signed-off-by: Ilias Apalodimas
Reviewed-by: Alexandru Gagniuc
---
changes since v1:
Switch over to uintptr as Alex suggested
lib/efi_loader/efi_tcg2.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 09046844c723..ed86a220fbd6 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -176,13 +176,14 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index,
u32 event_type,
struct tpml_digest_values
*digest_list,
u32 size, u8 event[])
{
- void *log = event_log.buffer + event_log.pos;
+ void *log = (void *)((uintptr_t)event_log.buffer + event_log.pos);
I see two patterns here. This is the first one. If you want to dress up
the casts, you could do:
static void *ptr_after(void *loc, size_t offset);
...
void *log = ptr_after(event_log.buffer, event_log.pos);
size_t pos;
int i;
u32 event_size;
if (event_log.get_event_called)
- log = event_log.final_buffer + event_log.final_pos;
+ log = (void *)((uintptr_t)event_log.final_buffer +
+ event_log.final_pos);
/*
* size refers to the length of event[] only, we need to check against
@@ -197,24 +198,24 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index,
u32 event_type,
put_unaligned_le32(pcr_index, log);
pos = offsetof(struct tcg_pcr_event2, event_type);
- put_unaligned_le32(event_type, log + pos);
+ put_unaligned_le32(event_type, (void *)((uintptr_t)log + pos));
This is the second pattern, and I think you could improve this by doing:
static
void put_unaligned_le32_at(uint32_t val, void *base, size_t offset);
...
put_unaligned_le32_at(event_type, log, pos);
pos = offsetof(struct tcg_pcr_event2, digests); /* count */
- put_unaligned_le32(digest_list->count, log + pos);
+ put_unaligned_le32(digest_list->count, (void *)((uintptr_t)log + pos));
pos += offsetof(struct tpml_digest_values, digests);
for (i = 0; i < digest_list->count; i++) {
u16 hash_alg = digest_list->digests[i].hash_alg;
u8 *digest = (u8 *)&digest_list->digests[i].digest;
- put_unaligned_le16(hash_alg, log + pos);
+ put_unaligned_le16(hash_alg, (void *)((uintptr_t)log + pos));
pos += offsetof(struct tpmt_ha, digest);
- memcpy(log + pos, digest, alg_to_len(hash_alg));
+ memcpy((void *)((uintptr_t)log + pos), digest,
alg_to_len(hash_alg));
pos += alg_to_len(hash_alg);
}
- put_unaligned_le32(size, log + pos);
+ put_unaligned_le32(size, (void *)((uintptr_t)log + pos));
pos += sizeof(u32); /* tcg_pcr_event2 event_size*/
- memcpy(log + pos, event, size);
+ memcpy((void *)((uintptr_t)log + pos), event, size);
pos += size;
/* make sure the calculated buffer is what we checked against */
@@ -1046,7 +1047,7 @@ static efi_status_t efi_init_event_log(void)
put_unaligned_le32(0, &event_header->pcr_index);
put_unaligned_le32(EV_NO_ACTION, &event_header->event_type);
memset(&event_header->digest, 0, sizeof(event_header->digest));
- ret = create_specid_event(dev, event_log.buffer + sizeof(*event_header),
+ ret = create_specid_event(dev, (void *)((uintptr_t)event_log.buffer +
sizeof(*event_header)),
&spec_event_size);
if (ret != EFI_SUCCESS)
goto out;
Re: [EXTERNAL] Re: [PATCH v2 6/6] test: dm: Add test for ECDSA UCLASS support
On 4/7/21 12:29 PM, Tim Romanski wrote:
Question for Alex, I see your repo has a few branches related to ECDSA
(patch-ecdsa-v[1-5], patch-mkimage-keyfile-v{1,2}). You sent me a link
to 'patch-ecdsa-v1' in a previous email, is that the one that's being
upstreamed? Should I be working off a different branch or is that one ok?
I'm up to v6 on the patch submission. The differences are not that big,
but I recommend sticking to the latest.
Alex
Re: [EXTERNAL] Re: [PATCH v2 6/6] test: dm: Add test for ECDSA UCLASS support
On 4/8/21 11:56 AM, Tim Romanski wrote:
Ok, will do. I'm writing the verification code, I noticed you're passing
the public key into the fdt using fdt_add_bignum, which converts the x
and y values into big endian integer arrays. Do you have a method to
read these values from the fdt and convert them back into bignums, or is
that TODO? I can get that done if it's not yet implemented.
Because u-boot proper doesn't use openssl, there hasn't been a need to
convert data into types such as BIGNUM* at runtime. You could check
BN_bin2bn() for inspiration.
Alex
All the best,
Tim
On 2021-04-07 4:03 p.m., Alex G. wrote:
On 4/7/21 12:29 PM, Tim Romanski wrote:
Question for Alex, I see your repo has a few branches related to
ECDSA (patch-ecdsa-v[1-5], patch-mkimage-keyfile-v{1,2}). You sent me
a link to 'patch-ecdsa-v1' in a previous email, is that the one
that's being upstreamed? Should I be working off a different branch
or is that one ok?
I'm up to v6 on the patch submission. The differences are not that
big, but I recommend sticking to the latest.
Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/6/21 9:24 AM, 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_HOST_xxx
is distinct from CONFIG_xxx.
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
This makes me wonder whether we should just always enable host features.
Right now, each defconfig can have a different mkimage config. So we
should really have mkimage-imx8, mkimage-stm32mp, etc, which support
different feature sets. This doesn't make much sense.
The alternative is to get rid of all these configs and always enable
mkimage features. The disadvantage is that we'd require openssl for
building target code.
A second alternative is to have a mkimage-nossl that gets built and used
when openssl isn't available. It's really just openssl that causes such
a schism.
Alex
---
(no changes since v1)
common/image-fit-sig.c | 3 ++-
common/image-fit.c | 4 ++--
tools/Kconfig | 25 +
tools/Makefile | 18 +-
4 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 55ddf1879ed..12a6745c642 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) {
*err_msgp = "Total size too large";
return 1;
}
-
+#endif
if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
*err_msgp = "Can't get hash algo property";
return -1;
diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe39..a16e2dd54a5 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -165,7 +165,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
@@ -573,7 +573,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 b2f5012240c..f00ab661135 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -9,4 +9,29 @@ 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 HOST_FIT
+ def_bool y
+ help
+ Enable FIT support in the host build.
Don't we always want to enable this for mkimage?
+
+config HOST_FIT_FULL_CHECK
+ def_bool y
+ help
+ Do a full check of the FIT before using it in the host build
How would this be used? FIT vs FIT_FULL is mostly an SPL distinction. I
don't think we should have it in host tools.
+
+config HOST_FIT_PRINT
+ def_bool y
+ help
+ Print the content of the FIT verbosely in the host build
This option also doesn't make sense.This seems to do what 'mkimage -l'
already supports.
+
+config HOST_FIT_SIGNATURE
+ def_bool y
+ help
+ Enable signature verification of FIT uImages in the host build
s/verification/signing and verification/
+
+config HOST_FIT_SIGNATURE_MAX_SIZE
+ hex
+ depends on HOST_FIT_SIGNATURE
+ default 0x1000
+
The only use of FIT_SIGNATURE_MAX_SIZE is under "#ifndef USE_HOSTCC". So
this wouldn't make any sense for the host.
endmenu
diff --git a/tools/Makefile b/tools/Makefile
index 2b4bc547abd..d143198f7c9 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -53,12 +53,12 @@ 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_HOST_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_HOST_FIT) := fit_common.o fit_image.o image-host.o
common/image-fit.o
+FIT_SIG_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := common/image-sig.o
common/image-fit-sig.o
FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
# The following files are sync
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/11/21 5:34 PM, Tom Rini wrote: On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. wrote: On 5/6/21 9:24 AM, 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_HOST_xxx is distinct from CONFIG_xxx. Signed-off-by: Simon Glass Reviewed-by: Alexandru Gagniuc This makes me wonder whether we should just always enable host features. Right now, each defconfig can have a different mkimage config. So we should really have mkimage-imx8, mkimage-stm32mp, etc, which support different feature sets. This doesn't make much sense. The alternative is to get rid of all these configs and always enable mkimage features. The disadvantage is that we'd require openssl for building target code. A second alternative is to have a mkimage-nossl that gets built and used when openssl isn't available. It's really just openssl that causes such a schism. It would probably be best to have a single mkimage for everyone, with everything on. But before then we really need to move from openssl to gnutls or some other library that's compatible as it's been raised before that linking with openssl like we do is a license violation I believe. How about the former alternative for now? i.e. compile mkimage with or without openssl, and have that be the only host side switch. Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/12/21 9:51 AM, Simon Glass wrote: Hi Alex, On Tue, 11 May 2021 at 13:57, Alex G. wrote: On 5/6/21 9:24 AM, Simon Glass wrote: [snip] + +config HOST_FIT_PRINT + def_bool y + help + Print the content of the FIT verbosely in the host build This option also doesn't make sense.This seems to do what 'mkimage -l' already supports. Are you sure you have looked at the goal of the CONFIG_IS_ENABLED() changes? This is here purely to avoid #ifdefs in the share code. On the one hand, we have the cosmetic inconvenience caused by #ifdefs. On the other hand we have the config system. To most users, the config system is likely more visible, while it's mostly developers who will ever see the ifdefs. Therefore, in order to get the developer convenience of less ifdefs, we have to sacrifice user convenience by cloberring the Kconfig options. I think this is back-to-front. Can we reduce the host config count to just SLL/NOSSL? Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/12/21 10:54 AM, Simon Glass wrote: Hi Alex, On Wed, 12 May 2021 at 09:48, Alex G. wrote: On 5/12/21 9:51 AM, Simon Glass wrote: Hi Alex, On Tue, 11 May 2021 at 13:57, Alex G. wrote: On 5/6/21 9:24 AM, Simon Glass wrote: [snip] + +config HOST_FIT_PRINT + def_bool y + help + Print the content of the FIT verbosely in the host build This option also doesn't make sense.This seems to do what 'mkimage -l' already supports. Are you sure you have looked at the goal of the CONFIG_IS_ENABLED() changes? This is here purely to avoid #ifdefs in the share code. On the one hand, we have the cosmetic inconvenience caused by #ifdefs. On the other hand we have the config system. To most users, the config system is likely more visible, while it's mostly developers who will ever see the ifdefs. Therefore, in order to get the developer convenience of less ifdefs, we have to sacrifice user convenience by cloberring the Kconfig options. I think this is back-to-front. These Kconfig options are not visible to users. They cannot be updated in defconfig, nor in 'make menuconfig', etc. They are purely there for the build system. Can we reduce the host config count to just SLL/NOSSL? The point here is that the code has a special case for host builds, and this is a means to remove that special case and make the code easier to maintain and follow. I understand where you're coming from. Without these changes, the code knows what it should and should not do, correct? My argument is that if the code has the logic to do the correct thing, that logic should not be split with the config system. I agree with the goal of reducing clutter in the source code. I disagree with this specific course of fixing it. Instead, I propose a single kconfig for host tools for SSL on/off. The disadvantage of my proposal is that we have to refactor the common code in a way consistent with the goals, instead of just changing some #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my head, I don't have a detailed plan on how to achieve this. Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/12/21 10:52 AM, Simon Glass wrote: Hi, On Tue, 11 May 2021 at 19:10, Tom Rini wrote: On Tue, May 11, 2021 at 07:50:38PM -0500, Alex G. wrote: On 5/11/21 5:34 PM, Tom Rini wrote: On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. wrote: On 5/6/21 9:24 AM, 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_HOST_xxx is distinct from CONFIG_xxx. Signed-off-by: Simon Glass Reviewed-by: Alexandru Gagniuc This makes me wonder whether we should just always enable host features. Right now, each defconfig can have a different mkimage config. So we should really have mkimage-imx8, mkimage-stm32mp, etc, which support different feature sets. This doesn't make much sense. The alternative is to get rid of all these configs and always enable mkimage features. The disadvantage is that we'd require openssl for building target code. A second alternative is to have a mkimage-nossl that gets built and used when openssl isn't available. It's really just openssl that causes such a schism. It would probably be best to have a single mkimage for everyone, with everything on. But before then we really need to move from openssl to gnutls or some other library that's compatible as it's been raised before that linking with openssl like we do is a license violation I believe. How about the former alternative for now? i.e. compile mkimage with or without openssl, and have that be the only host side switch. That would be a step in the right direction, yeah. 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. Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/12/21 12:30 PM, Simon Glass wrote: Hi Alex, On Wed, 12 May 2021 at 10:18, Alex G. wrote: On 5/12/21 10:54 AM, Simon Glass wrote: Hi Alex, On Wed, 12 May 2021 at 09:48, Alex G. wrote: On 5/12/21 9:51 AM, Simon Glass wrote: Hi Alex, On Tue, 11 May 2021 at 13:57, Alex G. wrote: On 5/6/21 9:24 AM, Simon Glass wrote: [snip] + +config HOST_FIT_PRINT + def_bool y + help + Print the content of the FIT verbosely in the host build This option also doesn't make sense.This seems to do what 'mkimage -l' already supports. Are you sure you have looked at the goal of the CONFIG_IS_ENABLED() changes? This is here purely to avoid #ifdefs in the share code. On the one hand, we have the cosmetic inconvenience caused by #ifdefs. On the other hand we have the config system. To most users, the config system is likely more visible, while it's mostly developers who will ever see the ifdefs. Therefore, in order to get the developer convenience of less ifdefs, we have to sacrifice user convenience by cloberring the Kconfig options. I think this is back-to-front. These Kconfig options are not visible to users. They cannot be updated in defconfig, nor in 'make menuconfig', etc. They are purely there for the build system. Can we reduce the host config count to just SLL/NOSSL? The point here is that the code has a special case for host builds, and this is a means to remove that special case and make the code easier to maintain and follow. I understand where you're coming from. Without these changes, the code knows what it should and should not do, correct? My argument is that if the code has the logic to do the correct thing, that logic should not be split with the config system. I agree with the goal of reducing clutter in the source code. I disagree with this specific course of fixing it. Instead, I propose a single kconfig for host tools for SSL on/off. The disadvantage of my proposal is that we have to refactor the common code in a way consistent with the goals, instead of just changing some #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my head, I don't have a detailed plan on how to achieve this. You are mostly describing the status quo, so far as I understand it. The problem is with the code that is built for both boards and tools. For boards, we want this code to be compiled conditionally, depending on what options are enabled. For tools, we want the code to be compiled unconditionally. I can think of only three ways to do this: - status quo (add #ifdefs USE_HOSTCC wherever we need to) - my series (make use of hidden Kconfig options to avoid that) - put every single feature and associated lines of code in separate files and compile them conditionally for boards, but always for tools I believe the last option is actually impossible, or at least impractical. It would cause an explosion of source files to deal with all the various combinations, and would be quite painful to maintain also. I don't think the status quo is such a terrible solution, so I am looking at the aspects that can benefit from improvement. Hence why it may appear I am talking about the status quo. Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for those cases where you need to know if IS_HOST_BUILD(), there's a macro for that. You have the oddball case that uses a CONFIG_ value that's only defined on the target, and those you would have to split according to your option #3. I don't think the above is as dire an explosion in source files as it seems. Another point of contention is checksum_algos and crypto_algos arrays image-sig.c. On the target side, these should be in .u-boot-list. Status quo is the definition of rsa_verify is hidden behind #if macros, which just pushes the complexity out into the rsa.h headers. The two ideas here are CONFIG_IS_ENABLED() returns true for host code, and image-sig.c is split bwtween host and target versions, the target version making use of .uboot-list. Using these as the starting points, I think we can get to a much better solution Alex
Re: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build
On 5/13/21 6:56 PM, Simon Glass wrote:
Hi Alex,
On Thu, 13 May 2021 at 10:21, Alex G. wrote:
On 5/12/21 12:30 PM, Simon Glass wrote:
Hi Alex,
On Wed, 12 May 2021 at 10:18, Alex G. wrote:
On 5/12/21 10:54 AM, Simon Glass wrote:
Hi Alex,
On Wed, 12 May 2021 at 09:48, Alex G. wrote:
On 5/12/21 9:51 AM, Simon Glass wrote:
Hi Alex,
On Tue, 11 May 2021 at 13:57, Alex G. wrote:
On 5/6/21 9:24 AM, Simon Glass wrote:
[snip]
+
+config HOST_FIT_PRINT
+ def_bool y
+ help
+ Print the content of the FIT verbosely in the host build
This option also doesn't make sense.This seems to do what 'mkimage -l'
already supports.
Are you sure you have looked at the goal of the CONFIG_IS_ENABLED()
changes? This is here purely to avoid #ifdefs in the share code.
On the one hand, we have the cosmetic inconvenience caused by #ifdefs.
On the other hand we have the config system. To most users, the config
system is likely more visible, while it's mostly developers who will
ever see the ifdefs.
Therefore, in order to get the developer convenience of less ifdefs, we
have to sacrifice user convenience by cloberring the Kconfig options. I
think this is back-to-front.
These Kconfig options are not visible to users. They cannot be updated
in defconfig, nor in 'make menuconfig', etc. They are purely there for
the build system.
Can we reduce the host config count to just SLL/NOSSL?
The point here is that the code has a special case for host builds,
and this is a means to remove that special case and make the code
easier to maintain and follow.
I understand where you're coming from. Without these changes, the code
knows what it should and should not do, correct? My argument is that if
the code has the logic to do the correct thing, that logic should not be
split with the config system.
I agree with the goal of reducing clutter in the source code. I disagree
with this specific course of fixing it. Instead, I propose a single
kconfig for host tools for SSL on/off.
The disadvantage of my proposal is that we have to refactor the common
code in a way consistent with the goals, instead of just changing some
#ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my
head, I don't have a detailed plan on how to achieve this.
You are mostly describing the status quo, so far as I understand it.
The problem is with the code that is built for both boards and tools.
For boards, we want this code to be compiled conditionally, depending
on what options are enabled. For tools, we want the code to be
compiled unconditionally.
I can think of only three ways to do this:
- status quo (add #ifdefs USE_HOSTCC wherever we need to)
- my series (make use of hidden Kconfig options to avoid that)
- put every single feature and associated lines of code in separate
files and compile them conditionally for boards, but always for tools
I believe the last option is actually impossible, or at least
impractical. It would cause an explosion of source files to deal with
all the various combinations, and would be quite painful to maintain
also.
I don't think the status quo is such a terrible solution, so I am
looking at the aspects that can benefit from improvement. Hence why it
may appear I am talking about the status quo.
Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for
those cases where you need to know if IS_HOST_BUILD(), there's a macro
for that. You have the oddball case that uses a CONFIG_ value that's
only defined on the target, and those you would have to split according
to your option #3.
I don't think the above is as dire an explosion in source files as it seems.
Another point of contention is checksum_algos and crypto_algos arrays
image-sig.c. On the target side, these should be in .u-boot-list. Status
quo is the definition of rsa_verify is hidden behind #if macros, which
just pushes the complexity out into the rsa.h headers.
The two ideas here are CONFIG_IS_ENABLED() returns true for host code,
and image-sig.c is split bwtween host and target versions, the target
version making use of .uboot-list. Using these as the starting points, I
think we can get to a much better solution
I did consider simply defining CONFIG_IS_ENABLED() to true for the
host, but rejected that because I worried it would break down at some
point. Examples I can think of at the moment:
- conflicting or alternative options (OF_LIVE, OF_HOST_FILE, OF_SEPARATE)
- code that is not actually wanted on the host (WATCHDOG,
NEEDS_MANUAL_RELOC, FPGA)
- code that we want on the host but not the board (so we end up with a
dummy CONFIG for the boards?)
- all the SPL / TPL / VPL options which would always end up being
true, when in fact we probably want none of them
I think you should more clearly explain your objection to the hidden
Kconfig options, since your original reason ("clobbering the Kconfig
space") doesn't seem
Re: [PATCH v2 18/50] image: Shorten FIT_ENABLE_SHAxxx_SUPPORT
On 5/6/21 9:24 AM, Simon Glass wrote:
The ENABLE part of this name is redundant, since all boolean Kconfig
options serve to enable something. The SUPPORT part is also redundant
since Kconfigs can be assumed to enable support for something. Together
they just serve to make these options overly long and inconsistent
with other options.
Rename FIT_ENABLE_SHAxxx_SUPPORT to FIT_SHAxxx
Signed-off-by: Simon Glass
Reviewed-by: Alexandru Gagniuc
---
(no changes since v1)
common/Kconfig.boot | 6 +++---
configs/mt8516_pumpkin_defconfig | 2 +-
include/image.h | 12 ++--
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 5a18d62d780..af3325a7ce2 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -35,7 +35,7 @@ config FIT_EXTERNAL_OFFSET
could be put in the hole between data payload and fit image
header, such as CSF data on i.MX platform.
-config FIT_ENABLE_SHA256_SUPPORT
+config FIT_SHA256
bool "Support SHA256 checksum of FIT image contents"
default y
select SHA256
@@ -44,7 +44,7 @@ config FIT_ENABLE_SHA256_SUPPORT
SHA256 checksum is a 256-bit (32-byte) hash value used to check that
the image contents have not been corrupted.
-config FIT_ENABLE_SHA384_SUPPORT
+config FIT_SHA384
bool "Support SHA384 checksum of FIT image contents"
default n
select SHA384
@@ -54,7 +54,7 @@ config FIT_ENABLE_SHA384_SUPPORT
the image contents have not been corrupted. Use this for the highest
security.
-config FIT_ENABLE_SHA512_SUPPORT
+config FIT_SHA512
bool "Support SHA512 checksum of FIT image contents"
default n
select SHA512
diff --git a/configs/mt8516_pumpkin_defconfig b/configs/mt8516_pumpkin_defconfig
index 5270ec28cbd..d330c03db3c 100644
--- a/configs/mt8516_pumpkin_defconfig
+++ b/configs/mt8516_pumpkin_defconfig
@@ -13,7 +13,7 @@ CONFIG_DEBUG_UART_CLOCK=2600
CONFIG_DEFAULT_DEVICE_TREE="mt8516-pumpkin"
CONFIG_DEBUG_UART=y
CONFIG_FIT=y
-# CONFIG_FIT_ENABLE_SHA256_SUPPORT is not set
+# CONFIG_FIT_SHA256 is not set
# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
CONFIG_DEFAULT_FDT_FILE="mt8516-pumpkin"
# CONFIG_DISPLAY_BOARDINFO is not set
diff --git a/include/image.h b/include/image.h
index 459685d4d43..9319a779b93 100644
--- a/include/image.h
+++ b/include/image.h
@@ -31,9 +31,9 @@ struct fdt_region;
#define IMAGE_ENABLE_OF_LIBFDT1
#define CONFIG_FIT_VERBOSE1 /* enable fit_format_{error,warning}() */
#define CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT 1
-#define CONFIG_FIT_ENABLE_SHA256_SUPPORT
-#define CONFIG_FIT_ENABLE_SHA384_SUPPORT
-#define CONFIG_FIT_ENABLE_SHA512_SUPPORT
+#define CONFIG_FIT_SHA256
+#define CONFIG_FIT_SHA384
+#define CONFIG_FIT_SHA512
#define CONFIG_SHA1
#define CONFIG_SHA256
#define CONFIG_SHA384
@@ -89,21 +89,21 @@ struct fdt_region;
#define IMAGE_ENABLE_SHA1 0
#endif
-#if defined(CONFIG_FIT_ENABLE_SHA256_SUPPORT) || \
+#if defined(CONFIG_FIT_SHA256) || \
defined(CONFIG_SPL_SHA256_SUPPORT)
#define IMAGE_ENABLE_SHA256 1
#else
#define IMAGE_ENABLE_SHA256 0
#endif
-#if defined(CONFIG_FIT_ENABLE_SHA384_SUPPORT) || \
+#if defined(CONFIG_FIT_SHA384) || \
defined(CONFIG_SPL_SHA384_SUPPORT)
#define IMAGE_ENABLE_SHA384 1
#else
#define IMAGE_ENABLE_SHA384 0
#endif
-#if defined(CONFIG_FIT_ENABLE_SHA512_SUPPORT) || \
+#if defined(CONFIG_FIT_SHA512) || \
defined(CONFIG_SPL_SHA512_SUPPORT)
#define IMAGE_ENABLE_SHA512 1
#else
Re: [PATCH v2 19/50] image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx
On 5/6/21 9:24 AM, Simon Glass wrote: These option are named inconsistently with other SPL options, thus making them incompatible with the CONFIG_IS_ENABLED() macro. Rename them. Signed-off-by: Simon Glass Reviewed-by: Alexandru Gagniuc --- (no changes since v1) common/spl/Kconfig | 8 include/image.h| 8 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/common/spl/Kconfig b/common/spl/Kconfig FIT_SHAxxx is in common/Kconfig.boot, so it seems unnatural to have the SPL equivalents in a different file. I have a patch out to address this and move the options to the correct Kconfig, although your approach focuses FIT_SHA strictly. index df5468f1ac2..d94b9892175 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -429,7 +429,7 @@ config SPL_MD5_SUPPORT applications where images may be changed maliciously, you should consider SHA256 or SHA384. -config SPL_SHA1_SUPPORT +config SPL_FIT_SHA1 bool "Support SHA1" depends on SPL_FIT select SHA1 @@ -441,7 +441,7 @@ config SPL_SHA1_SUPPORT due to the expanding computing power available to brute-force attacks. For more security, consider SHA256 or SHA384. -config SPL_SHA256_SUPPORT +config SPL_FIT_SHA256 bool "Support SHA256" depends on SPL_FIT select SHA256 @@ -450,7 +450,7 @@ config SPL_SHA256_SUPPORT checksum is a 256-bit (32-byte) hash value used to check that the image contents have not been corrupted. -config SPL_SHA384_SUPPORT +config SPL_FIT_SHA384 bool "Support SHA384" depends on SPL_FIT select SHA384 @@ -461,7 +461,7 @@ config SPL_SHA384_SUPPORT image contents have not been corrupted. Use this for the highest security. -config SPL_SHA512_SUPPORT +config SPL_FIT_SHA512 bool "Support SHA512" depends on SPL_FIT select SHA512 diff --git a/include/image.h b/include/image.h index 9319a779b93..3284f36c97a 100644 --- a/include/image.h +++ b/include/image.h @@ -68,7 +68,7 @@ struct fdt_region; # ifdef CONFIG_SPL_MD5_SUPPORT # define IMAGE_ENABLE_MD5 1 # endif -# ifdef CONFIG_SPL_SHA1_SUPPORT +# ifdef CONFIG_SPL_FIT_SHA1 # define IMAGE_ENABLE_SHA1 1 # endif # else @@ -90,21 +90,21 @@ struct fdt_region; #endif #if defined(CONFIG_FIT_SHA256) || \ - defined(CONFIG_SPL_SHA256_SUPPORT) + defined(CONFIG_SPL_FIT_SHA256) #define IMAGE_ENABLE_SHA256 1 #else #define IMAGE_ENABLE_SHA256 0 #endif #if defined(CONFIG_FIT_SHA384) || \ - defined(CONFIG_SPL_SHA384_SUPPORT) + defined(CONFIG_SPL_FIT_SHA384) #define IMAGE_ENABLE_SHA384 1 #else #define IMAGE_ENABLE_SHA384 0 #endif #if defined(CONFIG_FIT_SHA512) || \ - defined(CONFIG_SPL_SHA512_SUPPORT) + defined(CONFIG_SPL_FIT_SHA512) #define IMAGE_ENABLE_SHA512 1 #else #define IMAGE_ENABLE_SHA512 0
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 CONFIG_IS_EN
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(), &verify_all)) {
err_msg = "Unable to verify required signature";
@@ -1323,7 +1323,7 @@ int fit_image_verify_with_data(const void *fit, int
image_noffset,
&err_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 = &checksum_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
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 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 i
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 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 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 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, &desc);
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 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, &algo_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, &os_arch);
images->os.arch = os_arch;
+ }
#endif
bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
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 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 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 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 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 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/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/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/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 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 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 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 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: 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] 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 follo
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/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: [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 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/6] Add support for ECDSA image signing (with test)
On 1/28/21 10:40 AM, Patrick DELAUNAY wrote:
Hi Alexandru,
Hi Patrick
I found in doc/uImage.FIT/signature.txt the description
- key-name-hint: Name of key to use for signing. The keys will
normally be in a single directory (parameter -k to mkimage).
[snip]
You are correct that the ECDSA path does not use the "key-name-hint".
And this deviates from what RSA does. This is intentional.
[snip]> so today the RSA features seens more compete based on openssl (with
ENGINE and pkcs11 support for exmaple)
and keydir / keyname seens clear enought...
The the common case with image signing is that one wants to sign an
image with a key. keyname comes from the console, and keydir comes from
the FIT image, which contradicts this simplicity.
Another issue is incorporating this into a bigger build system like
yocto. Now mkimage would impose a specific directory structure for
signing keys. This would not be ideal.
PS: I think the engine part could be shared between RSA and EDCSA part.
I don't see the benefit of using the engine, and it seems highly
libcrypto specific. It would be a lot more code, with no useful
functionality -- we can ecdsa sign with the simpler code.
[snip]
This new option -K with full path will be permanent added to mkimage
or it is a temporarily solution (for initial ECDSA implementation).
If it is permanent it should be also supported in RSA mode ?
=> for example: -K allow to override the "key-name-hint" value
Yes and no. It is temporary in that we'd like to update the RSA path to
be consistent with the ECDSA path. It's permanent in that we want to
have the -'k' option to specify the key filename instead of the key dir.
At least that's my understanding given the previous discussion with Simon.
[snip]
Sorry to open debate.
I propose to change the test with previous proposal.
[snip]
with test/py/tests/vboot/sign-images-sha256.its
fdt@1 {
signature@1 {
algo = "sha1,rsa2048";
- key-name-hint = "dev";
+ key-name-hint = "ecdsa-test-key.pem";
This would go against us wanting to decouple the key filename from the
key name. Consider haing several keys:
dev-key-frobnozzle.pem
prov-key-frobnozzle-r1.pem
prov-key-frobnozzle-r2.pem
prov-key-frobnozzle-r3-after-hack-mitigation.pem
One might not want to expose those key names in the .its. The issue is,
when the fit-image is created, the key filename must be known. But when
the signing happens on a separate machine, the filename really isn't known.
So we should really use the "key-name-hint" as a hint rather than a
filename or part of a filename.
Alex
Re: [PATCH v2 26/40] test: Use a local variable for test state
Hi Simon,
On 1/30/21 9:32 PM, Simon Glass wrote:
[snip]
+static struct unit_test_state *cur_test_state;
+
+struct unit_test_state *test_get_state(void)
+{
+ return cur_test_state;
+}
+
+void test_set_state(struct unit_test_state *uts)
+{
+ cur_test_state = uts;
+}
+
/**
* dm_test_pre_run() - Get ready to run a driver model test
*
@@ -180,6 +192,9 @@ static int ut_run_test(struct unit_test_state *uts, struct
unit_test *test,
note = " (flat tree)";
printf("Test: %s: %s%s\n", test_name, fname, note);
+ /* Allow access to test state from drivers */
+ cur_test_state = uts;
+
Is there a reason for setting 'cur_test_state' directly instead of
through test_set_state() ?
ret = test_pre_run(uts, test);
if (ret == -EAGAIN)
return -EAGAIN;
@@ -192,6 +207,8 @@ static int ut_run_test(struct unit_test_state *uts, struct
unit_test *test,
if (ret)
return ret;
+ cur_test_state = NULL;
+
ditto
return 0;
}
Re: [PATCH 1/5] spl: mmc: Support OP-TEE payloads in Falcon mode
This series was re-sent in error. Please ignore.
On 2/4/21 1:55 PM, Alexandru Gagniuc wrote:
In general, Falcon mode means we're booting a linux kernel directly.
With FIT images, however, an OP-TEE secure kernel can be booted before
linux. Thus, if the next stage is an IH_OS_TEE, this isn't necessarily
a problem.
Of course, a general solution would involve mmc_load_image_raw_os()
only loading the binary, and leaving the decision of suitability to
someone else. However, a rework of the boot flow is beyond the scope
of this patch. Accept IH_OS_TEE as a valid OS value.
Signed-off-by: Alexandru Gagniuc
---
common/spl/spl_mmc.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index add2785b4e..bab558d055 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -230,8 +230,10 @@ static int mmc_load_image_raw_os(struct spl_image_info
*spl_image,
if (ret)
return ret;
- if (spl_image->os != IH_OS_LINUX) {
- puts("Expected Linux image is not found. Trying to start
U-boot\n");
+ if (spl_image->os != IH_OS_LINUX && spl_image->os != IH_OS_TEE) {
+ puts("Expected OS image is not found. Instead found ");
+ puts(genimg_get_os_name(spl_image->os));
+ puts(". Trying to start U-boot\n");
return -ENOENT;
}
Re: U-Boot ECDSA Implementation Question
Hi Tim, On 2/5/21 8:35 AM, Simon Glass wrote: I’m a current intern at Microsoft, and one of my priorities is to enable ECDSA for U-Boot image signing/verification. Simon mentioned someone is already working on ECC, it would be great to get synced up with related progress. For signing, I will likely replicate the existing approach of using the openssl library. I’m aware that signing happens on a host machine and verification happens during boot, which implies verification should have a custom implementation to avoid the openssl overhead in the U-Boot binary. My thoughts are to copy an ECC verification implementation from a well-tested widely-used open source project. I was wondering, is U-Boot’s current RSA verification copied from another project? If so, how are security patches between the two copies of code usually handled? I’m thinking of deriving from the ECDSA implementation currently in the Linux kernel, though I’d also appreciate suggestions if there’s a better/more widely tested & used implementation. [snip] Alexandru Gagniuc, on cc, has been looking at implementing the signing side of this recently and has sent some patches that you could look at. I hope I can save you some effort on the signing side. Generally, you have two types of signed images. The first is the signed bootloader (BL2 or FSBL in ARM terms). The other one is the signed Flattened Image Tree (FIT) that we use in u-boot. The first one is vendor-specific, so you'd usually use vendor tools or write your own. We use mkimage to deal with the latter. I've implemented the signing part [1] for mkimage. mkimage has the ability to use hardware signing via the PKCS11 engine of openssl, which I did not implement. You can read more about it here [3]. The verification part is still being defined [4][5]. The idea is to define a UCLASS which abstracts the underlying implementation. For RSA, it's defined here [6]. My goal with ECDSA verification was to use the ROM API of the STM32MP1. This meant I don't have to write a software implementation of ECDSA. This would be useful in two ways. It would enable ECDSA verification on devices that don't support it in hardware, and would also allow us to add some unit tests for ECDSA. I suspect what you could do from here, is try to build my branch with ECDSA signing, play around with mkimage, and let us know how we can point you to the correct documentation. There's a lot of it in doc/, but it's not always easy to find. Alex [1] https://github.com/mrnuke/u-boot/commits/patch-mkimage-keyfile-v1 [2] https://github.com/mrnuke/u-boot/commit/a2ae016f2f80579962d4ab058137c8e1a4f4741f [3] https://github.com/mrnuke/u-boot/blob/3f447efcf8ad98d0eea349994810a66b453ac188/doc/uImage.FIT/signature.txt#L488 [4] https://github.com/mrnuke/u-boot/commit/31caceb0e28959881e96ea49a0b28fd44d13a947 [5] https://github.com/mrnuke/u-boot/commits/patch-stm32-ecdsa-v1 [6] https://github.com/mrnuke/u-boot/blob/7d7ce8d70287568071a5d24acb6dc74b923fe7e0/include/u-boot/rsa-mod-exp.h#L79
Re: [PATCH 3/5] arm: stm32mp: Implement support for TZC 400 controller
On 2/7/21 8:37 AM, Simon Glass wrote: Hi Alexandru, On Thu, 4 Feb 2021 at 12:56, Alexandru Gagniuc wrote: The purpose of this change is to allow configuring TrustZone (TZC) memory permissions. For example, OP-TEE expects TZC regions to be configured in a very particular way. The API presented here is intended to allow exactly that. UCLASS support is not implemented, because it would not be too useful. Changing TZC permissions needs to be done with care, so as not to cut off access to memory we are currently using. One place where we can use this is at the end of SPL, right before jumping to OP-TEE. Signed-off-by: Alexandru Gagniuc --- arch/arm/mach-stm32mp/Makefile | 1 + arch/arm/mach-stm32mp/include/mach/tzc.h | 33 ++ arch/arm/mach-stm32mp/tzc400.c | 135 +++ 3 files changed, 169 insertions(+) create mode 100644 arch/arm/mach-stm32mp/include/mach/tzc.h create mode 100644 arch/arm/mach-stm32mp/tzc400.c If this is an API you should add comments to the header file structs and functions. Is this API specific to just this chip? I've designed and validated this to set up stm32mp for starting up OP-TEE. It's a narrow use case. I can't speak for other chips. Alex
Re: [PATCH 0/5] Enable ECDSA FIT verification for stm32mp
Hi Patrick, On 2/9/21 9:08 AM, Patrick DELAUNAY wrote: [snip] For information, today the STMicroelectronics expected that the boot sequence for secure boot (with closed STM32MP1 devices) is the trusted boot chain. TF-A (BL2) => OP-TEE or => U-Boot => OS TF-A (BL32) BL2 is authenticated by ROM code, with EDCSA support. I next OpenSTLinux release (and soon after in upstream) STMicroelectronics will add FIP support for STM32MP15x; TF-A FIP allows to boot Kernel after TF-A BL2 if you want to skip U-Boot TF-A (BL2) => FIP (OP-TEE + Kernel) And the FIP allow authentication with certificate for 'secured boot' with a complete chain of trust. https://trustedfirmware-a.readthedocs.io/en/latest/index.html So the ECDSA support in SPL for STM32MP15x will be not actively supported by STMicroelectronics for product design. The boot flow I'm working on will use an authenticated BL2 as well: ROM -> SPL(BL2) -> FIT(OP-TEE -> Linux) I'm using this to boot to a 3D application very fast (a couple of seconds max). It's really cool. I even wrote a utility for signing SPL images for the ROM code to check [1]. I had looked at FIP images briefly a few months back. I didn't see any advantage over the FIT format. I also wanted to have as little code as possible, so avoiding TF-A made sense. There were also some major issues with syncing the clock tree between linux and TF-A. TF-A was a beautiful disaster. Maybe that changed, but given I have a working proof of concept, I doubt I'll be re-engineering the boot flow. I realize there won't be any STM support for SPL. openstlinux seems to have moved away from SPL. I've gotten a lot of enthusiastic support from u-boot members, as a number of people seem to really like this chip (meself included). Alex [1] https://github.com/mrnuke/stm32mp-keygen
Re: [PATCH 2/5] lib: ecdsa: Add skeleton to implement ecdsa verification in u-boot
Hi Patrick, On 2/9/21 9:11 AM, Patrick DELAUNAY wrote: Hi, On 1/11/21 4:41 PM, Alexandru Gagniuc wrote: Prepare the source tree for accepting implementations of the ECDSA algorithm. This patch deals with the boring aspects of Makefiles and Kconfig files. Signed-off-by: Alexandru Gagniuc --- include/image.h | 10 +- include/u-boot/rsa.h | 2 +- lib/Kconfig | 1 + lib/Makefile | 1 + lib/ecdsa/Kconfig | 23 +++ lib/ecdsa/Makefile | 1 + lib/ecdsa/ecdsa-verify.c | 13 + 7 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 lib/ecdsa/Kconfig create mode 100644 lib/ecdsa/Makefile create mode 100644 lib/ecdsa/ecdsa-verify.c diff --git a/include/image.h b/include/image.h index 6628173dca..1d70ba0ece 100644 --- a/include/image.h +++ b/include/image.h @@ -1198,20 +1198,20 @@ int calculate_hash(const void *data, int data_len, const char *algo, #if defined(USE_HOSTCC) # if defined(CONFIG_FIT_SIGNATURE) # define IMAGE_ENABLE_SIGN 1 -# define IMAGE_ENABLE_VERIFY 1 +# define IMAGE_ENABLE_VERIFY_RSA 1 # define IMAGE_ENABLE_VERIFY_ECDSA 1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include # else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY 0 +# define IMAGE_ENABLE_VERIFY_RSA 0 # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(RSA_VERIFY) -# define IMAGE_ENABLE_VERIFY_ECDSA 0 +# define IMAGE_ENABLE_VERIFY_RSA CONFIG_IS_ENABLED(RSA_VERIFY) +# define IMAGE_ENABLE_VERIFY_ECDSA CONFIG_IS_ENABLED(ECDSA_VERIFY) here you are using CONFIG_IS_ENABLED. This macro imply to test CONFIG_ECDSA_VERIFY or CONFIG_SPL_ECDSA_VERIFY (for SPL build) => but CONFIG_SPL_ECDSA_VERIFY is missing, I think you need to add it, as RSA This patch adds both "config ECDSA_VERIFY" and "config SPL_ECDSA_VERIFY" see @lib/ecdsa/Kconfig. I believe this achieves what you need. [snip] diff --git a/lib/Makefile b/lib/Makefile index cf64188ba5..ab86be2678 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -59,6 +59,7 @@ endif obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/ obj-$(CONFIG_$(SPL_)MD5) += md5.o +obj-$(CONFIG_ECDSA) += ecdsa/ obj-$(CONFIG_$(SPL_)ECDSA) += ecdsa/ The intent here is to use CONFIG_ECDSA to denote ECDSA support. CONFIG_ECDSA_VERIFY and CONFIG_SPL_ECDSA_VERIFY are used to enable the code in u-boot and SPL respectively. Only verification is supported on the target, so these are the only switches that enable or disable code. obj-$(CONFIG_$(SPL_)RSA) += rsa/ obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o obj-$(CONFIG_SHA1) += sha1.o diff --git a/lib/ecdsa/Kconfig b/lib/ecdsa/Kconfig new file mode 100644 index 00..1244d6b6ea --- /dev/null +++ b/lib/ecdsa/Kconfig @@ -0,0 +1,23 @@ +config ECDSA + bool "Enable ECDSA support" + depends on DM + help + This enables the ECDSA algorithm for FIT image verification in U-Boot. + See doc/uImage.FIT/signature.txt for more details. + The ECDSA algorithm is implemented using the driver model. So + CONFIG_DM is required by this library. + ECDSA is enabled for mkimage regardless of this option. + +if ECDSA + Add CONFIG_SPL_ECDSA to select independently support in SPL et/or in U-Boot as it is done for RSA + config SPL_ECDSA + bool "Use ECDSA library within in SPL" I though about an SPL_ECDSA kconfig. As mentioned above, we have independent switches to enable the code for u-boot/SPL. We can enable ECDSA support in u-boot, SPL, neither or both. What would this switch add? +config ECDSA_VERIFY + bool "Enable ECDSA verification support in U-Boot." + select SPL_ECDSA + help + Allow ECDSA signatures to be recognized and verified in U-Boot. + +config SPL_ECDSA_VERIFY + bool "Enable ECDSA verification support in SPL" + help + Allow ECDSA signatures to be recognized and verified in SPL. This is the switch for SPL (@mentioned earlier). Alex
Re: [PATCH 3/5] lib: ecdsa: Implement signature verification for crypto_algo API
On 2/9/21 9:56 AM, Patrick DELAUNAY wrote:
Hi,
[snip]
diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
index d2e6a40f4a..d84f6eb093 100644
--- a/lib/ecdsa/ecdsa-verify.c
+++ b/lib/ecdsa/ecdsa-verify.c
@@ -1,13 +1,128 @@
// SPDX-License-Identifier: GPL-2.0+
/*
+ * ECDSA signature verification for u-boot
+ *
+ * This implements the firmware-side wrapper for ECDSA verification.
It bridges
+ * the struct crypto_algo API to the ECDSA uclass implementations.
+ *
* Copyright (c) 2020, Alexandru Gagniuc
*/
http://www.denx.de/wiki/U-Boot/CodingStyle #Include files
#include
it is normally the first in alphabetic order of directory
Thank your for catching that. I will have it fixed in the next iteration
of this series.
+#include
#include
+#include
+
+/*
+ * Derive size of an ECDSA key from the curve name
+ *
+ * While it's possible to extract the key size by using string
manipulation,
+ * use a list of known curves for the time being.
+ */
+static int ecdsa_key_size(const char *curve_name)
+{
+ if (!strcmp(curve_name, "prime256v1"))
+ return 256;
+ else
+ return 0;
+}
+
To prepare the future can you parse a array of supported curves with
associated ID
used as parameter of ECDSA parameter = enum ECDSA_CURVES
for example: char * name, int size, enum ECDSA_CURVES
const [] = {
{"prime256v1", 256, ECDSA_PRIME256V1 },
}
That is possible. If I were to have a longer list of curve names, I
change things to extract the key length from the name itself. So instead
of running strncmp() of N keyname strings, I would extract the digits
from 'curve_name'.
I chose not to do that here because I want this patch to be didactic.
That is, I'm trying to achieve the goal clearly, with the lowest number
of lines of code.
[snip]
+static int ecdsa_verify_hash(struct udevice *dev,
+ const struct image_sign_info *info,
+ const void *hash, const void *sig, uint sig_len)
+{
+ const struct ecdsa_ops *ops = device_get_ops(dev);
+ const struct checksum_algo *algo = info->checksum;
+ struct ecdsa_public_key key;
+ int sig_node, key_node, ret;
+
+ if (!ops || !ops->verify)
+ return -ENODEV;
+
+ if (info->required_keynode > 0) {
+ ret = fdt_get_key(&key, info->fdt_blob, info->required_keynode);
+ if (ret < 0)
+ return ret;
+
+ return ops->verify(dev, &key, hash, algo->checksum_len,
+ sig, sig_len);
Need to indicate the used curve here as parameter of the verify opts ?
The curve name is part of the key (struct ecdsa_public_key).
[snip]
+ ret = ops->verify(dev, &key, hash, algo->checksum_len,
+ sig, sig_len);
+
+ /* On success, don't worry about remaining keys */
+ if (ret == 0)
issue raised by chekpatch I think
if (!ret)
Oh! I'll get this fixed. Thanks!
Alex
Re: [PATCH v5 6/6] test/py: ecdsa: Add test for mkimage ECDSA signing
On 2/1/21 2:43 PM, Simon Glass wrote: Hi Alexandru, [snip] As mentioned earlier, this does need a test that checks the U-Boot code paths. This just seems to be checking the signing process. This likely involves implementing the verification (or a fake of it) in sandbox. If I am missing something, please let me know. Yes! The test runs mkimage -F, which tests the host paths we've added. There's no target u-boot code in this series. Alex
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] 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/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 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 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 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 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 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)
>
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 v5 6/6] test/py: ecdsa: Add test for mkimage ECDSA signing
On 2/17/21 5:03 PM, Tom Rini wrote: On Thu, Jan 28, 2021 at 09:52:48AM -0600, Alexandru Gagniuc wrote: Add a test to make sure that the ECDSA signatures generated by mkimage can be verified successfully. pyCryptodomex was chosen as the crypto library because it integrates much better with python code. Using openssl would have been unnecessarily painful. Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass So, to run this test I've done a "pip install -r test/py/requirements.txt" to make sure I have everything now needed installed. When I run this test (building in /tmp): +/tmp/.bm-work/sandbox/tools/mkimage -F /tmp/.bm-work/sandbox/test.fit -k/tmp/.bm-work/sandbox/ecdsa-test-key.pem Can not get key file '/tmp/.bm-work/sandbox/ecdsa-test-key.pem/dev.pem' Can not get key file '/tmp/.bm-work/sandbox/ecdsa-test-key.pem/dev.pem' Failed to sign 'signature' signature node in 'kernel' image node: -2 Failed to sign 'signature' signature node in 'fdt-1' image node: -2 FIT description: Chrome OS kernel image with one or more FDT blobs ... +fdtget -tbi /tmp/.bm-work/sandbox/test.fit /images/kernel/signature value Error at 'value': FDT_ERR_NOTFOUND Which I think means that since we have a key-name-hint of "dev" it's taking the -k argument as a keydir and that's where it goes wrong. Did this happen with this series alone? I realize not that also applying "mkimage: Add a 'keyfile' argument for image signing" would cause this. I shoudl have (but forgot to) update the test in that series. I'll update the other series if you want to pull them in together. Alex
Frustrations of running testing
Hi, I keep being hit by two frustrating issues when trying to run tests. My expectation is that I can run a test at any time when working on something -- usually to check the correctness of that something. That's not the case today. The first one is an error message about mrproper: $ ./test/py/test.py --bd sandbox_spl .. is not clean, please run 'make mrproper' I don't think this message is very useful. Running 'mrproper' is out of the question, as it would wipe out the branch I'm currently working on. So what I end up doing is deleting the mrimproper check from the Makefile. And while it's a fast workaround, it dirties the tree and gets in the way of rebasing patches or working with git. I can continue just fine without 'mrproper death' so I really don't understand the over-abundance of caution in breaking the build. The second issue is the how the tests are trying to build graphics: $ ./test/py/test.py --bd sandbox_spl -k "whatever" make[1]: sdl2-config: Command not found ../arch/sandbox/cpu/sdl.c:10:10: fatal error: SDL2/SDL.h: No such file or directory 10 | #include The solution is to set NO_SDL=1, but this is far from obvious. The first instinct is always to look up the command help: $ ./test/py/test.py --help But this really isn't at all useful. A simple grep for sdl2-config also doesn't immediately reveal the solution. Most u-boot tools get their arguments via the commandline. Thus, a reasonably competent u-boot developer will not think that environment variables are the solution. Ergo, environment variables are not the ideal way to solve this. Maybe we could have a commandline option, and at the very least, catch this error and print something useful on the console. I'm not sure what the preferred way would be to solve the above. For me, these issues cause a significant enough disruption to my workflow, that I am very likely to not run tests regularly. I suspect I'm not alone. Alex
