[nrf noup] bootutil: Add error log on KMU not provisioned #435
Conversation
hellesvik-nordic
force-pushed
the
kmu_no_key_error_log
branch
from
c5c2466 to
01efde1
Compare
boot/bootutil/src/ed25519_psa.c
Outdated
|
|
||
| BOOT_LOG_ERR("ED25519 signature verification failed %d", status); | ||
| if(status == PSA_ERROR_INVALID_HANDLE) { | ||
| BOOT_LOG_ERR("PSA_ERROR_INVALID_HANDLE(-136) could mean that the KMU slot is not provisioned."); |
There was a problem hiding this comment.
| BOOT_LOG_ERR("PSA_ERROR_INVALID_HANDLE(-136) could mean that the KMU slot is not provisioned."); | |
| BOOT_LOG_ERR("PSA ED25519 signature failed (-136), unknown key ID"); |
Are you sure we want to be that verbose? Non KMU specific message could actually be brought upstream too.
There was a problem hiding this comment.
I would argue that we want users to think "Oh I need to provision the KMU" when reading the message, so at the very least the KMU should be mentioned.
This function is also KMU specific (inside the elif) so I think it makes sense to mention the KMU in the log.
hellesvik-nordic
force-pushed
the
kmu_no_key_error_log
branch
from
01efde1 to
ade6a05
Compare
|
@de-nordic, I added CONFIG_MCUBOOT_SETUP_VALIDATION, which might be what we talked about. What do you think about this? |
hellesvik-nordic
force-pushed
the
kmu_no_key_error_log
branch
from
ade6a05 to
70ddb36
Compare
|
After discussing with @de-nordic directly, we concluded that this change will not need a Kconfig option, as this is a one-off extra log to catch a very common error case. |
Checks if the KMU has been provisioned, and add extra log if it has not. Error 136 could theoretically be caused by something else, so the extra logs is not phrased in a absolute manner. Fixes: NCSDK-33559 Signed-off-by: Sigurd Hellesvik <[email protected]>
hellesvik-nordic
force-pushed
the
kmu_no_key_error_log
branch
from
70ddb36 to
3a6fbc1
Compare
|
There was a problem hiding this comment.
my problem with this is that this bloats anyone whose got logging enabled's code on a condition that will only appear on someone's board who doesn't know what they are doing or doesn't read documentation or doesn't use the new (default) kconfig that is being added to automatically provision things
| } | ||
|
|
||
| BOOT_LOG_ERR("ED25519 signature verification failed %d", status); | ||
| if(status == PSA_ERROR_INVALID_HANDLE) { |
There was a problem hiding this comment.
space between if and bracket in c
There was a problem hiding this comment.
will only appear on someone's board who doesn't know what they are doing or doesn't read documentation
For these two, I would say that we should still warn for this case, as users who just program a sample can get this error. But then you say:
or doesn't use the new (default) kconfig that is being added to automatically provision things
And with this, Im not sure that this warning is needed after all.
The use-case for this PR is the following:
"I think that users should be able to enable a bootloader in any project without having to have a full overview over everything that happens. If the default case is that they need to provision manually, then pretty much everyone will get an error first time they try, and we should then tell them that they need to provision, so not everyone has to spend an hour reading docs to find the line saying they should provision."
However, if the devices will be automatically provisioned by default, then I think we can expect users that set CONFIG_MANUAL_PROVISION or whatever should understand the consequence of this, and the use-case for this PR is no longer valid.
@nordicjm Do I understand correctly that from v3.1.0 and onwards, the nRF54L15 KMU will be provisioned when I run "west flash", so that both MCUboot and NSIB will run without any issues first try?
There was a problem hiding this comment.
Not sure if it's 3.1.0 or how it fully works because not tried running it manually myself but PR is nrfconnect/sdk-nrf#22516
There was a problem hiding this comment.
That is good enough for me.
I will close this PR, and then test the new provisioning feature with "west flash" when that is ready to verify that it indeed solves the problem I see.
|
A lot less reasons to give another warning for un-provisioned KMU if the KMU is provisioned by default: |
but it will not be provisioned by default, user must set:
So still the problem exists. |
5 participants
An unprovisioned KMU can cause confusion, so improve the logs for this case.