[PATCH v2] drivers: crypto: mod_exp_sw: Re-add DM_FLAG_PRE_RELOC

[Heinrich Schuchardt]

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 <jan.kiszka at siemens.com>:
>>>>>> 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 <jan.kiszka at siemens.com>
>>>>>>>>>
>>>>>>>>> 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 <xypron.glpk at gmx.de>
>>>>>>>>> CC: Marek Vasut <marex at denx.de>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
>>>>>>>>
>>>>>>>> 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.

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

Best regards

Heinrich