shim should not self revoke

Before applying an updated SbatLevel shim should re-run
introspection and never apply a revocation level that would
prevent the currently running shim from booting. The proper
way forward is to update shim first.

Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

4 files changed, 62 insertions, 28 deletions

diff --git a/include/sbat.h b/include/sbat.h
index af4c1a8f..20009ada 100644
--- a/include/sbat.h
+++ b/include/sbat.h
@@ -53,7 +53,7 @@ extern list_t sbat_var;
 #define SBAT_VAR_COLUMNS ((sizeof (struct sbat_var_entry) - sizeof(list_t)) / sizeof(CHAR8 *))
 #define SBAT_VAR_REQUIRED_COLUMNS (SBAT_VAR_COLUMNS - 1)
 
-EFI_STATUS parse_sbat_var(list_t *entries);
+EFI_STATUS parse_sbat_var(list_t *entries, char *sbat_var_candidate);
 void cleanup_sbat_var(list_t *entries);
 EFI_STATUS set_sbat_uefi_variable_internal(void);
 EFI_STATUS set_sbat_uefi_variable(char *, char *);
diff --git a/sbat.c b/sbat.c
index b33fe85c..391b59e6 100644
--- a/sbat.c
+++ b/sbat.c
@@ -302,7 +302,7 @@ err:
 }
 
 EFI_STATUS
-parse_sbat_var(list_t *entries)
+parse_sbat_var(list_t *entries, char *sbat_var_candidate)
 {
 	UINT8 *data = 0;
 	UINTN datasize;
@@ -314,10 +314,17 @@ parse_sbat_var(list_t *entries)
 		return EFI_INVALID_PARAMETER;
 	}
 
-	efi_status = get_variable(SBAT_VAR_NAME, &data, &datasize, SHIM_LOCK_GUID);
-	if (EFI_ERROR(efi_status)) {
-		LogError(L"Failed to read SBAT variable\n", efi_status);
-		return efi_status;
+	if (sbat_var_candidate == NULL) {
+		dprint(L"sbat_var_candidate is NULL, reading variable\n");
+		efi_status = get_variable(SBAT_VAR_NAME, &data, &datasize, SHIM_LOCK_GUID);
+		if (EFI_ERROR(efi_status)) {
+			LogError(L"Failed to read SBAT variable\n", efi_status);
+			return efi_status;
+		}
+	} else {
+		datasize = strlen(sbat_var_candidate);
+		data = AllocatePool(datasize + 1);
+		memcpy(data, sbat_var_candidate, datasize);
 	}
 
 	/*
@@ -325,8 +332,10 @@ parse_sbat_var(list_t *entries)
 	 * allocations, so use that here.
 	 */
 	efi_status = parse_sbat_var_data(entries, data, datasize+1);
-	if (EFI_ERROR(efi_status))
-		return efi_status;
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"parse_sbat_var_data() failed datasize: %d\n", datasize);
+		goto out;
+	}
 
 	dprint(L"SBAT variable entries:\n");
 	list_for_each(pos, entries) {
@@ -337,6 +346,8 @@ parse_sbat_var(list_t *entries)
 		       entry->component_generation, entry->sbat_datestamp);
 	}
 
+out:
+	FreePool(data);
 	return efi_status;
 }
 
@@ -465,7 +476,7 @@ set_sbat_uefi_variable(char *sbat_var_previous, char *sbat_var_latest)
 	UINTN sbatsize = 0;
 	UINTN sbat_policysize = 0;
 
-	char *sbat_var = NULL;
+	char *sbat_var_candidate = NULL;
 	bool reset_sbat = false;
 
 	if (sbat_policy == POLICY_NOTREAD) {
@@ -481,39 +492,39 @@ set_sbat_uefi_variable(char *sbat_var_previous, char *sbat_var_latest)
 	if (EFI_ERROR(efi_status)) {
 		dprint("Default sbat policy: previous\n");
 		if (secure_mode()) {
-			sbat_var = sbat_var_previous;
+			sbat_var_candidate = sbat_var_previous;
 		} else {
 			reset_sbat = true;
-			sbat_var = SBAT_VAR_ORIGINAL;
+			sbat_var_candidate = SBAT_VAR_ORIGINAL;
 		}
 	} else {
 		switch (sbat_policy) {
-		case SBAT_POLICY_LATEST:
+		case POLICY_LATEST:
 			dprint("Custom sbat policy: latest\n");
-			sbat_var = sbat_var_latest;
+			sbat_var_candidate = sbat_var_latest;
 			break;
-		case SBAT_POLICY_PREVIOUS:
+		case POLICY_PREVIOUS:
 			dprint("Custom sbat policy: previous\n");
-			sbat_var = sbat_var_previous;
+			sbat_var_candidate = sbat_var_previous;
 			break;
-		case SBAT_POLICY_RESET:
+		case POLICY_RESET:
 			if (secure_mode()) {
 				console_print(L"Cannot reset SBAT policy: Secure Boot is enabled.\n");
-				sbat_var = sbat_var_previous;
+				sbat_var_candidate = sbat_var_previous;
 			} else {
 				dprint(L"Custom SBAT policy: reset OK\n");
 				reset_sbat = true;
-				sbat_var = SBAT_VAR_ORIGINAL;
+				sbat_var_candidate = SBAT_VAR_ORIGINAL;
 			}
 			break;
 		default:
 			console_error(L"SBAT policy state %llu is invalid",
 				      EFI_INVALID_PARAMETER);
 			if (secure_mode()) {
-				sbat_var = sbat_var_previous;
+				sbat_var_candidate = sbat_var_previous;
 			} else {
 				reset_sbat = true;
-				sbat_var = SBAT_VAR_ORIGINAL;
+				sbat_var_candidate = SBAT_VAR_ORIGINAL;
 			}
 			break;
 		}
@@ -526,8 +537,9 @@ set_sbat_uefi_variable(char *sbat_var_previous, char *sbat_var_latest)
 	 */
 	if (EFI_ERROR(efi_status)) {
 		dprint(L"SBAT read failed %r\n", efi_status);
-	} else if (preserve_sbat_uefi_variable(sbat, sbatsize, attributes, sbat_var)
-			&& !reset_sbat) {
+	} else if (preserve_sbat_uefi_variable(sbat, sbatsize, attributes,
+	                                       sbat_var_candidate) &&
+	           !reset_sbat) {
 		dprint(L"preserving %s variable it is %d bytes, attributes are 0x%08x\n",
 		       SBAT_VAR_NAME, sbatsize, attributes);
 		FreePool(sbat);
@@ -535,6 +547,27 @@ set_sbat_uefi_variable(char *sbat_var_previous, char *sbat_var_latest)
 	} else {
 		FreePool(sbat);
 
+		/*
+		 * parse the candidate SbatLevel and check that shim will not
+		 * self revoke before writing SbatLevel variable
+		 */
+		dprint(L"shim SBAT reparse before application\n");
+		efi_status = parse_sbat_var(&sbat_var, sbat_var_candidate);
+		if (EFI_ERROR(efi_status)) {
+			dprint(L"proposed SbatLevel failed to parse\n");
+			return efi_status;
+		}
+
+#ifndef SHIM_UNIT_TEST
+		char *sbat_start = (char *)&_sbat;
+		char *sbat_end = (char *)&_esbat;
+		efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1);
+		if (EFI_ERROR(efi_status)) {
+			dprint(L"shim SBAT self check fails for new SbatLevel, refusing to apply\n");
+			return efi_status;
+		}
+#endif /* SHIM_UNIT_TEST */
+
 		/* delete previous variable */
 		dprint("%s variable is %d bytes, attributes are 0x%08x\n",
 		       SBAT_VAR_NAME, sbatsize, attributes);
@@ -550,7 +583,7 @@ set_sbat_uefi_variable(char *sbat_var_previous, char *sbat_var_latest)
 
 	/* set variable */
 	efi_status = set_variable(SBAT_VAR_NAME, SHIM_LOCK_GUID, SBAT_VAR_ATTRS,
-	                          strlen(sbat_var), sbat_var);
+	                          strlen(sbat_var_candidate), sbat_var_candidate);
 	if (EFI_ERROR(efi_status)) {
 		dprint(L"%s variable writing failed %r\n", SBAT_VAR_NAME,
 				efi_status);
@@ -565,10 +598,11 @@ set_sbat_uefi_variable(char *sbat_var_previous, char *sbat_var_latest)
 		return efi_status;
 	}
 
-	if (sbatsize != strlen(sbat_var) ||
-	    strncmp((const char *)sbat, sbat_var, strlen(sbat_var)) != 0) {
+	if (sbatsize != strlen(sbat_var_candidate) ||
+	    strncmp((const char *)sbat, sbat_var_candidate,
+		    strlen(sbat_var_candidate)) != 0) {
 		dprint("new sbatsize is %d, expected %d\n", sbatsize,
-		       strlen(sbat_var));
+		       strlen(sbat_var_candidate));
 		efi_status = EFI_INVALID_PARAMETER;
 	} else {
 		dprint(L"%s variable initialization succeeded\n", SBAT_VAR_NAME);
diff --git a/shim.c b/shim.c
index 2ad1c063..cfdf2732 100644
--- a/shim.c
+++ b/shim.c
@@ -1857,7 +1857,7 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 		char *sbat_end = (char *)&_esbat;
 
 		INIT_LIST_HEAD(&sbat_var);
-		efi_status = parse_sbat_var(&sbat_var);
+		efi_status = parse_sbat_var(&sbat_var, NULL);
 		if (EFI_ERROR(efi_status)) {
 			perror(L"Parsing %s variable failed: %r\n",
 				SBAT_VAR_NAME, efi_status);
diff --git a/test-sbat.c b/test-sbat.c
index f59a1723..980a8f86 100644
--- a/test-sbat.c
+++ b/test-sbat.c
@@ -379,7 +379,7 @@ test_parse_sbat_var_null_list(void)
 	EFI_STATUS status;
 
 	INIT_LIST_HEAD(&sbat_var);
-	status = parse_sbat_var(NULL);
+	status = parse_sbat_var(NULL, NULL);
 	cleanup_sbat_var(&sbat_var);
 	assert_equal_return(status, EFI_INVALID_PARAMETER, -1, "got %#hhx expected %#hhx\n");