From patchwork Sun May 31 15:34:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heinrich Schuchardt X-Patchwork-Id: 246892 List-Id: U-Boot discussion From: xypron.glpk at gmx.de (Heinrich Schuchardt) Date: Sun, 31 May 2020 17:34:45 +0200 Subject: [PATCH v2] drivers: crypto: mod_exp_sw: Re-add DM_FLAG_PRE_RELOC In-Reply-To: <737f4d7e-7b19-90b4-507b-f24476b18f52@gmx.de> References: <20200520122255.GP14425@bill-the-cat> <1723fde0-3805-1722-a5cd-9afa4a048e4d@gmx.de> <073af9fd-f375-8d27-18da-9eed6cb8a3f6@siemens.com> <98ACA8C4-BC39-42ED-8D8E-E14C9AFD7942@gmx.de> <48fba4fc-d7c9-e788-f4b7-83a81070ea23@siemens.com> <8e7ca531-8595-8de1-1f3f-7ea9e7b31f02@gmx.de> <1ff18668-f51b-0eb6-6708-2547c4a0b776@siemens.com> <737f4d7e-7b19-90b4-507b-f24476b18f52@gmx.de> Message-ID: <6804b435-c70d-32a1-b66c-1461f03aa063@gmx.de> On 5/22/20 8:12 PM, Heinrich Schuchardt wrote: > On 5/22/20 5:21 PM, Jan Kiszka wrote: >> On 22.05.20 16:55, Heinrich Schuchardt wrote: >>> On 22.05.20 14:21, Jan Kiszka wrote: >>>> On 22.05.20 13:38, Heinrich Schuchardt wrote: >>>>> Am May 22, 2020 10:50:29 AM UTC schrieb Jan Kiszka : >>>>>> On 22.05.20 12:42, Heinrich Schuchardt wrote: >>>>>>> On 5/20/20 2:22 PM, Tom Rini wrote: >>>>>>>> On Thu, May 07, 2020 at 08:36:03PM +0200, Jan Kiszka wrote: >>>>>>>> >>>>>>>>> From: Jan Kiszka >>>>>>>>> >>>>>>>>> This driver is safe to use in SPL without relocation. Denying >>>>>>>>> DM_FLAG_PRE_RELOC prevents its usability for verifying the main >>>>>> U-Boot >>>>>>>>> or other artifacts from the SPL unless needless enabling the full >>>>>> driver >>>>>>>>> set (SPL_OF_PLATDATA). >>>>>>>>> >>>>>>>>> Fixes: 17e117408571 ("drivers: crypto: rsa_mod_exp: avoid >>>>>> DM_FLAG_PRE_RELOC") >>>>>>>>> CC: Heinrich Schuchardt >>>>>>>>> CC: Marek Vasut >>>>>>>>> Signed-off-by: Jan Kiszka >>>>>>>> >>>>>>>> Applied to u-boot/master, thanks! >>>>>>>> >>>>>>> >>>>>>> With this patch applied pine64-lts_defconfig with CONFIG_RSA=y does >>>>>> not >>>>>>> boot anymore. See the output below. So something is wrong with this >>>>>> driver. >>>>>>> >>>>>>> Do you have an idea how to analyze what is wrong? Unfortunately there >>>>>> is >>>>>>> no DEBUG_UART available on the Pine A64 LTS board. >>>>>> >>>>>> I would start crippling it down until things start to boot again. Are >>>>>> you using it (for image verification e.g.), or is this just the >>>>>> registration that breaks already? >>>>>> >>>>> >>>>> >>>>> RSA is needed in the UEFI subsystem for verifying variables and images. But there is no need in SPL for it at all. >>>>> >>>>> In my configuration RSA is not used at all. Something breaks before even the console becomes available. >>>>> >>>>> The pine64-lts_defconfig board boots via SPL->BL31->U-Boot >>>> >>>> But then a workaround for you would be to turn this driver off in SPL. >>>> UEFI is main U-Boot only, isn't it? >>>> >>>> That said, understanding the reason for the breakage would still be nice >>>> for the case someone needs to validate what SPL loads with the help of >>>> RSA (which is the case for us on an AM65x board). >>>> >>>> Jan >>>> >>> As I described above I did *not* select RSA_SPL. The breakage is in main >>> U-boot. SPL works fine loading TF-A BL31 which in turn loads U-Boot. But >>> during driver initialization U-Boot does not even reach the point where >>> we have a console due to something wrong with DM_FLAG_PRE_RELOC. >>> >> >> Sorry, missed that detail. But that is indeed weird because - to my >> understanding - this driver should be totally passive until someone >> calls rsa_mod_exp() (which should only happen late, when UEFI comes into >> play). >> >> Can you validate that this function is not involved by emptying its body? >> >> Also, until everything is understood, we could limit DM_FLAG_PRE_RELOC >> to BUILD_SPL. >> >> Jan >> > > Disabling the only consumer of UCLASS_MOD_EXP does not help. > > diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c > index 1d55b997e3..89a08274f2 100644 > --- a/lib/rsa/rsa-verify.c > +++ b/lib/rsa/rsa-verify.c > @@ -334,7 +334,8 @@ static int rsa_verify_key(struct image_sign_info *info, > hash_len = checksum->checksum_len; > > #if !defined(USE_HOSTCC) > - ret = uclass_get_device(UCLASS_MOD_EXP, 0, &mod_exp_dev); > + //ret = uclass_get_device(UCLASS_MOD_EXP, 0, &mod_exp_dev); > + ret = 1; > > Emptying function mod_exp_sw() does not help. > > The board boots if I rename the driver: > > U_BOOT_DRIVER(mod_exp_sw) = { > - .name = "mod_exp_sw", > + .name = "mod_exp_sw1", > > So it seems the very act of loading the driver before relocation is > enough to cause the problem. > > To be more specific, if > > ??????? ret = uclass_get(drv->id, &uc); > > is called in device_bind_common() - drivers/core/device.c for name = > "mod_exp_sw" booting fails. > > This does not boot: > > ret = uclass_get(drv->id, &uc); > + if (!strcmp(name, "mod_exp_sw")) > + return -ENOENT; > > This does boot: > > + if (!strcmp(name, "mod_exp_sw")) > + return -ENOENT; > ret = uclass_get(drv->id, &uc); > > So I tried to look into uclass_get(): > > If > > uc = calloc(1, sizeof(*uc)); > > is executed in uclass_add() for UCLASS_MOD_EXP booting fails. > > This does not boot: > > uc = calloc(1, sizeof(*uc)); > if (!uc) > return -ENOMEM; > + if (id == UCLASS_MOD_EXP) > + return -ENOENT; > > This boots: > > + if (id == UCLASS_MOD_EXP) > + return -ENOENT; > uc = calloc(1, sizeof(*uc)); > > Enlarging CONFIG_SYS_MALLOC_F_LEN from 0x400 to 0x8000 does not help. > > Best regards > > Heinrich > What finally helps is CONFIG_INIT_SP_RELATIVE=y. Best regards Heinrich diff --git a/configs/pine64-lts_defconfig b/configs/pine64-lts_defconfig index ef108a1a31..b03bef01b1 100644 --- a/configs/pine64-lts_defconfig +++ b/configs/pine64-lts_defconfig @@ -1,5 +1,8 @@ CONFIG_ARM=y +CONFIG_INIT_SP_RELATIVE=y CONFIG_ARCH_SUNXI=y +CONFIG_SYS_MALLOC_F_LEN=0x8000 +CONSPL_SYS_MALLOC_F_LEN=FIG_0x400 CONFIG_SPL=y CONFIG_MACH_SUN50I=y CONFIG_SUNXI_DRAM_LPDDR3_STOCK=y