Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
On Tue, Oct 06, 2020 at 12:09:45PM +0200, Rasmus Villemoes wrote:
> Commit fdf0819afb (rsa: fix alignment issue when getting public
> exponent) changed the logic to avoid doing an 8-byte access to a
> possibly-not-8-byte-aligned address.
>
> However, using rsa_convert_big_endian is wrong: That function converts
> an array of big-endian (32-bit) words with the most significant word
> first (aka a BE byte array) to an array of cpu-endian words with the
> least significant word first. While the exponent is indeed _stored_ as
> a big-endian 64-bit word (two BE words with MSW first), we want to
> extract it as a cpu-endian 64 bit word. On a little-endian host,
> swapping the words and byte-swapping each 32-bit word works, because
> that's the same as byte-swapping the whole 64 bit word. But on a
> big-endian host, the fdt32_to_cpu are no-ops, but
> rsa_convert_big_endian() still does the word-swapping, breaking
> verified boot.
>
> To fix that, while still ensuring we don't do unaligned accesses, add
> a little helper that first memcpy's the bytes to a local fdt64_t, then
> applies fdt64_to_cpu(). [The name is chosen based on the
> [bl]eXX_to_cpup in linux/byteorder/generic.h].
>
> Fixes: fdf0819afb ("rsa: fix alignment issue when getting public exponent")
> Signed-off-by: Rasmus Villemoes
> Reviewed-by: Simon Glass
Applied to u-boot/master, thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
Hi Rasmus, On Mon, 12 Oct 2020 at 01:04, Rasmus Villemoes wrote: > > On 09/10/2020 15.08, Tom Rini wrote: > > On Wed, Oct 07, 2020 at 12:17:56AM +0200, Rasmus Villemoes wrote: > >> On 07/10/2020 00.02, Simon Glass wrote: > >>> > >>> Reviewed-by: Simon Glass > >>> > >>> Is there a way to add a test for this? > >> > >> Not that I can think of, other than finding some BE board and hooking it > >> up in some CI. Apparently not very many people use verified boot on BE > >> platforms :( or at least they don't follow upstream U-Boot closely. > > > > We have tests for verified boot for sandbox. Can we not expand them to > > run on qemu* including ppce500? > > Perhaps. I didn't know sandbox was supposed to be buildable for other > arches than x86ish. I just tried doing the naive thing, > > $ export ARCH=powerpc > $ export CROSS_COMPILE=powerpc-linux-gnu- > $ make sandbox_defconfig > $ make -j8 > include/config_distro_bootcmd.h:335:3: error: #error "sandbox EFI > support is only supported on ARM and x86" > > After trimming the .config to git rid of that, I get a million other > warnings and errors, so that doesn't seem to be the right way. For this you would need to build on a power PC linux machine. I think someone did get it running on ARM but I'm not sure if anyone else has tried with As Tom said, he was referring to building a board target (not sandbox) and running a test under qemu. Probably could just have a little test that calls rsa_mod_exp_sw(), I suppose. But it is a lot of setup for just a small thing. > > Can we please apply the patch to unbreak actual boards even if there's > no regression test for it? Regards, Simon
Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
On Mon, Oct 12, 2020 at 09:04:28AM +0200, Rasmus Villemoes wrote: > On 09/10/2020 15.08, Tom Rini wrote: > > On Wed, Oct 07, 2020 at 12:17:56AM +0200, Rasmus Villemoes wrote: > >> On 07/10/2020 00.02, Simon Glass wrote: > >>> > >>> Reviewed-by: Simon Glass > >>> > >>> Is there a way to add a test for this? > >> > >> Not that I can think of, other than finding some BE board and hooking it > >> up in some CI. Apparently not very many people use verified boot on BE > >> platforms :( or at least they don't follow upstream U-Boot closely. > > > > We have tests for verified boot for sandbox. Can we not expand them to > > run on qemu* including ppce500? > > Perhaps. I didn't know sandbox was supposed to be buildable for other > arches than x86ish. I just tried doing the naive thing, > > $ export ARCH=powerpc > $ export CROSS_COMPILE=powerpc-linux-gnu- > $ make sandbox_defconfig > $ make -j8 > include/config_distro_bootcmd.h:335:3: error: #error "sandbox EFI > support is only supported on ARM and x86" > > After trimming the .config to git rid of that, I get a million other > warnings and errors, so that doesn't seem to be the right way. Sorry, I meant the literal QEMU targets that we use. > Can we please apply the patch to unbreak actual boards even if there's > no regression test for it? Yes, sorry it wasn't clear. I will pick this up soon, with a bunch of other stuff. -- Tom signature.asc Description: PGP signature
Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
On 09/10/2020 15.08, Tom Rini wrote: > On Wed, Oct 07, 2020 at 12:17:56AM +0200, Rasmus Villemoes wrote: >> On 07/10/2020 00.02, Simon Glass wrote: >>> >>> Reviewed-by: Simon Glass >>> >>> Is there a way to add a test for this? >> >> Not that I can think of, other than finding some BE board and hooking it >> up in some CI. Apparently not very many people use verified boot on BE >> platforms :( or at least they don't follow upstream U-Boot closely. > > We have tests for verified boot for sandbox. Can we not expand them to > run on qemu* including ppce500? Perhaps. I didn't know sandbox was supposed to be buildable for other arches than x86ish. I just tried doing the naive thing, $ export ARCH=powerpc $ export CROSS_COMPILE=powerpc-linux-gnu- $ make sandbox_defconfig $ make -j8 include/config_distro_bootcmd.h:335:3: error: #error "sandbox EFI support is only supported on ARM and x86" After trimming the .config to git rid of that, I get a million other warnings and errors, so that doesn't seem to be the right way. Can we please apply the patch to unbreak actual boards even if there's no regression test for it? Rasmus
Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
On Wed, Oct 07, 2020 at 12:17:56AM +0200, Rasmus Villemoes wrote:
> On 07/10/2020 00.02, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 6 Oct 2020 at 04:10, Rasmus Villemoes
> > wrote:
> >>
> >> Commit fdf0819afb (rsa: fix alignment issue when getting public
> >> exponent) changed the logic to avoid doing an 8-byte access to a
> >> possibly-not-8-byte-aligned address.
> >>
> >> However, using rsa_convert_big_endian is wrong: That function converts
> >> an array of big-endian (32-bit) words with the most significant word
> >> first (aka a BE byte array) to an array of cpu-endian words with the
> >> least significant word first. While the exponent is indeed _stored_ as
> >> a big-endian 64-bit word (two BE words with MSW first), we want to
> >> extract it as a cpu-endian 64 bit word. On a little-endian host,
> >> swapping the words and byte-swapping each 32-bit word works, because
> >> that's the same as byte-swapping the whole 64 bit word. But on a
> >> big-endian host, the fdt32_to_cpu are no-ops, but
> >> rsa_convert_big_endian() still does the word-swapping, breaking
> >> verified boot.
> >>
> >> To fix that, while still ensuring we don't do unaligned accesses, add
> >> a little helper that first memcpy's the bytes to a local fdt64_t, then
> >> applies fdt64_to_cpu(). [The name is chosen based on the
> >> [bl]eXX_to_cpup in linux/byteorder/generic.h].
> >>
> >> Fixes: fdf0819afb ("rsa: fix alignment issue when getting public exponent")
> >> Signed-off-by: Rasmus Villemoes
> >> ---
> >> lib/rsa/rsa-mod-exp.c | 11 +--
> >> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >
> > Reviewed-by: Simon Glass
> >
> > Is there a way to add a test for this?
>
> Not that I can think of, other than finding some BE board and hooking it
> up in some CI. Apparently not very many people use verified boot on BE
> platforms :( or at least they don't follow upstream U-Boot closely.
We have tests for verified boot for sandbox. Can we not expand them to
run on qemu* including ppce500?
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
On 07/10/2020 00.02, Simon Glass wrote:
> Hi Rasmus,
>
> On Tue, 6 Oct 2020 at 04:10, Rasmus Villemoes
> wrote:
>>
>> Commit fdf0819afb (rsa: fix alignment issue when getting public
>> exponent) changed the logic to avoid doing an 8-byte access to a
>> possibly-not-8-byte-aligned address.
>>
>> However, using rsa_convert_big_endian is wrong: That function converts
>> an array of big-endian (32-bit) words with the most significant word
>> first (aka a BE byte array) to an array of cpu-endian words with the
>> least significant word first. While the exponent is indeed _stored_ as
>> a big-endian 64-bit word (two BE words with MSW first), we want to
>> extract it as a cpu-endian 64 bit word. On a little-endian host,
>> swapping the words and byte-swapping each 32-bit word works, because
>> that's the same as byte-swapping the whole 64 bit word. But on a
>> big-endian host, the fdt32_to_cpu are no-ops, but
>> rsa_convert_big_endian() still does the word-swapping, breaking
>> verified boot.
>>
>> To fix that, while still ensuring we don't do unaligned accesses, add
>> a little helper that first memcpy's the bytes to a local fdt64_t, then
>> applies fdt64_to_cpu(). [The name is chosen based on the
>> [bl]eXX_to_cpup in linux/byteorder/generic.h].
>>
>> Fixes: fdf0819afb ("rsa: fix alignment issue when getting public exponent")
>> Signed-off-by: Rasmus Villemoes
>> ---
>> lib/rsa/rsa-mod-exp.c | 11 +--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>
> Reviewed-by: Simon Glass
>
> Is there a way to add a test for this?
Not that I can think of, other than finding some BE board and hooking it
up in some CI. Apparently not very many people use verified boot on BE
platforms :( or at least they don't follow upstream U-Boot closely.
Rasmus
Re: [PATCH 1/1] rsa: fix retrieving public exponent on big-endian systems
Hi Rasmus,
On Tue, 6 Oct 2020 at 04:10, Rasmus Villemoes
wrote:
>
> Commit fdf0819afb (rsa: fix alignment issue when getting public
> exponent) changed the logic to avoid doing an 8-byte access to a
> possibly-not-8-byte-aligned address.
>
> However, using rsa_convert_big_endian is wrong: That function converts
> an array of big-endian (32-bit) words with the most significant word
> first (aka a BE byte array) to an array of cpu-endian words with the
> least significant word first. While the exponent is indeed _stored_ as
> a big-endian 64-bit word (two BE words with MSW first), we want to
> extract it as a cpu-endian 64 bit word. On a little-endian host,
> swapping the words and byte-swapping each 32-bit word works, because
> that's the same as byte-swapping the whole 64 bit word. But on a
> big-endian host, the fdt32_to_cpu are no-ops, but
> rsa_convert_big_endian() still does the word-swapping, breaking
> verified boot.
>
> To fix that, while still ensuring we don't do unaligned accesses, add
> a little helper that first memcpy's the bytes to a local fdt64_t, then
> applies fdt64_to_cpu(). [The name is chosen based on the
> [bl]eXX_to_cpup in linux/byteorder/generic.h].
>
> Fixes: fdf0819afb ("rsa: fix alignment issue when getting public exponent")
> Signed-off-by: Rasmus Villemoes
> ---
> lib/rsa/rsa-mod-exp.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
Reviewed-by: Simon Glass
Is there a way to add a test for this?
Regards,
Simon
