diff mbox series

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

Message ID 20200511213152.24952-5-erichte@linux.ibm.com
State Changes Requested
Headers show
Series Add initial secure variable storage and backend drivers | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot fail Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (0f1937ef40fca0c3212a9dff1010b832a24fb063)

Commit Message

Eric Richter May 11, 2020, 9:31 p.m. UTC
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. 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.

Signed-off-by: Eric Richter <erichte@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(-)

Comments

Oliver O'Halloran May 12, 2020, 4:22 a.m. UTC | #1
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@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;
>  }
diff mbox series

Patch

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 */
 	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:
+	/* 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;
 }