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

Marek Vasut marex at denx.de
Thu Nov 22 17:09:01 UTC 2018


On 11/22/2018 06:18 AM, Simon Goldschmidt wrote:
> 
> 
> Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut <marex at denx.de
> <mailto:marex at 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 at denx.de
>     <mailto:marex at denx.de>
>     >>> <mailto:marex at denx.de <mailto:marex at 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 at de.pepperl-fuchs.com
>     <mailto:sgoldschmidt at de.pepperl-fuchs.com>
>     >>>      <mailto:sgoldschmidt at de.pepperl-fuchs.com
>     <mailto:sgoldschmidt at 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 at gmail.com
>     <mailto:simon.k.r.goldschmidt at gmail.com>
>     >>>      <mailto:simon.k.r.goldschmidt at gmail.com
>     <mailto:simon.k.r.goldschmidt at 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...

If it doesn't break any boards, that's actually a massive improvement.

> I'll send a patch.

Thanks

-- 
Best regards,
Marek Vasut


More information about the U-Boot mailing list