diff mbox series

rsa: fix alignment issue when getting public exponent

Message ID 20200503112634.590399-1-heiko@sntech.de
State New
Headers show
Series rsa: fix alignment issue when getting public exponent | expand

Commit Message

Heiko Stuebner May 3, 2020, 11:26 a.m. UTC 
From: Heiko Stuebner <heiko.stuebner at 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 at theobroma-systems.com>
---
 lib/rsa/rsa-mod-exp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Philipp Tomsich May 3, 2020, 12:13 p.m. UTC | #1 
> On 03.05.2020, at 13:26, Heiko Stuebner <heiko at sntech.de> wrote:
> 
> From: Heiko Stuebner <heiko.stuebner at 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 at theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
Simon Glass May 4, 2020, 2:17 p.m. UTC | #2 
+Tom Rini

On Sun, 3 May 2020 at 05:26, Heiko Stuebner <heiko at sntech.de> wrote:
>
> From: Heiko Stuebner <heiko.stuebner at 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 at 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 at chromium.org>
Heiko Stuebner May 4, 2020, 3:40 p.m. UTC | #3 
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 at sntech.de> wrote:
> >
> > From: Heiko Stuebner <heiko.stuebner at 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 at 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 at chromium.org>
>
Simon Glass May 4, 2020, 5:06 p.m. UTC | #4 
Hi Heiko,

On Mon, 4 May 2020 at 09:40, Heiko St?bner <heiko at 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 at sntech.de> wrote:
> > >
> > > From: Heiko Stuebner <heiko.stuebner at 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 at 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
Jan Kiszka May 7, 2020, 7:10 a.m. UTC | #5 
On 03.05.20 13:26, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner at 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 at theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> Reviewed-by: Simon Glass <sjg at 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 at 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
Tom Rini May 7, 2020, 1:04 p.m. UTC | #6 
On Sun, May 03, 2020 at 01:26:34PM +0200, Heiko Stuebner wrote:

> From: Heiko Stuebner <heiko.stuebner at 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 at theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> Reviewed-by: Simon Glass <sjg at chromium.org>

Applied to u-boot/master, thanks!
Tom Rini May 7, 2020, 1:05 p.m. UTC | #7 
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 at 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 at theobroma-systems.com>
> > Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> > Reviewed-by: Simon Glass <sjg at 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 at 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...
diff mbox series

Patch

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__);