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

[Jan Kiszka <jan.kiszka@siemens.com>]

On 31.05.20 17:34, Heinrich Schuchardt wrote:
> 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@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@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@gmx.de>
>>>>>>>>>> CC: Marek Vasut <marex@denx.de>
>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@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
> 

Good that this is resolved.

Is it a dependency that should be selected by the target or some other
option?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux