[Skiboot] [PATCH v4 04/18] secvar_main: rework secvar_main error flow, make storage locking explicit
Oliver O'Halloran
oohall at gmail.com
Tue May 12 14:22:44 AEST 2020
On Mon, 2020-05-11 at 16:31 -0500, Eric Richter wrote:
> This patch adjusts the behavior of secvar_main to actually halt the boot
> in some form if there is an issue initializing secure variables.
> We
> halt in skiboot if the secvar storage driver fails to initialize, as this
> should only happen due to unresolvable hardware issues, and contains our
> secure boot state.
doesn't parse
> For all other cases we enforce secure boot in the
> bootloader (secure mode flag is set, but there will be no keys).
>
> Previously, the storage driver was expected to handle any locking
> procedures implicitly as part of the write operation. This patch makes
> the locking behavior now explicit, and part of the secvar_main flow.
>
> The storage driver is now locked unconditionally when exiting
> secvar_main, and the lock() call should halt the boot if it encounters
> any sign of struggle.
Call it .seal(), or .lockdown(), or similar. Anything called lock() I'd
expect to be balanced with an unlock().
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
> V4:
> - adjusted lines to 80 columns
> - calls secureboot_enforce() instead of abort()
>
> libstb/secvar/secvar_main.c | 38 +++++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c
> index 3b842d16..f4b98925 100644
> --- a/libstb/secvar/secvar_main.c
> +++ b/libstb/secvar/secvar_main.c
> @@ -8,6 +8,7 @@
> #include <stdlib.h>
> #include <skiboot.h>
> #include <opal.h>
> +#include <libstb/secureboot.h>
> #include "secvar.h"
> #include "secvar_devtree.h"
>
> @@ -39,11 +40,15 @@ int secvar_main(struct secvar_storage_driver storage_driver,
> list_head_init(&variable_bank);
> list_head_init(&update_bank);
>
> + /* Failures here should indicate some kind of hardware problem,
> + * therefore we don't even attempt to continue */
> rc = secvar_storage.store_init();
> if (rc)
> - goto fail;
> + secureboot_enforce();
> - /* Failures here should indicate some kind of hardware problem */
> + /* Failures here may be recoverable,
> + * (PNOR was corrupted, restore from backup)
> + * so we want to boot up to skiroot to give the user a chance to fix */
Can you elaborate about what is actually happening here. I think you're
saying it'll boot to petitboot with an empty key ring so you can
potentially restore it, but the verbiage here assumes you already know
what it does.
> rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> if (rc)
> goto fail;
> @@ -61,6 +66,8 @@ int secvar_main(struct secvar_storage_driver storage_driver,
> rc = secvar_backend.pre_process();
> if (rc) {
> prlog(PR_ERR, "Error in backend pre_process = %d\n", rc);
> + /* Early failure state, lock the storage */
> + secvar_storage.lock();
> goto out;
> }
> }
> @@ -83,11 +90,20 @@ int secvar_main(struct secvar_storage_driver storage_driver,
> SECVAR_VARIABLE_BANK);
> if (rc)
> goto out;
> -
> - rc = secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK);
> + }
> + /* Write (and probably clear) the update bank if .process() actually
> + * detected and handled updates in the update bank. Unlike above, this
> + * includes error cases, where the backend should probably be clearing
> + * the bank.
> + */
> + if (rc != OPAL_EMPTY) {
> + rc = secvar_storage.write_bank(&update_bank,
> + SECVAR_UPDATE_BANK);
> if (rc)
> goto out;
> }
> + /* Unconditionally lock the storage at this point */
> + secvar_storage.lock();
>
> if (secvar_backend.post_process) {
> rc = secvar_backend.post_process();
> @@ -100,9 +116,23 @@ int secvar_main(struct secvar_storage_driver storage_driver,
> prlog(PR_INFO, "secvar initialized successfully\n");
>
> return OPAL_SUCCESS;
> +
> fail:
> + /* Early failure, base secvar support failed to initialize */
> secvar_set_status("fail");
> + secvar_storage.lock();
> + secvar_set_secure_mode();
> +
> + prerror("secvar failed to initialize, rc = %04x\n", rc);
> + return rc;
> +
> out:
rename out to soft_fail or something.
> + /* Soft-failure, enforce secure boot in bootloader for debug/recovery */
> + clear_bank_list(&variable_bank);
> + clear_bank_list(&update_bank);
> + secvar_storage.lock();
> + secvar_set_secure_mode();
> +
> prerror("secvar failed to initialize, rc = %04x\n", rc);
> return rc;
> }
More information about the Skiboot
mailing list