From patchwork Sat May 9 18:34:04 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Wildt X-Patchwork-Id: 245384 List-Id: U-Boot discussion From: patrick at blueri.se (Patrick Wildt) Date: Sat, 9 May 2020 20:34:04 +0200 Subject: spl: allow board_spl_fit_post_load() to fail In-Reply-To: <0e0c885d-dbcc-d7ac-4b98-f123de9d2894@gmx.de> References: <20200509161328.GA6201@nox.fritz.box> <6fb21594-fd58-93f0-986b-7cb0d4eb11d2@gmx.de> <20200509174557.GA6261@nox.fritz.box> <0e0c885d-dbcc-d7ac-4b98-f123de9d2894@gmx.de> Message-ID: <20200509183404.GA6495@nox.fritz.box> On Sat, May 09, 2020 at 08:09:39PM +0200, Heinrich Schuchardt wrote: > On 5/9/20 7:45 PM, Patrick Wildt wrote: > > On Sat, May 09, 2020 at 06:38:33PM +0200, Heinrich Schuchardt wrote: > >> On 5/9/20 6:13 PM, Patrick Wildt wrote: > >>> On i.MX platforms board_spl_fit_post_load() can check the loaded > >>> SPL image for authenticity using its HAB engine. U-Boot's SPL > >>> mechanism allows booting images from other sources as well, but > >>> in the current setup the SPL would just hang if it encounters an > >>> image that does not pass scrutiny. Allowing the function to return > >>> an error, allows the SPL to try booting from another source as a > >>> fallback instead of ending up as a brick. > >>> > >>> Signed-off-by: Patrick Wildt > >> > >> Could an intruder abuse this by destroying a signed image and providing > >> an unsigned image on a source under his control? > >> > >> Best regards > >> > >> Heinrich > > > > Sure, let's think about it here. Maybe you have some more thoughts to > > add. > > > > First of all, the SPL goes through all the boot devices, and if there's > > none to find with an image, it will hang. It will hang like it does > > before the diff. The only difference is that it tries additional > > sources before hanging. Thus the attacker, unless he can exploit it > > in his first trial, or is able to force a reset, must have some access > > to reset the machine to have it boot and try again. This seems like he > > must have some kind of local or remote phyiscal access. > > > > Let's assume the image is on the network or on another remote medium. > > Then I guess the attacker will just try to attack that medium, and the > > alternate boot sources won't make a difference. > > > > I guess that means we should focus on local sources. I think we can > > also ignore a removable SD card, since he can just put in another one > > and try again. > > > > So maybe let's think about SPI flash and eMMC, soldered on, not directly > > accessible. If he has physical access, I guess he could open up the box > > and desolder a few pins, or add some voltage here and there to try and > > disrupt the bootup. But, then maybe it's easier to just desolder the > > whole SPI/eMMC and add his own. > > > > But what if he doesn't have that access? If he's remote? Ok, he will > > probably have to exploit the daemon (webserver or whatever) to gain some > > code execution. Then he'll try and become root, so he can access the > > disks. Then I figure he'll try and overwrite or remove the image. > > With this, on the next reboot it will (hopefully) fail to boot, unless > > he already has an exploit, then my patch won't make a difference. > > > > I figure the real issue could be that when the attacker has physical > > access, manages to remove/replace the image with a fallback to load from > > a device like an SD card, that it's now easier for him to try and find > > an exploit. > > This last scenario is what I was thinking of. > > So if HAB is used it may be ok to search all devices for signed images > but non-signed images should be rejected. Is that given with your patch? > > Best regards > > Heinrich I think you have a very good point there. No, with that diff I think there's an issue, but I have added a second diff here. I'll explain. i.MX's jump_to_image_no_args() has a check: if (spl_image->flags & SPL_FIT_FOUND) { image_entry(); } else { [...] if (!imx_hab_authenticate_image(spl_image->load_addr, That means that it will jump right away if the flags say SPL_FIT_FOUND. Unfortunately my diff sets that flag before returning an error, and that means if the fallback tried to boot a non-FIT, it will skip the HAB check and jump right away. I have also not (yet) seen code that resets the flag. I figure we should just set the flag after doing the post load. Since it's a "fit_post_load"-function, we can assume that that function does not depend on the SPL_FIT_FOUND flag to do its work. I mean, it's only called because it parses a FIT in the first place. Thanks for making me think about this again! Best regards, Patrick diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index fd3fa04600..b8f6fcb4df 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -311,7 +311,7 @@ ulong board_spl_fit_size_align(ulong size) return size; } -void board_spl_fit_post_load(ulong load_addr, size_t length) +int board_spl_fit_post_load(ulong load_addr, size_t length) { u32 offset = length - CONFIG_CSF_SIZE; @@ -319,8 +319,10 @@ void board_spl_fit_post_load(ulong load_addr, size_t length) offset + IVT_SIZE + CSF_PAD_SIZE, offset)) { puts("spl: ERROR: image authentication unsuccessful\n"); - hang(); + return -1; } + + return 0; } #endif diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index c51e4beb1c..7e9aff9240 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -24,8 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_SYS_BOOTM_LEN (64 << 20) #endif -__weak void board_spl_fit_post_load(ulong load_addr, size_t length) +__weak int board_spl_fit_post_load(ulong load_addr, size_t length) { + return 0; } __weak ulong board_spl_fit_size_align(ulong size) @@ -675,11 +676,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (spl_image->entry_point == FDT_ERROR || spl_image->entry_point == 0) spl_image->entry_point = spl_image->load_addr; - spl_image->flags |= SPL_FIT_FOUND; - #ifdef CONFIG_IMX_HAB - board_spl_fit_post_load((ulong)fit, size); + ret = board_spl_fit_post_load((ulong)fit, size); + if (ret) + return ret; #endif + spl_image->flags |= SPL_FIT_FOUND; return 0; } diff --git a/include/spl.h b/include/spl.h index 6bf9fd8beb..93d5a5a1f3 100644 --- a/include/spl.h +++ b/include/spl.h @@ -560,7 +560,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image, * board_spl_fit_post_load - allow process images after loading finished * */ -void board_spl_fit_post_load(ulong load_addr, size_t length); +int board_spl_fit_post_load(ulong load_addr, size_t length); /** * board_spl_fit_size_align - specific size align before processing payload