diff mbox

[U-Boot] RSA depends on DM

Message ID CAGHP0pKkfmbbUawc4b7m2eZZEKaBdsfAHxKD=PoGyz428EO3pA@mail.gmail.com
State Superseded
Headers show

Commit Message

Chris Kuethe Feb. 3, 2015, 7:42 a.m. UTC
Discovered while experimenting with signature checking on vexpress
which doesn't typically use DM. Rather than complaining about unmet
dependencies it might be better to enable those them.

---
 lib/rsa/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Comments

Simon Glass Feb. 4, 2015, 12:38 a.m. UTC | #1
+Masahiro

Hi Chris,

On 3 February 2015 at 00:42, Chris Kuethe <chris.kuethe@gmail.com> wrote:
> Discovered while experimenting with signature checking on vexpress
> which doesn't typically use DM. Rather than complaining about unmet
> dependencies it might be better to enable those them.
>
> ---
>  lib/rsa/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> index 1268a1b..4db5da4 100644
> --- a/lib/rsa/Kconfig
> +++ b/lib/rsa/Kconfig
> @@ -2,6 +2,7 @@ config RSA
>   bool "Use RSA Library"
>   select RSA_FREESCALE_EXP if FSL_CAAM
>   select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
> + select DM
>   help
>    RSA support. This enables the RSA algorithm used for FIT image
>    verification in U-Boot.

I wonder whether 'depends on DM' might be better? It seems odd to have
the tail wagging the dog.

Regards,
Simon
Chris Kuethe Feb. 4, 2015, 12:57 a.m. UTC | #2
On Tue, Feb 3, 2015 at 4:38 PM, Simon Glass <sjg@chromium.org> wrote:
> +Masahiro
>
> Hi Chris,
>
> On 3 February 2015 at 00:42, Chris Kuethe <chris.kuethe@gmail.com> wrote:
>> Discovered while experimenting with signature checking on vexpress
>> which doesn't typically use DM. Rather than complaining about unmet
>> dependencies it might be better to enable those them.
>>
>> ---
>>  lib/rsa/Kconfig | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
>> index 1268a1b..4db5da4 100644
>> --- a/lib/rsa/Kconfig
>> +++ b/lib/rsa/Kconfig
>> @@ -2,6 +2,7 @@ config RSA
>>   bool "Use RSA Library"
>>   select RSA_FREESCALE_EXP if FSL_CAAM
>>   select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
>> + select DM
>>   help
>>    RSA support. This enables the RSA algorithm used for FIT image
>>    verification in U-Boot.
>
> I wonder whether 'depends on DM' might be better? It seems odd to have
> the tail wagging the dog.
>
> Regards,
> Simon

No, that would not be better because a few lines down,
RSA_SOFTWARE_EXP and RSA_FREESCALE_EXP both say "depends on DM" but
they don't actually enable it if they need it.

As a user, my expectation is that when I turn on some high level
feature, that will enable all of its lower level dependencies. Would
it be less strange to make FIT_SIGNATURE turn on DM instead of RSA?
Simon Glass Feb. 4, 2015, 1:01 a.m. UTC | #3
Hi Chris,

On 3 February 2015 at 17:57, Chris Kuethe <chris.kuethe@gmail.com> wrote:
> On Tue, Feb 3, 2015 at 4:38 PM, Simon Glass <sjg@chromium.org> wrote:
>> +Masahiro
>>
>> Hi Chris,
>>
>> On 3 February 2015 at 00:42, Chris Kuethe <chris.kuethe@gmail.com> wrote:
>>> Discovered while experimenting with signature checking on vexpress
>>> which doesn't typically use DM. Rather than complaining about unmet
>>> dependencies it might be better to enable those them.
>>>
>>> ---
>>>  lib/rsa/Kconfig | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
>>> index 1268a1b..4db5da4 100644
>>> --- a/lib/rsa/Kconfig
>>> +++ b/lib/rsa/Kconfig
>>> @@ -2,6 +2,7 @@ config RSA
>>>   bool "Use RSA Library"
>>>   select RSA_FREESCALE_EXP if FSL_CAAM
>>>   select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
>>> + select DM
>>>   help
>>>    RSA support. This enables the RSA algorithm used for FIT image
>>>    verification in U-Boot.
>>
>> I wonder whether 'depends on DM' might be better? It seems odd to have
>> the tail wagging the dog.
>>
>> Regards,
>> Simon
>
> No, that would not be better because a few lines down,
> RSA_SOFTWARE_EXP and RSA_FREESCALE_EXP both say "depends on DM" but
> they don't actually enable it if they need it.
>
> As a user, my expectation is that when I turn on some high level
> feature, that will enable all of its lower level dependencies. Would
> it be less strange to make FIT_SIGNATURE turn on DM instead of RSA?

We certainly must avoid the build break.

My concern is that CONFIG_DM may introduce a run-time break. For
example if you don't have pre-relocation malloc() available the board
may not boot. Driver model is a fundamental core feature, and we are
working to move everything over to it, but I'm not quite comfortable
with forcing it on when someone changes a feature. It feel it would be
better to not offer it.

I'm interested to hear other viewpoints though.

Perhaps soon we can enable CONFIG_DM globally but we are not there yet.

Regards,
Simon
Chris Kuethe Feb. 4, 2015, 1:12 a.m. UTC | #4
On Tue, Feb 3, 2015 at 5:01 PM, Simon Glass <sjg@chromium.org> wrote:
> We certainly must avoid the build break.
>
> My concern is that CONFIG_DM may introduce a run-time break. For
> example if you don't have pre-relocation malloc() available the board
> may not boot. Driver model is a fundamental core feature, and we are
> working to move everything over to it, but I'm not quite comfortable
> with forcing it on when someone changes a feature. It feel it would be
> better to not offer it.
>
> I'm interested to hear other viewpoints though.
>
> Perhaps soon we can enable CONFIG_DM globally but we are not there yet.


I appreciate the caution.

For now, vexpress works with qemu which means I can get back to
playing with verified boot. I haven't checked to see if it's possible
to make RSA not always require DM - I defer to those who know the code
better than I do.

-C
Robert Moskowitz Feb. 4, 2015, 5:41 a.m. UTC | #5
Of course there is the meta question of why RSA sig is still being used 
rather than ECDSA.

As a crypto plumber, I occationally wonder why we perpetuate need of 
large, slow RSA keys over ECC.  Perhaps the patent concerns even with 
RFC 6090.

I will shut up and let you to your important work of getting all this 
wonderful support working in uboot.

On 02/03/2015 08:01 PM, Simon Glass wrote:
> Hi Chris,
>
> On 3 February 2015 at 17:57, Chris Kuethe <chris.kuethe@gmail.com> wrote:
>> On Tue, Feb 3, 2015 at 4:38 PM, Simon Glass <sjg@chromium.org> wrote:
>>> +Masahiro
>>>
>>> Hi Chris,
>>>
>>> On 3 February 2015 at 00:42, Chris Kuethe <chris.kuethe@gmail.com> wrote:
>>>> Discovered while experimenting with signature checking on vexpress
>>>> which doesn't typically use DM. Rather than complaining about unmet
>>>> dependencies it might be better to enable those them.
>>>>
>>>> ---
>>>>   lib/rsa/Kconfig | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
>>>> index 1268a1b..4db5da4 100644
>>>> --- a/lib/rsa/Kconfig
>>>> +++ b/lib/rsa/Kconfig
>>>> @@ -2,6 +2,7 @@ config RSA
>>>>    bool "Use RSA Library"
>>>>    select RSA_FREESCALE_EXP if FSL_CAAM
>>>>    select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
>>>> + select DM
>>>>    help
>>>>     RSA support. This enables the RSA algorithm used for FIT image
>>>>     verification in U-Boot.
>>> I wonder whether 'depends on DM' might be better? It seems odd to have
>>> the tail wagging the dog.
>>>
>>> Regards,
>>> Simon
>> No, that would not be better because a few lines down,
>> RSA_SOFTWARE_EXP and RSA_FREESCALE_EXP both say "depends on DM" but
>> they don't actually enable it if they need it.
>>
>> As a user, my expectation is that when I turn on some high level
>> feature, that will enable all of its lower level dependencies. Would
>> it be less strange to make FIT_SIGNATURE turn on DM instead of RSA?
> We certainly must avoid the build break.
>
> My concern is that CONFIG_DM may introduce a run-time break. For
> example if you don't have pre-relocation malloc() available the board
> may not boot. Driver model is a fundamental core feature, and we are
> working to move everything over to it, but I'm not quite comfortable
> with forcing it on when someone changes a feature. It feel it would be
> better to not offer it.
>
> I'm interested to hear other viewpoints though.
>
> Perhaps soon we can enable CONFIG_DM globally but we are not there yet.
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Albert ARIBAUD Feb. 4, 2015, 7:47 a.m. UTC | #6
Hello Simon,

On Tue, 3 Feb 2015 18:01:49 -0700, Simon Glass <sjg@chromium.org> wrote:
> Hi Chris,
> 
> On 3 February 2015 at 17:57, Chris Kuethe <chris.kuethe@gmail.com> wrote:
> > On Tue, Feb 3, 2015 at 4:38 PM, Simon Glass <sjg@chromium.org> wrote:
> >> +Masahiro
> >>
> >> Hi Chris,
> >>
> >> On 3 February 2015 at 00:42, Chris Kuethe <chris.kuethe@gmail.com> wrote:
> >>> Discovered while experimenting with signature checking on vexpress
> >>> which doesn't typically use DM. Rather than complaining about unmet
> >>> dependencies it might be better to enable those them.
> >>>
> >>> ---
> >>>  lib/rsa/Kconfig | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> >>> index 1268a1b..4db5da4 100644
> >>> --- a/lib/rsa/Kconfig
> >>> +++ b/lib/rsa/Kconfig
> >>> @@ -2,6 +2,7 @@ config RSA
> >>>   bool "Use RSA Library"
> >>>   select RSA_FREESCALE_EXP if FSL_CAAM
> >>>   select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
> >>> + select DM
> >>>   help
> >>>    RSA support. This enables the RSA algorithm used for FIT image
> >>>    verification in U-Boot.
> >>
> >> I wonder whether 'depends on DM' might be better? It seems odd to have
> >> the tail wagging the dog.
> >>
> >> Regards,
> >> Simon
> >
> > No, that would not be better because a few lines down,
> > RSA_SOFTWARE_EXP and RSA_FREESCALE_EXP both say "depends on DM" but
> > they don't actually enable it if they need it.
> >
> > As a user, my expectation is that when I turn on some high level
> > feature, that will enable all of its lower level dependencies. Would
> > it be less strange to make FIT_SIGNATURE turn on DM instead of RSA?
> 
> We certainly must avoid the build break.
> 
> My concern is that CONFIG_DM may introduce a run-time break.

I can tell it does. :)

> For example if you don't have pre-relocation malloc() available the
> board may not boot. Driver model is a fundamental core feature, and we
> are working to move everything over to it, but I'm not quite comfortable
> with forcing it on when someone changes a feature. It feel it would be
> better to not offer it.
> 
> I'm interested to hear other viewpoints though.

Agreed for me: no board should have DM enabled 'behind its back'.

If RSA depends on DM, then the make menuconfig user should be unable to
select RSA unless and until (s)he has selected DM (and the RSA help
should make it clear that the board must support DM, and that just
enabling CONFIG_DM probably won't be enough).

> Perhaps soon we can enable CONFIG_DM globally but we are not there yet.
>
> Regards,
> Simon

Amicalement,
Simon Glass Feb. 4, 2015, 8:07 p.m. UTC | #7
Hi Chris,

On 4 February 2015 at 00:47, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> Hello Simon,
>
> On Tue, 3 Feb 2015 18:01:49 -0700, Simon Glass <sjg@chromium.org> wrote:
>> Hi Chris,
>>
>> On 3 February 2015 at 17:57, Chris Kuethe <chris.kuethe@gmail.com> wrote:
>> > On Tue, Feb 3, 2015 at 4:38 PM, Simon Glass <sjg@chromium.org> wrote:
>> >> +Masahiro
>> >>
>> >> Hi Chris,
>> >>
>> >> On 3 February 2015 at 00:42, Chris Kuethe <chris.kuethe@gmail.com> wrote:
>> >>> Discovered while experimenting with signature checking on vexpress
>> >>> which doesn't typically use DM. Rather than complaining about unmet
>> >>> dependencies it might be better to enable those them.
>> >>>
>> >>> ---
>> >>>  lib/rsa/Kconfig | 1 +
>> >>>  1 file changed, 1 insertion(+)
>> >>>
>> >>> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
>> >>> index 1268a1b..4db5da4 100644
>> >>> --- a/lib/rsa/Kconfig
>> >>> +++ b/lib/rsa/Kconfig
>> >>> @@ -2,6 +2,7 @@ config RSA
>> >>>   bool "Use RSA Library"
>> >>>   select RSA_FREESCALE_EXP if FSL_CAAM
>> >>>   select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
>> >>> + select DM
>> >>>   help
>> >>>    RSA support. This enables the RSA algorithm used for FIT image
>> >>>    verification in U-Boot.
>> >>
>> >> I wonder whether 'depends on DM' might be better? It seems odd to have
>> >> the tail wagging the dog.
>> >>
>> >> Regards,
>> >> Simon
>> >
>> > No, that would not be better because a few lines down,
>> > RSA_SOFTWARE_EXP and RSA_FREESCALE_EXP both say "depends on DM" but
>> > they don't actually enable it if they need it.
>> >
>> > As a user, my expectation is that when I turn on some high level
>> > feature, that will enable all of its lower level dependencies. Would
>> > it be less strange to make FIT_SIGNATURE turn on DM instead of RSA?
>>
>> We certainly must avoid the build break.
>>
>> My concern is that CONFIG_DM may introduce a run-time break.
>
> I can tell it does. :)
>
>> For example if you don't have pre-relocation malloc() available the
>> board may not boot. Driver model is a fundamental core feature, and we
>> are working to move everything over to it, but I'm not quite comfortable
>> with forcing it on when someone changes a feature. It feel it would be
>> better to not offer it.
>>
>> I'm interested to hear other viewpoints though.
>
> Agreed for me: no board should have DM enabled 'behind its back'.
>
> If RSA depends on DM, then the make menuconfig user should be unable to
> select RSA unless and until (s)he has selected DM (and the RSA help
> should make it clear that the board must support DM, and that just
> enabling CONFIG_DM probably won't be enough).
>
>> Perhaps soon we can enable CONFIG_DM globally but we are not there yet.

Can you please adjust your patch to depend on DM rather than select it?

It was me that requested that RSA should require DM, because we should
not be adding new driver frameworks that don't use DM.

Regards,
Simon
diff mbox

Patch

diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
index 1268a1b..4db5da4 100644
--- a/lib/rsa/Kconfig
+++ b/lib/rsa/Kconfig
@@ -2,6 +2,7 @@  config RSA
  bool "Use RSA Library"
  select RSA_FREESCALE_EXP if FSL_CAAM
  select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
+ select DM
  help
   RSA support. This enables the RSA algorithm used for FIT image
   verification in U-Boot.