Re: [U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

22 Nov 2018

      Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut marex@denx.de geschrieben:
...
On 11/21/2018 10:07 PM, Simon Goldschmidt wrote:
...
On 21.11.2018 15:08, Marek Vasut wrote:
...
On 11/21/2018 06:09 AM, Simon Goldschmidt wrote:
...
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <marex@denx.de
mailto:marex@denx.de> geschrieben:
 On 11/20/2018 09:54 PM, Simon Goldschmidt wrote:
 > On 20.11.2018 20:33, Marek Vasut wrote:
 >> On 11/20/2018 08:22 PM, Simon Goldschmidt wrote:
 >>> From: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com
 <mailto:sgoldschmidt@de.pepperl-fuchs.com>>
 >>>
 >>> On socfpga gen5, a warm reboot from Linux currently triggers

a warm
     >>> reset via reset manager ctrl register.
     >>>
     >>> This currently leads to the boot rom just jumping to onchip
ram
...
...
...
 >>> executing the SPL that is supposed to still be there. This is
 >>> because we tell the boot rom to do so by writing a magin value
 >>> the warmramgrp_enable register in arch_early_init_r().
 >>>
 >>> However, this can lead to lockups on reboot if this register

still
     >>> contains its magic value but the SPL is not intact any more
(e.g.
     >>> partly overwritten onchip ram).
     >>>
     >>> To fis this, store a crc calculated over SPL in sysmgr
registers to
     >>> let the boot rom check it on next warm boot. If the crc is
still
     >>> correct, SPL can be executd from onchip ram. If the crc
fails, SPL
     >>> is loaded from original boot source.
     >>>
     >>> The crc that is written to the warmramgrp_crc register is
the crc
     >>> found in the SPL sfp image but with one addiional u32 added.
For
     >>> this, we need to add a function to calculate the updated
crc. This
     >>> is done as a bitwise calculation to keep the code increase
small.
     >>>
     >>> This whole patch added 96 bytes to .text for SPL for
     >>> socfpga_socrates_defconfig.
     >>>
     >>> Signed-off-by: Simon Goldschmidt
     <simon.k.r.goldschmidt@gmail.com
     mailto:simon.k.r.goldschmidt@gmail.com>
     >>> ---
     >>>
     >>>   arch/arm/mach-socfpga/misc_gen5.c |  9 ----
     >>>   arch/arm/mach-socfpga/spl_gen5.c  | 73
     +++++++++++++++++++++++++++++++
     >>>   2 files changed, 73 insertions(+), 9 deletions(-)
     >>>
     >>> diff --git a/arch/arm/mach-socfpga/misc_gen5.c
     >>> b/arch/arm/mach-socfpga/misc_gen5.c
     >>> index 5fa40937c4..492a3082de 100644
     >>> --- a/arch/arm/mach-socfpga/misc_gen5.c
     >>> +++ b/arch/arm/mach-socfpga/misc_gen5.c
     >>> @@ -204,15 +204,6 @@ int arch_early_init_r(void)
     >>>   {
     >>>       int i;
     >>>   -    /*
     >>> -     * Write magic value into magic register to unlock
support for
     >>> -     * issuing warm reset. The ancient kernel code expects
this
     >>> -     * value to be written into the register by the
bootloader, so
     >>> -     * to support that old code, we write it here instead
of in the
     >>> -     * reset_cpu() function just before resetting the CPU.
     >>> -     */
     >>> -    writel(0xae9efebc,
&sysmgr_regs->romcodegrp_warmramgrp_enable);
     >>> -
     >>>       for (i = 0; i < 8; i++)    /* Cache initial SW setting
regs */
     >>>           iswgrp_handoff[i] =
     readl(&sysmgr_regs->iswgrp_handoff[i]);
     >>>   diff --git a/arch/arm/mach-socfpga/spl_gen5.c
     >>> b/arch/arm/mach-socfpga/spl_gen5.c
     >>> index ccdc661d05..3416e19f79 100644
     >>> --- a/arch/arm/mach-socfpga/spl_gen5.c
     >>> +++ b/arch/arm/mach-socfpga/spl_gen5.c
     >>> @@ -63,6 +63,76 @@ u32 spl_boot_mode(const u32 boot_device)
     >>>   }
     >>>   #endif
     >>>   +/* This function calculates the CRC32 used by the Cyclone
5 SoC
     >>> Boot Rom */
     >>> +static u32 socfpga_boot_crc(u32 crc, const unsigned char
*ptr, u32
     >>> length)
     >>> +{
     >>> +    uint i;
     >>> +    u8 bit;
     >>> +    unsigned char data;
     >>> +    const u32 poly = 0x02608edb;
     >>> +
     >>> +    for (; length > 0; length--, ptr++) {
     >>> +        data = *ptr;
     >>> +        for (i = 0; i < 8; i++) {
     >>> +            if (data & 0x80)
     >>> +                bit = 1;
     >>> +            else
     >>> +                bit = 0;
     >>> +
     >>> +            data = data << 1;
     >>> +            if (crc & 0x80000000)
     >>> +                bit = 1 - bit;
     >>> +
     >>> +            if (bit) {
     >>> +                crc ^= poly;
     >>> +                crc = crc << 1;
     >>> +                crc |= 1;
     >>> +            } else {
     >>> +                crc = crc << 1;
     >>> +            }
     >>> +        }
     >>> +    }
     >>> +    return crc;
     >>> +}
     >>> +
     >>> +/*
     >>> + * Write magic value into magic register to unlock support
for the
     >>> boot rom to
     >>> + * execute spl from sram on warm reset. This may be
required at
     >>> least on some
     >>> + * boards that start from qspi where the flash chip might
be in a
     >>> state that
     >>> + * cannot be handled by the boot rom (e.g. 4 byte mode).
     >>> + *
     >>> + * To prevent just jumping to corrupted memory, a crc of
the spl is
     >>> calculated.
     >>> + * This crc is loaded from the running image, but has to be
     extended
     >>> by the
     >>> + * modified contents of the "datastart" register (i.e.
0xffff0000).
     >>> + */
     >>> +static void spl_init_reboot_config(void)
     >>> +{
     >>> +    u32 spl_crc, spl_length;
     >>> +    const u32 spl_start = (u32)__image_copy_start;
     >>> +    const u32 spl_start_16 = spl_start & 0xffff;
     >>> +    u32 spl_length_u32;
     >>> +
     >>> +    /* load image length from sfp header (includes crc) */
     >>> +    spl_length_u32 = *(const u16 *)(spl_start + 0x46);
     >>> +    /* subtract crc */
     >>> +    spl_length_u32--;
     >>> +    /* get length in bytes */
     >>> +    spl_length = spl_length_u32 * 4;
     >>> +    /* load crc */
     >>> +    spl_crc = *(const u32 *)(spl_start + spl_length);
     >>> +    /* undo xor */
     >>> +    spl_crc ^= 0xffffffff;
     >>> +    /* add contents of modified datastart register */
     >>> +    spl_crc = socfpga_boot_crc(spl_crc, (const u8
*)&spl_start, 4);
     >>> +    /* finalize */
     >>> +    spl_crc ^= 0xffffffff;
     >>> +
     >>> +    writel(0xae9efebc,
&sysmgr_regs->romcodegrp_warmramgrp_enable);
     >>> +    writel(spl_start_16,
     >>> &sysmgr_regs->romcodegrp_warmramgrp_datastart);
     >>> +    writel(spl_length,
&sysmgr_regs->romcodegrp_warmramgrp_length);
     >>> +    writel(spl_crc, &sysmgr_regs->romcodegrp_warmramgrp_crc);
     >>> +}
     >>> +
     >>>   void board_init_f(ulong dummy)
     >>>   {
     >>>       const struct cm_config *cm_default_cfg =
     cm_get_default_config();
     >>> @@ -82,6 +152,9 @@ void board_init_f(ulong dummy)
     >>>           writel(SYSMGR_ECC_OCRAM_DERR  | SYSMGR_ECC_OCRAM_EN,
     >>>                  &sysmgr_regs->eccgrp_ocram);
     >>>   +    if (!socfpga_is_booting_from_fpga())
     >>> +        spl_init_reboot_config();
     >>> +
     >>>       memset(__bss_start, 0, __bss_end - __bss_start);
     >>>         socfpga_sdram_remap_zero();
     >>>
     >> Can't we use the library CRC32 function instead ?
     >
     > No, unfortunately, it's bit-reversed. Plus it uses a table,
which
...
...
...
 > increases the SPL binary by more than 2 KByte.

 Are you sure ? The uImage code also uses crc32, so I suspect

that crc32
     stuff is already in SPL. And the bit operation can probably be
easily
     done. I might be wrong ...
I wrote that as a result of testing it. The binary grew by 2k+. I'll
have to check the uimage crc.
That's probably a good idea.
...
Bit reversing can be done, yes. I tested that too. It's a rarther small
function that could be added to some lib code (if it doesn't already
exist, I haven't checked).
If we can reuse the CRC code, that'd be awesome.
Ok, so I retested this and the result came as a bit of a shock to me.
Binary size does increase by ~1.3kByte (not 2k+ as I wrote before) when
using lib/crc32.c to do the CRC check (with private code for reversing
bits that works). This is 1K for the CRC table plus some code. That's
probably ok, but:
Thanks!
...
The shock was that I thought lib/crc32.c is what is used to check uImage
CRC and we shouldn't see any code size increasement. Turns out that none
of the files in common/spl (core or loaders) check the CRC of a uImage!
(Even with CONFIG_SPL_LOAD_FIT, the crc32 code isn't included.)
Uh, how does the signature verification work then ? I guess it at least
includes the SHA support ?
...
Unless I'm mistaken, this means that SPL does not check the validity of
a U-Boot image at all (unless using FIT and enabling
CONFIG_SPL_FIT_SIGNATURE, but this does not fit into 64k for socfpga
gen5).
Yikes
...
Is this a known issue? Has it always been like this? Why do we have a 2
CRCs in the U-Boot image then?
The bootm command should check it. I was not aware SPL doesn't check it
and I don't believe that was intended, but I might be wrong. Can you
investigate ? (I'm pushing more things unto you since I'm under a lot of
pressure recently, too much stuff to do, and I have quite a bit of
confidence in you, in that you can figure it out)
Adding that CRC check to SPL is no big deal, I already did that to see
booting does fail when I invalidate the image.
The thing I am concerned about is size increasement...
I'll send a patch.
Simon
...