[PATCH] rsa: fix alignment issue when getting public exponent

[PATCH] rsa: fix alignment issue when getting public exponent

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
did a cast to uint64_t of the key_prop->public_exponent field.
But that alignment is not guaranteed in all cases.

This came to light when in my spl-fit-signature the key-name exceeded
a certain length and with it the verification then started failing.
(naming it "integrity" worked fine, "integrity-uboot" failed)

key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
also just returns such a void-pointer and inside the devicetree the 64bit
exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
can lead to false reads.

So just use the already existing rsa_convert_big_endian() to do the actual
conversion from the dt's big-endian to the needed uint64 value.

Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 lib/rsa/rsa-mod-exp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/rsa/rsa-mod-exp.c b/lib/rsa/rsa-mod-exp.c
index 420ab2eba0..62b2557cc2 100644
--- a/lib/rsa/rsa-mod-exp.c
+++ b/lib/rsa/rsa-mod-exp.c
@@ -262,8 +262,8 @@ int rsa_mod_exp_sw(const uint8_t *sig, uint32_t sig_len,
 	if (!prop->public_exponent)
 		key.exponent = RSA_DEFAULT_PUBEXP;
 	else
-		key.exponent =
-			fdt64_to_cpu(*((uint64_t *)(prop->public_exponent)));
+		rsa_convert_big_endian((uint32_t *)&key.exponent,
+				       prop->public_exponent, 2);
 
 	if (!key.len || !prop->modulus || !prop->rr) {
 		debug("%s: Missing RSA key info", __func__);
-- 
2.25.1

[PATCH] rsa: fix alignment issue when getting public exponent

[Philipp Tomsich]

> On 03.05.2020, at 13:26, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> did a cast to uint64_t of the key_prop->public_exponent field.
> But that alignment is not guaranteed in all cases.
> 
> This came to light when in my spl-fit-signature the key-name exceeded
> a certain length and with it the verification then started failing.
> (naming it "integrity" worked fine, "integrity-uboot" failed)
> 
> key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> also just returns such a void-pointer and inside the devicetree the 64bit
> exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> can lead to false reads.
> 
> So just use the already existing rsa_convert_big_endian() to do the actual
> conversion from the dt's big-endian to the needed uint64 value.
> 
> Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[PATCH] rsa: fix alignment issue when getting public exponent

[Simon Glass]

+Tom Rini

On Sun, 3 May 2020 at 05:26, Heiko Stuebner <heiko@sntech.de> wrote:
>
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>
> To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> did a cast to uint64_t of the key_prop->public_exponent field.
> But that alignment is not guaranteed in all cases.
>
> This came to light when in my spl-fit-signature the key-name exceeded
> a certain length and with it the verification then started failing.
> (naming it "integrity" worked fine, "integrity-uboot" failed)
>
> key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> also just returns such a void-pointer and inside the devicetree the 64bit
> exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> can lead to false reads.
>
> So just use the already existing rsa_convert_big_endian() to do the actual
> conversion from the dt's big-endian to the needed uint64 value.
>
> Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  lib/rsa/rsa-mod-exp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Nice find! This probably changed when we updated the DT recently since
the unaligned-access thing was reverted I think. Or has this problem
been there forever?

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH] rsa: fix alignment issue when getting public exponent

[Heiko Stübner]

Am Montag, 4. Mai 2020, 16:17:52 CEST schrieb Simon Glass:
> +Tom Rini
> 
> On Sun, 3 May 2020 at 05:26, Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >
> > To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> > did a cast to uint64_t of the key_prop->public_exponent field.
> > But that alignment is not guaranteed in all cases.
> >
> > This came to light when in my spl-fit-signature the key-name exceeded
> > a certain length and with it the verification then started failing.
> > (naming it "integrity" worked fine, "integrity-uboot" failed)
> >
> > key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> > also just returns such a void-pointer and inside the devicetree the 64bit
> > exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> > can lead to false reads.
> >
> > So just use the already existing rsa_convert_big_endian() to do the actual
> > conversion from the dt's big-endian to the needed uint64 value.
> >
> > Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > ---
> >  lib/rsa/rsa-mod-exp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> 
> Nice find! This probably changed when we updated the DT recently since
> the unaligned-access thing was reverted I think. Or has this problem
> been there forever?

To me it looks like it must've been present since the beginning.
In commit e0f2f1553414 ("Implement generalised RSA public exponents for verified boot")
which introduced the exponent handling it already did:

const uint64_t *public_exponent;
public_exponent = fdt_getprop(blob, node, "rsa,exponent", &length);

So if the fdt_getprop internals didn't change since then it would've
even then cast the void* to an uint64*


I really don't understand yet why the longer key-name would trigger it
though ... names like "dev", "integrity" work fine and seemingly starting
at a certain length the alignment changed.


Heiko

> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

[PATCH] rsa: fix alignment issue when getting public exponent

[Simon Glass]

Hi Heiko,

On Mon, 4 May 2020 at 09:40, Heiko St?bner <heiko@sntech.de> wrote:
>
> Am Montag, 4. Mai 2020, 16:17:52 CEST schrieb Simon Glass:
> > +Tom Rini
> >
> > On Sun, 3 May 2020 at 05:26, Heiko Stuebner <heiko@sntech.de> wrote:
> > >
> > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > >
> > > To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> > > did a cast to uint64_t of the key_prop->public_exponent field.
> > > But that alignment is not guaranteed in all cases.
> > >
> > > This came to light when in my spl-fit-signature the key-name exceeded
> > > a certain length and with it the verification then started failing.
> > > (naming it "integrity" worked fine, "integrity-uboot" failed)
> > >
> > > key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> > > also just returns such a void-pointer and inside the devicetree the 64bit
> > > exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> > > can lead to false reads.
> > >
> > > So just use the already existing rsa_convert_big_endian() to do the actual
> > > conversion from the dt's big-endian to the needed uint64 value.
> > >
> > > Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > ---
> > >  lib/rsa/rsa-mod-exp.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> >
> > Nice find! This probably changed when we updated the DT recently since
> > the unaligned-access thing was reverted I think. Or has this problem
> > been there forever?
>
> To me it looks like it must've been present since the beginning.
> In commit e0f2f1553414 ("Implement generalised RSA public exponents for verified boot")
> which introduced the exponent handling it already did:
>
> const uint64_t *public_exponent;
> public_exponent = fdt_getprop(blob, node, "rsa,exponent", &length);
>
> So if the fdt_getprop internals didn't change since then it would've
> even then cast the void* to an uint64*
>

See this patch:

e8c2d25845 libfdt: Revert 6dcb8ba4 from upstream libfdt

> I really don't understand yet why the longer key-name would trigger it
> though ... names like "dev", "integrity" work fine and seemingly starting
> at a certain length the alignment changed.

My guess is that these lengths work:

1-3 chars (aligns to 4 bytes)
8-11 chars (aligns to 12 bytes)
16-19 chars (aligns to 20 bytes)
etc.

Regards,
Simon

[PATCH] rsa: fix alignment issue when getting public exponent

[Jan Kiszka]

On 03.05.20 13:26, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> did a cast to uint64_t of the key_prop->public_exponent field.
> But that alignment is not guaranteed in all cases.
> 
> This came to light when in my spl-fit-signature the key-name exceeded
> a certain length and with it the verification then started failing.
> (naming it "integrity" worked fine, "integrity-uboot" failed)
> 
> key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> also just returns such a void-pointer and inside the devicetree the 64bit
> exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> can lead to false reads.
> 
> So just use the already existing rsa_convert_big_endian() to do the actual
> conversion from the dt's big-endian to the needed uint64 value.
> 
> Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>   lib/rsa/rsa-mod-exp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/rsa/rsa-mod-exp.c b/lib/rsa/rsa-mod-exp.c
> index 420ab2eba0..62b2557cc2 100644
> --- a/lib/rsa/rsa-mod-exp.c
> +++ b/lib/rsa/rsa-mod-exp.c
> @@ -262,8 +262,8 @@ int rsa_mod_exp_sw(const uint8_t *sig, uint32_t sig_len,
>   	if (!prop->public_exponent)
>   		key.exponent = RSA_DEFAULT_PUBEXP;
>   	else
> -		key.exponent =
> -			fdt64_to_cpu(*((uint64_t *)(prop->public_exponent)));
> +		rsa_convert_big_endian((uint32_t *)&key.exponent,
> +				       prop->public_exponent, 2);
>   
>   	if (!key.len || !prop->modulus || !prop->rr) {
>   		debug("%s: Missing RSA key info", __func__);
> 

Tested-by: Jan Kiszka <jan.kiszka@siemens.com>

I debugged the same issue on our AM654x-based board where we do fit 
image verification in SPL. As there is apparently no unaligned access 
possible, this bit us as well. Obsoletes my own patch attempt.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[PATCH] rsa: fix alignment issue when getting public exponent

[Tom Rini]

On Sun, May 03, 2020 at 01:26:34PM +0200, Heiko Stuebner wrote:

> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> did a cast to uint64_t of the key_prop->public_exponent field.
> But that alignment is not guaranteed in all cases.
> 
> This came to light when in my spl-fit-signature the key-name exceeded
> a certain length and with it the verification then started failing.
> (naming it "integrity" worked fine, "integrity-uboot" failed)
> 
> key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> also just returns such a void-pointer and inside the devicetree the 64bit
> exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> can lead to false reads.
> 
> So just use the already existing rsa_convert_big_endian() to do the actual
> conversion from the dt's big-endian to the needed uint64 value.
> 
> Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200507/29b21d93/attachment.sig>

[PATCH] rsa: fix alignment issue when getting public exponent

[Tom Rini]

On Thu, May 07, 2020 at 09:10:17AM +0200, Jan Kiszka wrote:
> On 03.05.20 13:26, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > To fill the exponent field of the rsa_public_key struct, rsa_mod_exp_sw
> > did a cast to uint64_t of the key_prop->public_exponent field.
> > But that alignment is not guaranteed in all cases.
> > 
> > This came to light when in my spl-fit-signature the key-name exceeded
> > a certain length and with it the verification then started failing.
> > (naming it "integrity" worked fine, "integrity-uboot" failed)
> > 
> > key_prop.public_exponent itself is actually a void-pointer, fdt_getprop()
> > also just returns such a void-pointer and inside the devicetree the 64bit
> > exponent is represented as 2 32bit numbers, so assuming a 64bit alignment
> > can lead to false reads.
> > 
> > So just use the already existing rsa_convert_big_endian() to do the actual
> > conversion from the dt's big-endian to the needed uint64 value.
> > 
> > Fixes: fc2f4246b4b3 ("rsa: Split the rsa-verify to separate the modular exponentiation")
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >   lib/rsa/rsa-mod-exp.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/rsa/rsa-mod-exp.c b/lib/rsa/rsa-mod-exp.c
> > index 420ab2eba0..62b2557cc2 100644
> > --- a/lib/rsa/rsa-mod-exp.c
> > +++ b/lib/rsa/rsa-mod-exp.c
> > @@ -262,8 +262,8 @@ int rsa_mod_exp_sw(const uint8_t *sig, uint32_t sig_len,
> >   	if (!prop->public_exponent)
> >   		key.exponent = RSA_DEFAULT_PUBEXP;
> >   	else
> > -		key.exponent =
> > -			fdt64_to_cpu(*((uint64_t *)(prop->public_exponent)));
> > +		rsa_convert_big_endian((uint32_t *)&key.exponent,
> > +				       prop->public_exponent, 2);
> >   	if (!key.len || !prop->modulus || !prop->rr) {
> >   		debug("%s: Missing RSA key info", __func__);
> > 
> 
> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I debugged the same issue on our AM654x-based board where we do fit image
> verification in SPL. As there is apparently no unaligned access possible,
> this bit us as well. Obsoletes my own patch attempt.

And, blarg, I just put your tested-by on the mkimage revert and not this
bugfix, sorry!  I should go make coffee now...

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200507/718907c1/attachment.sig>