| Message ID | 1513144986-13619-1-git-send-email-jagan@amarulasolutions.com |
|---|---|
| State | Deferred |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,v3,1/5] sunxi: a64: Enable FIT Signature | expand |
On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: > From: Jagan Teki <jagannadh.teki@gmail.com> > > Enable FIT_SIGNATURE for sunxi a64. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes for v3: > - Move imply outside block > Changes for v2: > - Use imply instead of select > > arch/arm/mach-sunxi/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > index 1fededd..05e2d47 100644 > --- a/arch/arm/mach-sunxi/Kconfig > +++ b/arch/arm/mach-sunxi/Kconfig > @@ -179,6 +179,7 @@ config MACH_SUN50I > select SUNXI_DRAM_DW_32BIT > select FIT > select SPL_LOAD_FIT > + imply FIT_SIGNATURE I'm really not sure we should force it by default. How much code size is it adding? Maxime
>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes: Hi, >> +++ b/arch/arm/mach-sunxi/Kconfig >> @@ -179,6 +179,7 @@ config MACH_SUN50I >> select SUNXI_DRAM_DW_32BIT >> select FIT >> select SPL_LOAD_FIT >> + imply FIT_SIGNATURE > I'm really not sure we should force it by default. How much code size > is it adding? Not only that, enabling FIT_SIGNATURE support disables support for the legacy images (uImage / boot.scr) for security reasons, so it may very well break existing setups.
On Wed, Dec 13, 2017 at 9:38 PM, Peter Korsgaard <peter@korsgaard.com> wrote: >>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes: > > Hi, > > >> +++ b/arch/arm/mach-sunxi/Kconfig > >> @@ -179,6 +179,7 @@ config MACH_SUN50I > >> select SUNXI_DRAM_DW_32BIT > >> select FIT > >> select SPL_LOAD_FIT > >> + imply FIT_SIGNATURE > > > I'm really not sure we should force it by default. How much code size > > is it adding? > > Not only that, enabling FIT_SIGNATURE support disables support for the > legacy images (uImage / boot.scr) for security reasons, so it may very > well break existing setups. Sorry, I didn't get why legacy images will disable with fit? It depends how user can go with it, whether he want legacy or fit both will work. thanks!
On Wed, Dec 13, 2017 at 9:09 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: >> From: Jagan Teki <jagannadh.teki@gmail.com> >> >> Enable FIT_SIGNATURE for sunxi a64. >> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >> --- >> Changes for v3: >> - Move imply outside block >> Changes for v2: >> - Use imply instead of select >> >> arch/arm/mach-sunxi/Kconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >> index 1fededd..05e2d47 100644 >> --- a/arch/arm/mach-sunxi/Kconfig >> +++ b/arch/arm/mach-sunxi/Kconfig >> @@ -179,6 +179,7 @@ config MACH_SUN50I >> select SUNXI_DRAM_DW_32BIT >> select FIT >> select SPL_LOAD_FIT >> + imply FIT_SIGNATURE > > I'm really not sure we should force it by default. How much code size > is it adding? Why we need to consider u-boot size? (because it may cross the loader2 size?) Here is the delta of u-boot elf before: text data bss dec hex filename 457378 20248 244160 721786 b037a u-boot after: text data bss dec hex filename 461357 20848 244160 726365 b155d u-boot
>>>>> "Jagan" == Jagan Teki <jagannadh.teki@gmail.com> writes: > On Wed, Dec 13, 2017 at 9:38 PM, Peter Korsgaard <peter@korsgaard.com> wrote: >>>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes: >> >> Hi, >> >> >> +++ b/arch/arm/mach-sunxi/Kconfig >> >> @@ -179,6 +179,7 @@ config MACH_SUN50I >> >> select SUNXI_DRAM_DW_32BIT >> >> select FIT >> >> select SPL_LOAD_FIT >> >> + imply FIT_SIGNATURE >> >> > I'm really not sure we should force it by default. How much code size >> > is it adding? >> >> Not only that, enabling FIT_SIGNATURE support disables support for the >> legacy images (uImage / boot.scr) for security reasons, so it may very >> well break existing setups. > Sorry, I didn't get why legacy images will disable with fit? It > depends how user can go with it, whether he want legacy or fit both > will work. Not when you enable CONFIG_FIT_SIGNATURE. That option disables support for the legacy images.
On Thu, Dec 14, 2017 at 02:03:12PM +0530, Jagan Teki wrote: > On Wed, Dec 13, 2017 at 9:09 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: > >> From: Jagan Teki <jagannadh.teki@gmail.com> > >> > >> Enable FIT_SIGNATURE for sunxi a64. > >> > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >> --- > >> Changes for v3: > >> - Move imply outside block > >> Changes for v2: > >> - Use imply instead of select > >> > >> arch/arm/mach-sunxi/Kconfig | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > >> index 1fededd..05e2d47 100644 > >> --- a/arch/arm/mach-sunxi/Kconfig > >> +++ b/arch/arm/mach-sunxi/Kconfig > >> @@ -179,6 +179,7 @@ config MACH_SUN50I > >> select SUNXI_DRAM_DW_32BIT > >> select FIT > >> select SPL_LOAD_FIT > >> + imply FIT_SIGNATURE > > > > I'm really not sure we should force it by default. How much code size > > is it adding? > > Why we need to consider u-boot size? (because it may cross the loader2 size?) > Here is the delta of u-boot elf The same reason than anything else on our arm64 builds lately: we have a u-boot binary too big for the size compared to our environment offset. Maxime
Hi, On 15/12/17 13:41, Maxime Ripard wrote: > On Thu, Dec 14, 2017 at 02:03:12PM +0530, Jagan Teki wrote: >> On Wed, Dec 13, 2017 at 9:09 PM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >>> On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: >>>> From: Jagan Teki <jagannadh.teki@gmail.com> >>>> >>>> Enable FIT_SIGNATURE for sunxi a64. >>>> >>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>> --- >>>> Changes for v3: >>>> - Move imply outside block >>>> Changes for v2: >>>> - Use imply instead of select >>>> >>>> arch/arm/mach-sunxi/Kconfig | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >>>> index 1fededd..05e2d47 100644 >>>> --- a/arch/arm/mach-sunxi/Kconfig >>>> +++ b/arch/arm/mach-sunxi/Kconfig >>>> @@ -179,6 +179,7 @@ config MACH_SUN50I >>>> select SUNXI_DRAM_DW_32BIT >>>> select FIT >>>> select SPL_LOAD_FIT >>>> + imply FIT_SIGNATURE >>> >>> I'm really not sure we should force it by default. How much code size >>> is it adding? >> >> Why we need to consider u-boot size? (because it may cross the loader2 size?) >> Here is the delta of u-boot elf > > The same reason than anything else on our arm64 builds lately: we have > a u-boot binary too big for the size compared to our environment offset. I agree, and aside from that I don't see how this is useful: - We don't *need* this for Allwinner boards. - It is not usable without some more setup (which that other doc patch describes). - As Maxime mentioned, this is not very helpful on it's own, due to it inherent vulnerability without a protected SPL as well. - No other boards seems to set FIT_SIGNATURE. So you can happily add this to your private config file (overlay), where you create U-Boot binaries and pipe them to some encryption chain afterwards. But I don't think it belongs into the mainline Kconfig. Cheers, Andre.
On Fri, Dec 15, 2017 at 7:11 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Dec 14, 2017 at 02:03:12PM +0530, Jagan Teki wrote: >> On Wed, Dec 13, 2017 at 9:09 PM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: >> >> From: Jagan Teki <jagannadh.teki@gmail.com> >> >> >> >> Enable FIT_SIGNATURE for sunxi a64. >> >> >> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >> >> --- >> >> Changes for v3: >> >> - Move imply outside block >> >> Changes for v2: >> >> - Use imply instead of select >> >> >> >> arch/arm/mach-sunxi/Kconfig | 1 + >> >> 1 file changed, 1 insertion(+) >> >> >> >> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >> >> index 1fededd..05e2d47 100644 >> >> --- a/arch/arm/mach-sunxi/Kconfig >> >> +++ b/arch/arm/mach-sunxi/Kconfig >> >> @@ -179,6 +179,7 @@ config MACH_SUN50I >> >> select SUNXI_DRAM_DW_32BIT >> >> select FIT >> >> select SPL_LOAD_FIT >> >> + imply FIT_SIGNATURE >> > >> > I'm really not sure we should force it by default. How much code size >> > is it adding? >> >> Why we need to consider u-boot size? (because it may cross the loader2 size?) >> Here is the delta of u-boot elf > > The same reason than anything else on our arm64 builds lately: we have > a u-boot binary too big for the size compared to our environment offset. Was that the env size is the show-stopper for adding new feature? since U-Boot proper runs on DRAM env's are those can be update, I'm not saying based on this FIT in general for new features. thanks!
On Fri, Dec 15, 2017 at 8:06 PM, Andre Przywara <andre.przywara@arm.com> wrote: > Hi, > > On 15/12/17 13:41, Maxime Ripard wrote: >> On Thu, Dec 14, 2017 at 02:03:12PM +0530, Jagan Teki wrote: >>> On Wed, Dec 13, 2017 at 9:09 PM, Maxime Ripard >>> <maxime.ripard@free-electrons.com> wrote: >>>> On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: >>>>> From: Jagan Teki <jagannadh.teki@gmail.com> >>>>> >>>>> Enable FIT_SIGNATURE for sunxi a64. >>>>> >>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>>> --- >>>>> Changes for v3: >>>>> - Move imply outside block >>>>> Changes for v2: >>>>> - Use imply instead of select >>>>> >>>>> arch/arm/mach-sunxi/Kconfig | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >>>>> index 1fededd..05e2d47 100644 >>>>> --- a/arch/arm/mach-sunxi/Kconfig >>>>> +++ b/arch/arm/mach-sunxi/Kconfig >>>>> @@ -179,6 +179,7 @@ config MACH_SUN50I >>>>> select SUNXI_DRAM_DW_32BIT >>>>> select FIT >>>>> select SPL_LOAD_FIT >>>>> + imply FIT_SIGNATURE >>>> >>>> I'm really not sure we should force it by default. How much code size >>>> is it adding? >>> >>> Why we need to consider u-boot size? (because it may cross the loader2 size?) >>> Here is the delta of u-boot elf >> >> The same reason than anything else on our arm64 builds lately: we have >> a u-boot binary too big for the size compared to our environment offset. > > I agree, and aside from that I don't see how this is useful: > - We don't *need* this for Allwinner boards. why? can you elaborate? > - It is not usable without some more setup (which that other doc patch > describes). doc patch is rejected since we have redundant docs on the same topic. > - As Maxime mentioned, this is not very helpful on it's own, due to it > inherent vulnerability without a protected SPL as well. > - No other boards seems to set FIT_SIGNATURE. I'm mentioning this again, please check the other platforms as well this is verified-boot not secure-boot, other platforms will do use same. thanks!
Hi, On 15/12/17 15:05, Jagan Teki wrote: > On Fri, Dec 15, 2017 at 8:06 PM, Andre Przywara <andre.przywara@arm.com> wrote: >> Hi, >> >> On 15/12/17 13:41, Maxime Ripard wrote: >>> On Thu, Dec 14, 2017 at 02:03:12PM +0530, Jagan Teki wrote: >>>> On Wed, Dec 13, 2017 at 9:09 PM, Maxime Ripard >>>> <maxime.ripard@free-electrons.com> wrote: >>>>> On Wed, Dec 13, 2017 at 11:33:02AM +0530, Jagan Teki wrote: >>>>>> From: Jagan Teki <jagannadh.teki@gmail.com> >>>>>> >>>>>> Enable FIT_SIGNATURE for sunxi a64. >>>>>> >>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>>>> --- >>>>>> Changes for v3: >>>>>> - Move imply outside block >>>>>> Changes for v2: >>>>>> - Use imply instead of select >>>>>> >>>>>> arch/arm/mach-sunxi/Kconfig | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >>>>>> index 1fededd..05e2d47 100644 >>>>>> --- a/arch/arm/mach-sunxi/Kconfig >>>>>> +++ b/arch/arm/mach-sunxi/Kconfig >>>>>> @@ -179,6 +179,7 @@ config MACH_SUN50I >>>>>> select SUNXI_DRAM_DW_32BIT >>>>>> select FIT >>>>>> select SPL_LOAD_FIT >>>>>> + imply FIT_SIGNATURE >>>>> >>>>> I'm really not sure we should force it by default. How much code size >>>>> is it adding? >>>> >>>> Why we need to consider u-boot size? (because it may cross the loader2 size?) >>>> Here is the delta of u-boot elf >>> >>> The same reason than anything else on our arm64 builds lately: we have >>> a u-boot binary too big for the size compared to our environment offset. >> >> I agree, and aside from that I don't see how this is useful: >> - We don't *need* this for Allwinner boards. > > why? can you elaborate? I guess *you* have to present why we would need it. Fact is it happily runs without it, it's just some optional feature. Most users which rely on the defconfig don't have a need for this. If someone wants to use it, enabling this is the config is their least problem. >> - It is not usable without some more setup (which that other doc patch >> describes). > > doc patch is rejected since we have redundant docs on the same topic. I know ;-) As mentioned above, for this feature to be useful you have to go through quite some other steps, so there is no reason for it to be enabled by default. >> - As Maxime mentioned, this is not very helpful on it's own, due to it >> inherent vulnerability without a protected SPL as well. >> - No other boards seems to set FIT_SIGNATURE. > > I'm mentioning this again, please check the other platforms as well > this is verified-boot not secure-boot, other platforms will do use > same. Sure, but none of them sets this unconditionally in their Kconfig. And given that most users are totally happy with an unsigned firmware for their Allwinner board ... You could market it as some upsell of your product ;-) Cheers, Andre.
diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index 1fededd..05e2d47 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -179,6 +179,7 @@ config MACH_SUN50I select SUNXI_DRAM_DW_32BIT select FIT select SPL_LOAD_FIT + imply FIT_SIGNATURE config MACH_SUN50I_H5 bool "sun50i (Allwinner H5)"