[PATCH] crypto/fsl: Fallback to SW sha1/256 is misaligned buffers

[PATCH] crypto/fsl: Fallback to SW sha1/256 is misaligned buffers

[Christian Sørensen]

Problem:
With U-Boot 2021.10, we currently cannot load a fitImage on our imx7d
board, due to misaligned buffers.

Reason:
Commit 92055e138f28 ("image: Drop if/elseif hash selection in
calculate_hash()")
changed the way the FIT were verified. Previously, SW sha1/256 were always
used. Due to that commit, that can now be done in hardware.
caam_hash requires both the input, pbuf, and output buffer, pout, to be
aligned. E.g. for the kernel data, pbuf will be the data start address
for the kernel. The data start address is dependent on how the FIT is
constructed and what address the FIT is loaded to. I.e.; it is fairly
likely that we have a case of pbuf to not be aligned. pout is even more
likely to not be aligned since it is simply a stack variable declared in
fit_image_check_hash in common/image-fit.c.
So to rely upon both of these buffers to be aligned, makes errors fairly
likely.

Solution:
I wont propose copying the entire input buffer due to its size, so instead
just fallback to use the sw sha1/sha256 if buffers is misaligned.

Signed-off-by: Christian Sørensen <yocto@bsorensen.net>
---

 drivers/crypto/fsl/fsl_hash.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/fsl/fsl_hash.c b/drivers/crypto/fsl/fsl_hash.c
index 8b5c26db07..48dd10883e 100644
--- a/drivers/crypto/fsl/fsl_hash.c
+++ b/drivers/crypto/fsl/fsl_hash.c
@@ -16,6 +16,9 @@
 #include <hw_sha.h>
 #include <asm/cache.h>
 #include <linux/errno.h>
+#include <u-boot/sha1.h>
+#include <u-boot/sha256.h>
+#include <image.h>
 
 #define CRYPTO_MAX_ALG_NAME	80
 #define SHA1_DIGEST_SIZE        20
@@ -176,8 +179,12 @@ int caam_hash(const unsigned char *pbuf, unsigned int buf_len,
 
 	if (!IS_ALIGNED((uintptr_t)pbuf, ARCH_DMA_MINALIGN) ||
 	    !IS_ALIGNED((uintptr_t)pout, ARCH_DMA_MINALIGN)) {
-		puts("Error: Address arguments are not aligned\n");
-		return -EINVAL;
+		printf("Fallback to SW hash due to misaligned buffers\n");
+		if (algo == SHA1)
+			sha1_csum_wd(pbuf, buf_len, pout, CHUNKSZ_SHA1);
+		else
+			sha256_csum_wd(pbuf, buf_len, pout, CHUNKSZ_SHA256);
+		return 0;
 	}
 
 	size = ALIGN(buf_len, ARCH_DMA_MINALIGN);
-- 
2.25.1

Re: [EXT] [PATCH] crypto/fsl: Fallback to SW sha1/256 is misaligned buffers

[Ye Li]

On Fri, 2021-11-05 at 10:42 +0100, Christian Sørensen wrote:
> Caution: EXT Email
> 
> Problem:
> With U-Boot 2021.10, we currently cannot load a fitImage on our imx7d
> board, due to misaligned buffers.
> 
> Reason:
> Commit 92055e138f28 ("image: Drop if/elseif hash selection in
> calculate_hash()")
> changed the way the FIT were verified. Previously, SW sha1/256 were
> always
> used. Due to that commit, that can now be done in hardware.
> caam_hash requires both the input, pbuf, and output buffer, pout, to
> be
> aligned. E.g. for the kernel data, pbuf will be the data start
> address
> for the kernel. The data start address is dependent on how the FIT is
> constructed and what address the FIT is loaded to. I.e.; it is fairly
> likely that we have a case of pbuf to not be aligned. pout is even
> more
> likely to not be aligned since it is simply a stack variable declared
> in
> fit_image_check_hash in common/image-fit.c.
> So to rely upon both of these buffers to be aligned, makes errors
> fairly
> likely.
> 
> Solution:
> I wont propose copying the entire input buffer due to its size, so
> instead
> just fallback to use the sw sha1/sha256 if buffers is misaligned.
> 
> Signed-off-by: Christian Sørensen <yocto@bsorensen.net>
> ---
> 
>  drivers/crypto/fsl/fsl_hash.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/fsl/fsl_hash.c
> b/drivers/crypto/fsl/fsl_hash.c
> index 8b5c26db07..48dd10883e 100644
> --- a/drivers/crypto/fsl/fsl_hash.c
> +++ b/drivers/crypto/fsl/fsl_hash.c
> @@ -16,6 +16,9 @@
>  #include <hw_sha.h>
>  #include <asm/cache.h>
>  #include <linux/errno.h>
> +#include <u-boot/sha1.h>
> +#include <u-boot/sha256.h>
> +#include <image.h>
> 
>  #define CRYPTO_MAX_ALG_NAME    80
>  #define SHA1_DIGEST_SIZE        20
> @@ -176,8 +179,12 @@ int caam_hash(const unsigned char *pbuf,
> unsigned int buf_len,
> 
>         if (!IS_ALIGNED((uintptr_t)pbuf, ARCH_DMA_MINALIGN) ||
>             !IS_ALIGNED((uintptr_t)pout, ARCH_DMA_MINALIGN)) {
> -               puts("Error: Address arguments are not aligned\n");
> -               return -EINVAL;
> +               printf("Fallback to SW hash due to misaligned
> buffers\n");
> +               if (algo == SHA1)
> +                       sha1_csum_wd(pbuf, buf_len, pout,
> CHUNKSZ_SHA1);
> +               else
> +                       sha256_csum_wd(pbuf, buf_len, pout,
> CHUNKSZ_SHA256);
> +               return 0;

How about adding “#ifdef CONFIG_SHA1” and “#ifdef CONFIG_SHA256” here
?  Then it can depend on users’ selection to determine the fallback

Best regards,
Ye Li

>         }
> 
>         size = ALIGN(buf_len, ARCH_DMA_MINALIGN);
> --
> 2.25.1
> 

Re: [EXT] [PATCH] crypto/fsl: Fallback to SW sha1/256 is misaligned buffers

[Philip Oberfichtner]

Hi everybody,

On Mon, 2021-12-06 at 08:22 +0000, Ye Li wrote:
> On Fri, 2021-11-05 at 10:42 +0100, Christian Sørensen wrote:
> > Caution: EXT Email
> > 
> > Problem:
> > With U-Boot 2021.10, we currently cannot load a fitImage on our
> > imx7d
> > board, due to misaligned buffers.
> > 
> > Reason:
> > Commit 92055e138f28 ("image: Drop if/elseif hash selection in
> > calculate_hash()")
> > changed the way the FIT were verified. Previously, SW sha1/256 were
> > always
> > used. Due to that commit, that can now be done in hardware.
> > caam_hash requires both the input, pbuf, and output buffer, pout,
> > to
> > be
> > aligned. E.g. for the kernel data, pbuf will be the data start
> > address
> > for the kernel. The data start address is dependent on how the FIT
> > is
> > constructed and what address the FIT is loaded to. I.e.; it is
> > fairly
> > likely that we have a case of pbuf to not be aligned. pout is even
> > more
> > likely to not be aligned since it is simply a stack variable
> > declared
> > in
> > fit_image_check_hash in common/image-fit.c.
> > So to rely upon both of these buffers to be aligned, makes errors
> > fairly
> > likely.
> > 
> > Solution:
> > I wont propose copying the entire input buffer due to its size, so
> > instead
> > just fallback to use the sw sha1/sha256 if buffers is misaligned.
> > 

I have the same problem on my imx6d. This patch works fine for me.


> > Signed-off-by: Christian Sørensen <yocto@bsorensen.net>
> > ---
> > 
> >  drivers/crypto/fsl/fsl_hash.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/crypto/fsl/fsl_hash.c
> > b/drivers/crypto/fsl/fsl_hash.c
> > index 8b5c26db07..48dd10883e 100644
> > --- a/drivers/crypto/fsl/fsl_hash.c
> > +++ b/drivers/crypto/fsl/fsl_hash.c
> > @@ -16,6 +16,9 @@
> >  #include <hw_sha.h>
> >  #include <asm/cache.h>
> >  #include <linux/errno.h>
> > +#include <u-boot/sha1.h>
> > +#include <u-boot/sha256.h>
> > +#include <image.h>
> > 
> >  #define CRYPTO_MAX_ALG_NAME    80
> >  #define SHA1_DIGEST_SIZE        20
> > @@ -176,8 +179,12 @@ int caam_hash(const unsigned char *pbuf,
> > unsigned int buf_len,
> > 
> >         if (!IS_ALIGNED((uintptr_t)pbuf, ARCH_DMA_MINALIGN) ||
> >             !IS_ALIGNED((uintptr_t)pout, ARCH_DMA_MINALIGN)) {
> > -               puts("Error: Address arguments are not aligned\n");
> > -               return -EINVAL;
> > +               printf("Fallback to SW hash due to misaligned
> > buffers\n");
> > +               if (algo == SHA1)
> > +                       sha1_csum_wd(pbuf, buf_len, pout,
> > CHUNKSZ_SHA1);
> > +               else
> > +                       sha256_csum_wd(pbuf, buf_len, pout,
> > CHUNKSZ_SHA256);
> > +               return 0;
> 
> How about adding “#ifdef CONFIG_SHA1” and “#ifdef CONFIG_SHA256” here
> ?  Then it can depend on users’ selection to determine the fallback
> 
> Best regards,
> Ye Li
> 
> >         }
> > 
> >         size = ALIGN(buf_len, ARCH_DMA_MINALIGN);
> > --
> > 2.25.1

Is this patch still active? The change request is already two months
old. If the original patch request is abandoned, should I contribute a
[PATCH v2]?

Best Regards,
Philip Oberfichtner