[Skiboot] [PATCH v4 04/18] secvar_main: rework secvar_main error flow, make storage locking explicit

[Oliver O'Halloran]

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;
>  }