[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 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