| Message ID | 20201006100945.18331-2-rasmus.villemoes@prevas.dk |
|---|---|
| State | Accepted |
| Commit | 3f8808ebaa901ce18a7dfb3432e68e9c3a79f244 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fix verified boot on BE hosts | expand |
Hi Rasmus, On Tue, 6 Oct 2020 at 04:10, Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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 <rasmus.villemoes@prevas.dk> > --- > lib/rsa/rsa-mod-exp.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> Is there a way to add a test for this? Regards, Simon
On 07/10/2020 00.02, Simon Glass wrote: > Hi Rasmus, > > On Tue, 6 Oct 2020 at 04:10, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> 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 <rasmus.villemoes@prevas.dk> >> --- >> lib/rsa/rsa-mod-exp.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > 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
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 > > <rasmus.villemoes@prevas.dk> 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 <rasmus.villemoes@prevas.dk> > >> --- > >> lib/rsa/rsa-mod-exp.c | 11 +++++++++-- > >> 1 file changed, 9 insertions(+), 2 deletions(-) > >> > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > 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?
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 <sjg@chromium.org> >>> >>> 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
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 <sjg@chromium.org> > >>> > >>> 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.
Hi Rasmus, On Mon, 12 Oct 2020 at 01:04, Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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 <sjg@chromium.org> > >>> > >>> 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
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 <rasmus.villemoes@prevas.dk> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
diff --git a/lib/rsa/rsa-mod-exp.c b/lib/rsa/rsa-mod-exp.c index a437cbe26f..78c688d14c 100644 --- a/lib/rsa/rsa-mod-exp.c +++ b/lib/rsa/rsa-mod-exp.c @@ -25,6 +25,14 @@ #define get_unaligned_be32(a) fdt32_to_cpu(*(uint32_t *)a) #define put_unaligned_be32(a, b) (*(uint32_t *)(b) = cpu_to_fdt32(a)) +static inline uint64_t fdt64_to_cpup(const void *p) +{ + fdt64_t w; + + memcpy(&w, p, sizeof(w)); + return fdt64_to_cpu(w); +} + /* Default public exponent for backward compatibility */ #define RSA_DEFAULT_PUBEXP 65537 @@ -263,8 +271,7 @@ int rsa_mod_exp_sw(const uint8_t *sig, uint32_t sig_len, if (!prop->public_exponent) key.exponent = RSA_DEFAULT_PUBEXP; else - rsa_convert_big_endian((uint32_t *)&key.exponent, - prop->public_exponent, 2); + key.exponent = fdt64_to_cpup(prop->public_exponent); if (!key.len || !prop->modulus || !prop->rr) { debug("%s: Missing RSA key info", __func__);
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 <rasmus.villemoes@prevas.dk> --- lib/rsa/rsa-mod-exp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)