[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot
Simon Goldschmidt
simon.k.r.goldschmidt at gmail.com
Thu Nov 22 05:18:22 UTC 2018
Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut <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>> 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>>
> >>> >>>
> >>> >>> 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>>
> >>> >>> ---
> >>> >>>
> >>> >>> 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
>
More information about the U-Boot
mailing list