[U-Boot] [PATCH 0/3] HAB Fixes for 2018.03-rc4
This set fixes a few errors and gotchas I've found when rebasing my project on top of 2018-rc4.
First is a breakage with the Linaro compiler -> gcc-linaro-7.2.1 which is a simple one.
Second is consistency of the __packed attribute - more of a housekeeping activity.
Last patch fixes a behaviour that has been introduced without I believe fully thinking through the impact on downstream users.
If we force the DCD to be NULL for olders SoCs - that means people have to go out and re-sign all of their binaries if they are updating u-boot.
It should be possible to update u-boot in isolation without forcing update of signed binaries u-boot authenticates via HAB.
If newer NXP SoCs rely on the DCD being NULL then either
1. Just let the authenticate_image() call fail It can return an error - this should "just work" if the SoC will reject an API callback with the DCD non-NULL then - let it.
2. If the SoC actually has a bug that _requires_ the DCD to be NULL. Then add a check into u-boot to check SoC versions.
In either case older SoCs should "just work" and it should be possible to update u-boot without having to update all associated signed binaries.
Please apply before next official u-boot release.
Bryan O'Donoghue (3): imx: hab: Fix usage of packed attribute imx: hab: Make usage of packed attribute consistent imx: hab: Convert DCD non-NULL error to warning
arch/arm/include/asm/mach-imx/hab.h | 5 +++-- arch/arm/mach-imx/hab.c | 6 ++---- 2 files changed, 5 insertions(+), 6 deletions(-)
commit ed286bc80e9d ("imx: hab: Check if CSF is valid before authenticating image") makes use of "__packed" as a prefix to the "struct hab_hdr" declaration.
With my compiler "gcc version 7.2.1 20171011 (Linaro GCC 7.2-2017.11)" we get:
./arch/arm/include/asm/mach-imx/hab.h:42:25: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘{’ token struct __packed hab_hdr {
Fix this problem by including <linux/compiler.h>
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 --- arch/arm/include/asm/mach-imx/hab.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/mach-imx/hab.h b/arch/arm/include/asm/mach-imx/hab.h index 561de9c..a522cba 100644 --- a/arch/arm/include/asm/mach-imx/hab.h +++ b/arch/arm/include/asm/mach-imx/hab.h @@ -9,6 +9,7 @@ #define __SECURE_MX6Q_H__
#include <linux/types.h> +#include <linux/compiler.h>
/* * IVT header definitions
On Fri, Mar 9, 2018 at 10:07 AM, Bryan O'Donoghue bryan.odonoghue@linaro.org wrote:
commit ed286bc80e9d ("imx: hab: Check if CSF is valid before authenticating image") makes use of "__packed" as a prefix to the "struct hab_hdr" declaration.
With my compiler "gcc version 7.2.1 20171011 (Linaro GCC 7.2-2017.11)" we get:
./arch/arm/include/asm/mach-imx/hab.h:42:25: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘{’ token struct __packed hab_hdr {
Fix this problem by including <linux/compiler.h>
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
Reviewed-by: Fabio Estevam fabio.estevam@nxp.com
commit cd2d46003ce1 ("arm: imx: hab: Add IVT header definitions") declares struct ivt_header as "__attribute__((packed))".
commit ed286bc80e9d ("imx: hab: Check if CSF is valid before authenticating image") declares struct hab_hdr with __packed.
This patch makes the __packed convention consistent.
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 --- arch/arm/include/asm/mach-imx/hab.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/mach-imx/hab.h b/arch/arm/include/asm/mach-imx/hab.h index a522cba..ce9a44d 100644 --- a/arch/arm/include/asm/mach-imx/hab.h +++ b/arch/arm/include/asm/mach-imx/hab.h @@ -22,11 +22,11 @@ #define IVT_HEADER_V1 0x40 #define IVT_HEADER_V2 0x41
-struct ivt_header { +struct __packed ivt_header { uint8_t magic; uint16_t length; uint8_t version; -} __attribute__((packed)); +};
struct ivt { struct ivt_header hdr; /* IVT header above */
On Fri, Mar 9, 2018 at 10:07 AM, Bryan O'Donoghue bryan.odonoghue@linaro.org wrote:
commit cd2d46003ce1 ("arm: imx: hab: Add IVT header definitions") declares struct ivt_header as "__attribute__((packed))".
commit ed286bc80e9d ("imx: hab: Check if CSF is valid before authenticating image") declares struct hab_hdr with __packed.
This patch makes the __packed convention consistent.
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
Reviewed-by: Fabio Estevam fabio.estevam@nxp.com
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(-)
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;
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
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
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
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!
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
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
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
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
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
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
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!
participants (5)
-
Breno Matheus Lima -
Bryan O'Donoghue -
Fabio Estevam -
Stefano Babic -
Tom Rini