diff mbox series

[U-Boot,3/3] imx: hab: Convert DCD non-NULL error to warning

Message ID 1520600841-8810-4-git-send-email-bryan.odonoghue@linaro.org
State Accepted
Commit ca89df7dd46f3f9c2d5cfee277ce8597937c6163
Delegated to: Tom Rini
Headers show
Series HAB Fixes for 2018.03-rc4 | expand

Commit Message

Bryan O'Donoghue March 9, 2018, 1:07 p.m. UTC
commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
to calling HAB authenticate function.") makes the DCD field being NULL a
dependency.

This change though will break loading and executing of existing pre-signed
binaries on a u-boot update i.e. if this change is deployed on a board you
will be forced to redo all images on that board to NULL out the DCD.

There is no prior guidance from NXP that the DCD must be NULL similarly
public guidance on usage of the HAB doesn't call out this NULL dependency
(see boundary devices link).

Since later SoCs will reject a non-NULL DCD there's no reason to make a
NULL DCD a requirement, however if there is an actual dependency for later
SoCs the appropriate fix would be to do SoC version checking.

Earlier SoCs are capable (and happy) to authenticate images with non-NULL
DCDs, we should not be forcing this change on downstream users -
particularly if it means those users now must rewrite their build systems
and/or redeploy signed images in the field.

Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
to calling HAB authenticate function.")

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Cc: Breno Lima <breno.lima@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Link: https://boundarydevices.com/high-assurance-boot-hab-dummies
---
 arch/arm/mach-imx/hab.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Fabio Estevam March 10, 2018, 12:48 a.m. UTC | #1
Hi Bryan,

On Fri, Mar 9, 2018 at 10:07 AM, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.") makes the DCD field being NULL a
> dependency.
>
> This change though will break loading and executing of existing pre-signed
> binaries on a u-boot update i.e. if this change is deployed on a board you
> will be forced to redo all images on that board to NULL out the DCD.
>
> There is no prior guidance from NXP that the DCD must be NULL similarly
> public guidance on usage of the HAB doesn't call out this NULL dependency
> (see boundary devices link).
>
> Since later SoCs will reject a non-NULL DCD there's no reason to make a
> NULL DCD a requirement, however if there is an actual dependency for later
> SoCs the appropriate fix would be to do SoC version checking.
>
> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
> DCDs, we should not be forcing this change on downstream users -
> particularly if it means those users now must rewrite their build systems
> and/or redeploy signed images in the field.
>
> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.")
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Link: https://boundarydevices.com/high-assurance-boot-hab-dummies

Utkarsh is currently out of the office, but Breno knows the details
about 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null
prior
to calling HAB authenticate function.") and will comment.

Thanks
Breno Matheus Lima March 10, 2018, 1:10 a.m. UTC | #2
Hi Bryan,

2018-03-09 10:07 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.") makes the DCD field being NULL a
> dependency.
>
> This change though will break loading and executing of existing pre-signed
> binaries on a u-boot update i.e. if this change is deployed on a board you
> will be forced to redo all images on that board to NULL out the DCD.
>
> There is no prior guidance from NXP that the DCD must be NULL similarly
> public guidance on usage of the HAB doesn't call out this NULL dependency
> (see boundary devices link).
>
> Since later SoCs will reject a non-NULL DCD there's no reason to make a
> NULL DCD a requirement, however if there is an actual dependency for later
> SoCs the appropriate fix would be to do SoC version checking.
>
> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
> DCDs, we should not be forcing this change on downstream users -
> particularly if it means those users now must rewrite their build systems
> and/or redeploy signed images in the field.
>
> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.")

We understand the concern being raised however would prefer to leave
this as an error, and selected users relying on images with DCD
pointers can modify the code accordingly. This does not just apply to
new SoC’s but also applies to existing SoC’s. Users performing such an
update should definitely test the image prior to making an OTA
available. It has never been intended for DCD to be used in any post
boot image and we realize the lack of documentation is a hindsight by
us, and we are currently addressing that with updated guidance.

Best regards,
Breno Lima
Stefano Babic March 11, 2018, 2:36 p.m. UTC | #3
Hi Everybody,

I have applied 1-2 as Fabio suggested. I have a couple of comments for
this, too:

On 10/03/2018 02:10, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> 2018-03-09 10:07 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
>> to calling HAB authenticate function.") makes the DCD field being NULL a
>> dependency.
>>
>> This change though will break loading and executing of existing pre-signed
>> binaries on a u-boot update i.e. if this change is deployed on a board you
>> will be forced to redo all images on that board to NULL out the DCD.
>>
>> There is no prior guidance from NXP that the DCD must be NULL similarly
>> public guidance on usage of the HAB doesn't call out this NULL dependency
>> (see boundary devices link).
>>
>> Since later SoCs will reject a non-NULL DCD there's no reason to make a
>> NULL DCD a requirement, however if there is an actual dependency for later
>> SoCs the appropriate fix would be to do SoC version checking.
>>
>> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
>> DCDs, we should not be forcing this change on downstream users -
>> particularly if it means those users now must rewrite their build systems
>> and/or redeploy signed images in the field.
>>
>> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
>> to calling HAB authenticate function.")
> 
> We understand the concern being raised however would prefer to leave
> this as an error, and selected users relying on images with DCD
> pointers can modify the code accordingly. This does not just apply to
> new SoC’s but also applies to existing SoC’s.

Anyway, this is not so easy to identify. What we current know is that
older SOCs have no problem if the pointer is not set to Null. I agree
with Brian that if some constraint is coming, it should be defined with
a version checking as we currently do for a lot of things (is_soc() to
be clear).

I am quite lost because I do not know which SOCs are affected and which
not. What does it hide under "later SOCSs" ? Or better which new version
of HAB ? I know HAB was updated due to other issues - are the
corresponding SOC involved ?

If a not-null DCD pointer affects just future SOCs, we should fix it
when these SOCs will be available. This means both new SOSs (imx8x) as
new version of supported SOCs (mx5 / mx6 / mx7).

> Users performing such an
> update should definitely test the image prior to making an OTA
> available.

I think this is another topic and not what Brian is trying to address. I
hope as you that *any* update is tested before deploying. But this is
unrelated.

> It has never been intended for DCD to be used in any post
> boot image and we realize the lack of documentation is a hindsight by
> us, and we are currently addressing that with updated guidance.
> 

ok, thanks for this, very appreciated.

Anyway, I am facing what we should do with this patch for 2018.03. As I
said, I am not forcing anyone and I have just picked up 1/3 and 2/3. But
IMHO, if there are not good reason to say that not raising an error
breaks a lot of supplied device, this should flow into 2018.03, too. And
then we get enough time to better explore this issue.

Best regards,
Stefano
Bryan O'Donoghue March 12, 2018, 12:26 p.m. UTC | #4
On 10/03/18 01:10, Breno Matheus Lima wrote:
> Hi Bryan,
> 
> 2018-03-09 10:07 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
>> to calling HAB authenticate function.") makes the DCD field being NULL a
>> dependency.
>>
>> This change though will break loading and executing of existing pre-signed
>> binaries on a u-boot update i.e. if this change is deployed on a board you
>> will be forced to redo all images on that board to NULL out the DCD.
>>
>> There is no prior guidance from NXP that the DCD must be NULL similarly
>> public guidance on usage of the HAB doesn't call out this NULL dependency
>> (see boundary devices link).
>>
>> Since later SoCs will reject a non-NULL DCD there's no reason to make a
>> NULL DCD a requirement, however if there is an actual dependency for later
>> SoCs the appropriate fix would be to do SoC version checking.
>>
>> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
>> DCDs, we should not be forcing this change on downstream users -
>> particularly if it means those users now must rewrite their build systems
>> and/or redeploy signed images in the field.
>>
>> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
>> to calling HAB authenticate function.")
> 
> It has never been intended for DCD to be used in any post
> boot image 

Breno,

There's extensive documentation from NXP in the CST docs detailing usage 
of the DCD by post 1st-stage images.

High Assurance Boot Version 4 Application Programming Interface 
Reference Manual version 2.3.2 section "3.3 Authenticate Image"

"Purpose:
This function combines _DCD_, CSF and Assert functions in a standard 
sequence in order to authenticate a loaded image. It is intended for use 
by post-ROM boot stage components, via the ROM Vector Table. Support for 
images partially loaded to an initial location is provided
via a callback function"

<snip>

"Postconditions:
The post-conditions of the functions hab_rvt.check_target(), 
_hab_rvt.run_dcd()_,hab_rvt.run_csf() and hab_rvt.assert() apply also to 
this function. In particular, any audit events logged within the given 
functions have the context field appropriate to that function rather 
than HAB_CTX_AUTHENTICATE. In addition, the side-effects and 
post-conditions of any callback function supplied apply."

More than that - there's even a BootROM API callback "section 3.4 Run DCD"

"3.4
Run DCD

hab_status_t(* hab_rvt::run_dcd)(const uint8_t *dcd)
Execute boot configuration script.

Purpose:
This function configures the IC based upon a Device Configuration Data 
table. It is intended for use by post-ROM boot stage components, via the 
ROM Vector Table.

This function may be invoked as often as required for each boot stage.
The difference between the configuration functionality in this function 
and hab_rvt.run_csf() arises because the Device Configuration Data table 
is not authenticated prior to running the commands. Hence, there is a 
more limited range of commands allowed, and a limited range of 
parameters to allowed commands."

I don't think its reasonable to go forcing people to NULL out the DCD 
(which is work for them - and forces a OTA updates) - let alone reading 
the docs now - people might even be _doing_ DCD things right now.

There's even a callback that allows you to run the DCD from u-boot !

By all means restrict on a per-SoC basis but that should be version 
checked and justified - particularly if there is a derogation from the 
official documentation that comes with the code-signing tools.

---
bod
Tom Rini March 12, 2018, 4:07 p.m. UTC | #5
On Sun, Mar 11, 2018 at 03:36:16PM +0100, Stefano Babic wrote:
> Hi Everybody,
> 
> I have applied 1-2 as Fabio suggested. I have a couple of comments for
> this, too:
> 
> On 10/03/2018 02:10, Breno Matheus Lima wrote:
> > Hi Bryan,
> > 
> > 2018-03-09 10:07 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> >> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> >> to calling HAB authenticate function.") makes the DCD field being NULL a
> >> dependency.
> >>
> >> This change though will break loading and executing of existing pre-signed
> >> binaries on a u-boot update i.e. if this change is deployed on a board you
> >> will be forced to redo all images on that board to NULL out the DCD.
> >>
> >> There is no prior guidance from NXP that the DCD must be NULL similarly
> >> public guidance on usage of the HAB doesn't call out this NULL dependency
> >> (see boundary devices link).
> >>
> >> Since later SoCs will reject a non-NULL DCD there's no reason to make a
> >> NULL DCD a requirement, however if there is an actual dependency for later
> >> SoCs the appropriate fix would be to do SoC version checking.
> >>
> >> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
> >> DCDs, we should not be forcing this change on downstream users -
> >> particularly if it means those users now must rewrite their build systems
> >> and/or redeploy signed images in the field.
> >>
> >> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> >> to calling HAB authenticate function.")
> > 
> > We understand the concern being raised however would prefer to leave
> > this as an error, and selected users relying on images with DCD
> > pointers can modify the code accordingly. This does not just apply to
> > new SoC’s but also applies to existing SoC’s.
> 
> Anyway, this is not so easy to identify. What we current know is that
> older SOCs have no problem if the pointer is not set to Null. I agree
> with Brian that if some constraint is coming, it should be defined with
> a version checking as we currently do for a lot of things (is_soc() to
> be clear).
> 
> I am quite lost because I do not know which SOCs are affected and which
> not. What does it hide under "later SOCSs" ? Or better which new version
> of HAB ? I know HAB was updated due to other issues - are the
> corresponding SOC involved ?
> 
> If a not-null DCD pointer affects just future SOCs, we should fix it
> when these SOCs will be available. This means both new SOSs (imx8x) as
> new version of supported SOCs (mx5 / mx6 / mx7).
> 
> > Users performing such an
> > update should definitely test the image prior to making an OTA
> > available.
> 
> I think this is another topic and not what Brian is trying to address. I
> hope as you that *any* update is tested before deploying. But this is
> unrelated.
> 
> > It has never been intended for DCD to be used in any post
> > boot image and we realize the lack of documentation is a hindsight by
> > us, and we are currently addressing that with updated guidance.
> > 
> 
> ok, thanks for this, very appreciated.
> 
> Anyway, I am facing what we should do with this patch for 2018.03. As I
> said, I am not forcing anyone and I have just picked up 1/3 and 2/3. But
> IMHO, if there are not good reason to say that not raising an error
> breaks a lot of supplied device, this should flow into 2018.03, too. And
> then we get enough time to better explore this issue.

And I need an answer to this final part, before I can release v2018.03.
Thanks all!
Breno Matheus Lima March 12, 2018, 4:33 p.m. UTC | #6
Hi All,

2018-03-12 13:07 GMT-03:00 Tom Rini <trini@konsulko.com>:
> On Sun, Mar 11, 2018 at 03:36:16PM +0100, Stefano Babic wrote:
>> Hi Everybody,
>>
>> I have applied 1-2 as Fabio suggested. I have a couple of comments for
>> this, too:
>>
>> On 10/03/2018 02:10, Breno Matheus Lima wrote:
>> > Hi Bryan,
>> >
>> > 2018-03-09 10:07 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
>> >> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
>> >> to calling HAB authenticate function.") makes the DCD field being NULL a
>> >> dependency.
>> >>
>> >> This change though will break loading and executing of existing pre-signed
>> >> binaries on a u-boot update i.e. if this change is deployed on a board you
>> >> will be forced to redo all images on that board to NULL out the DCD.
>> >>
>> >> There is no prior guidance from NXP that the DCD must be NULL similarly
>> >> public guidance on usage of the HAB doesn't call out this NULL dependency
>> >> (see boundary devices link).
>> >>
>> >> Since later SoCs will reject a non-NULL DCD there's no reason to make a
>> >> NULL DCD a requirement, however if there is an actual dependency for later
>> >> SoCs the appropriate fix would be to do SoC version checking.
>> >>
>> >> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
>> >> DCDs, we should not be forcing this change on downstream users -
>> >> particularly if it means those users now must rewrite their build systems
>> >> and/or redeploy signed images in the field.
>> >>
>> >> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
>> >> to calling HAB authenticate function.")
>> >
>> > We understand the concern being raised however would prefer to leave
>> > this as an error, and selected users relying on images with DCD
>> > pointers can modify the code accordingly. This does not just apply to
>> > new SoC’s but also applies to existing SoC’s.
>>
>> Anyway, this is not so easy to identify. What we current know is that
>> older SOCs have no problem if the pointer is not set to Null. I agree
>> with Brian that if some constraint is coming, it should be defined with
>> a version checking as we currently do for a lot of things (is_soc() to
>> be clear).
>>
>> I am quite lost because I do not know which SOCs are affected and which
>> not. What does it hide under "later SOCSs" ? Or better which new version
>> of HAB ? I know HAB was updated due to other issues - are the
>> corresponding SOC involved ?
>>
>> If a not-null DCD pointer affects just future SOCs, we should fix it
>> when these SOCs will be available. This means both new SOSs (imx8x) as
>> new version of supported SOCs (mx5 / mx6 / mx7).
>>
>> > Users performing such an
>> > update should definitely test the image prior to making an OTA
>> > available.
>>
>> I think this is another topic and not what Brian is trying to address. I
>> hope as you that *any* update is tested before deploying. But this is
>> unrelated.
>>
>> > It has never been intended for DCD to be used in any post
>> > boot image and we realize the lack of documentation is a hindsight by
>> > us, and we are currently addressing that with updated guidance.
>> >
>>
>> ok, thanks for this, very appreciated.
>>
>> Anyway, I am facing what we should do with this patch for 2018.03. As I
>> said, I am not forcing anyone and I have just picked up 1/3 and 2/3. But
>> IMHO, if there are not good reason to say that not raising an error
>> breaks a lot of supplied device, this should flow into 2018.03, too. And
>> then we get enough time to better explore this issue.
>
> And I need an answer to this final part, before I can release v2018.03.
> Thanks all!
>

The purpose of hab_rvt_authenticate_image() API function is to
authenticate additional boot images in a post-ROM stage, initial boot
images are supposed to be authenticate only once by the initial ROM
code. The HAB implementation in older devices will process and run DCD
if we provide a DCD pointer. DCD commands are supposed to be executed
only once in an early boot stage, re-executing it could lead to an
incorrect authentication boot flow. If we convert DCD non-NULL error
to warning may also break supported devices, not only the new ones.

We understand Bryan's point based in CST documentation but
unfortunately our documentation is outdated, we are currently working
in a new version.

As Utkarsh mentioned in commit 8c4037a09a5c ("imx: hab: Ensure the IVT
DCD pointer is Null prior to calling HAB authenticate function."):
"DCD commands should only be present in the initial boot image loaded
by the SoC ROM. DCD should not be present in images that will be
verified by software using HAB RVT authentication APIs. Newer versions
of HAB will generate an error if a DCD pointer is present in an image
being authenticated by calling the HAB RVT API. Older versions of HAB
will process and run DCD if it is present, and this could lead to an
incorrect authentication boot flow."

Thanks,
Breno Lima
Bryan O'Donoghue March 12, 2018, 7:10 p.m. UTC | #7
On 12/03/18 16:33, Breno Matheus Lima wrote:

> The purpose of hab_rvt_authenticate_image() API function is to
> authenticate additional boot images in a post-ROM stage, initial boot
> images are supposed to be authenticate only once by the initial ROM
> code. The HAB implementation in older devices will process and run DCD
> if we provide a DCD pointer. DCD commands are supposed to be executed
> only once in an early boot stage,


Breno if that is so, why is there a ROM provided callback "run_dcd" ?

3.4
hab_status_t(* hab_rvt::run_dcd)(const uint8_t *dcd)

It may be the case that you are moving to the DCD being a bootrom only 
interface but that is certainly not the case right now.

> re-executing it could lead to an
> incorrect authentication boot flow. 

Which is the difference between "the DCD" i.e. the only DCD that can run 
and "a DCD" - meaning the DCD associated with an image.

You've provided APIs to run a DCD, make extensive reference to running 
'a DCD' with a given image.

How can you be so sure that all users of u-boot HAB don't have a DCD 
phase with images after the first phase ?

> If we convert DCD non-NULL error
> to warning may also break supported devices, not only the new ones.

Which ones ? Can you give some details to back this up ?


> We understand Bryan's point based in CST documentation but
> unfortunately our documentation is outdated, we are currently working
> in a new version.

But Breno - until and unless you publish something that super-cedes the 
current published standard - you are introducing breakage into the 
current HAB layer.

IMO the right thing to do is to publish a description the issue you are 
trying to address, then discuss fixes for it.


> As Utkarsh mentioned in commit 8c4037a09a5c ("imx: hab: Ensure the IVT
> DCD pointer is Null prior to calling HAB authenticate function."):

> "DCD commands should only be present in the initial boot image loaded
> by the SoC ROM.

Which is an assertion you are making now, without reference to any 
supporting litreature and proposing that everybody using the HAB 
interface just adopts this change and churns their downstream images.


> DCD should not be present in images that will be
> verified by software using HAB RVT authentication APIs.

Not according to your latest published standard on HAB. Can you really 
prove that nobody is using DCD specifically "run_dcd()" as provided by 
your own BootROM at this time ?

On what basis are you forcing end-users to rewrite their code-signing 
infrastructure and re-sign all of their binaries - potentially binaries 
in the field ?

I really can't agree with this approach. If you want to force such a 
change on people - you need a reason.

Consider a user with an i.MX6 board who wants to pick up a fix for an 
unrelated issue - USB for agrgument's sake. They then need to re-sign 
all of the binaries u-boot authenticates via HAB for no benefit to that 
user at all.

> Newer versions
> of HAB will generate an error if a DCD pointer is present in an image
> being authenticated by calling the HAB RVT API. 

Then version check it ! Why do existing users need to suck up the change 
for upcoming (unrleased?) HAB implementations ?

> Older versions of HAB
> will process and run DCD if it is present, and this could lead to an
> incorrect authentication boot flow."

Sorry I really don't accept this - you provide a _callback_ called 
"run_dcd()" in your BootROM.

Meaning I provide a pointer to a signed image that includes a DCD phase. 
I can then run the DCD in isolation.

Why is that now broken on older HAB implementations ?

Honestly - I think this change is pretty bogus - we should either revert 
it or as I've proposed her "Warn".

You can then come along and version check on later SoCs once you've 
published _supporting_documentation_ to go with it - that justifies and 
explains (in detail) why it is necessary to restrict this interface on 
new (or existing SoCs).

---
bod
Fabio Estevam March 12, 2018, 8:51 p.m. UTC | #8
Hi Tom and Stefano,

On Mon, Mar 12, 2018 at 1:07 PM, Tom Rini <trini@konsulko.com> wrote:

>> Anyway, I am facing what we should do with this patch for 2018.03. As I
>> said, I am not forcing anyone and I have just picked up 1/3 and 2/3. But
>> IMHO, if there are not good reason to say that not raising an error
>> breaks a lot of supplied device, this should flow into 2018.03, too. And
>> then we get enough time to better explore this issue.
>
> And I need an answer to this final part, before I can release v2018.03.
> Thanks all!

Bryan's points are valid, so please apply his patch for v2018.03.

Breno and team can then revisit this for v2018.05.

Thanks
Fabio Estevam March 12, 2018, 8:53 p.m. UTC | #9
On Fri, Mar 9, 2018 at 10:07 AM, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.") makes the DCD field being NULL a
> dependency.
>
> This change though will break loading and executing of existing pre-signed
> binaries on a u-boot update i.e. if this change is deployed on a board you
> will be forced to redo all images on that board to NULL out the DCD.
>
> There is no prior guidance from NXP that the DCD must be NULL similarly
> public guidance on usage of the HAB doesn't call out this NULL dependency
> (see boundary devices link).
>
> Since later SoCs will reject a non-NULL DCD there's no reason to make a
> NULL DCD a requirement, however if there is an actual dependency for later
> SoCs the appropriate fix would be to do SoC version checking.
>
> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
> DCDs, we should not be forcing this change on downstream users -
> particularly if it means those users now must rewrite their build systems
> and/or redeploy signed images in the field.
>
> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.")
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Link: https://boundarydevices.com/high-assurance-boot-hab-dummies

In order to avoid regression, better apply this one for v2018.03:

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Stefano Babic March 13, 2018, 8:55 a.m. UTC | #10
On 12/03/2018 21:51, Fabio Estevam wrote:
> Hi Tom and Stefano,
> 
> On Mon, Mar 12, 2018 at 1:07 PM, Tom Rini <trini@konsulko.com> wrote:
> 
>>> Anyway, I am facing what we should do with this patch for 2018.03. As I
>>> said, I am not forcing anyone and I have just picked up 1/3 and 2/3. But
>>> IMHO, if there are not good reason to say that not raising an error
>>> breaks a lot of supplied device, this should flow into 2018.03, too. And
>>> then we get enough time to better explore this issue.
>>
>> And I need an answer to this final part, before I can release v2018.03.
>> Thanks all!
> 
> Bryan's points are valid, so please apply his patch for v2018.03.>
> Breno and team can then revisit this for v2018.05.

Agree, this is the best way to go on.

Tom, can you apply it ? I have not other pending patches for the
release, and my PR had just this one.

Thanks,
Stefano
Tom Rini March 13, 2018, 12:13 p.m. UTC | #11
On Fri, Mar 09, 2018 at 01:07:21PM +0000, Bryan O'Donoghue wrote:

> commit 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.") makes the DCD field being NULL a
> dependency.
> 
> This change though will break loading and executing of existing pre-signed
> binaries on a u-boot update i.e. if this change is deployed on a board you
> will be forced to redo all images on that board to NULL out the DCD.
> 
> There is no prior guidance from NXP that the DCD must be NULL similarly
> public guidance on usage of the HAB doesn't call out this NULL dependency
> (see boundary devices link).
> 
> Since later SoCs will reject a non-NULL DCD there's no reason to make a
> NULL DCD a requirement, however if there is an actual dependency for later
> SoCs the appropriate fix would be to do SoC version checking.
> 
> Earlier SoCs are capable (and happy) to authenticate images with non-NULL
> DCDs, we should not be forcing this change on downstream users -
> particularly if it means those users now must rewrite their build systems
> and/or redeploy signed images in the field.
> 
> Fixes: 8c4037a09a5c ("imx: hab: Ensure the IVT DCD pointer is Null prior
> to calling HAB authenticate function.")
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> Cc: Breno Lima <breno.lima@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Link: https://boundarydevices.com/high-assurance-boot-hab-dummies
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
index c3fc699..c730c8f 100644
--- a/arch/arm/mach-imx/hab.c
+++ b/arch/arm/mach-imx/hab.c
@@ -526,10 +526,8 @@  int imx_hab_authenticate_image(uint32_t ddr_start, uint32_t image_size,
 	}
 
 	/* Verify if IVT DCD pointer is NULL */
-	if (ivt->dcd) {
-		puts("Error: DCD pointer must be NULL\n");
-		goto hab_authentication_exit;
-	}
+	if (ivt->dcd)
+		puts("Warning: DCD pointer should be NULL\n");
 
 	start = ddr_start;
 	bytes = image_size;