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

[Heinrich Schuchardt]

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