better error message when shim self validation fails

This exports the component name and found and expected SBAT level from verify_sbat_section for us to display to the user why validation failed. Further we now differentiate between errors regarding .sbat section related errors and SBAT revocations. Signed-off-by: Thore Sommer <[email protected]>
rhboot · Aug 22, 2024 · 8b3f5ce · 8b3f5ce
1 parent 0287c6b
commit 8b3f5ce
Show file tree

Hide file tree

Showing 6 changed files with 90 additions and 23 deletions.
diff --git a/include/pe.h b/include/pe.h
@@ -19,7 +19,7 @@ EFI_STATUS verify_image(void *data, unsigned int datasize,
 			PE_COFF_LOADER_IMAGE_CONTEXT *context);
 
 EFI_STATUS
-verify_sbat_section(char *SBATBase, size_t SBATSize);
+verify_sbat_section(char *SBATBase, size_t SBATSize, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name);
 
 EFI_STATUS
 get_section_vma (UINTN section_num,

diff --git a/include/sbat.h b/include/sbat.h
@@ -77,12 +77,14 @@ parse_sbat_section(char *section_base, size_t section_size, size_t *n,
 		   struct sbat_section_entry ***entriesp);
 void cleanup_sbat_section_entries(size_t n, struct sbat_section_entry **entries);
 
-EFI_STATUS verify_sbat(size_t n, struct sbat_section_entry **entries);
+EFI_STATUS verify_sbat(size_t n, struct sbat_section_entry **entries, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name);
 
 #ifdef SHIM_UNIT_TEST
 EFI_STATUS parse_sbat_var_data(list_t *entries, UINT8 *data, UINTN datasize);
-EFI_STATUS verify_sbat_helper(list_t *sbat_var, size_t n,
-                              struct sbat_section_entry **entries);
+EFI_STATUS verify_sbat_helper(list_t *local_sbat_var, size_t n,
+							struct sbat_section_entry **entries,
+							UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found,
+							CHAR8 **sbat_component_name);
 #endif /* !SHIM_UNIT_TEST */
 #endif /* !SBAT_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/pe.c b/pe.c
@@ -333,7 +333,7 @@ generate_hash(char *data, unsigned int datasize,
 }
 
 EFI_STATUS
-verify_sbat_section(char *SBATBase, size_t SBATSize)
+verify_sbat_section(char *SBATBase, size_t SBATSize, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name)
 {
 	unsigned int i;
 	EFI_STATUS efi_status;
@@ -385,7 +385,7 @@ verify_sbat_section(char *SBATBase, size_t SBATSize)
 		       entries[i]->vendor_url);
 	}
 
-	efi_status = verify_sbat(n, entries);
+	efi_status = verify_sbat(n, entries, sbat_gen_expected, sbat_gen_found, sbat_component_name);
 
 	cleanup_sbat_section_entries(n, entries);
 

diff --git a/sbat.c b/sbat.c
@@ -131,10 +131,8 @@ cleanup_sbat_section_entries(size_t n, struct sbat_section_entry **entries)
 }
 
 EFI_STATUS
-verify_single_entry(struct sbat_section_entry *entry, struct sbat_var_entry *sbat_var_entry, bool *found)
+verify_single_entry(struct sbat_section_entry *entry, struct sbat_var_entry *sbat_var_entry, bool *found, UINT16 *sbat_gen, UINT16 *sbat_var_gen)
 {
-	UINT16 sbat_gen, sbat_var_gen;
-
 	if (strcmp((const char *)entry->component_name, (const char *)sbat_var_entry->component_name) == 0) {
 		dprint(L"component %a has a matching SBAT variable entry, verifying\n",
 			entry->component_name);
@@ -144,12 +142,12 @@ verify_single_entry(struct sbat_section_entry *entry, struct sbat_var_entry *sba
 		 * atoi returns zero for failed conversion, so essentially
 		 * badly parsed component_generation will be treated as zero
 		 */
-		sbat_gen = atoi((const char *)entry->component_generation);
-		sbat_var_gen = atoi((const char *)sbat_var_entry->component_generation);
+		*sbat_gen = atoi((const char *)entry->component_generation);
+		*sbat_var_gen = atoi((const char *)sbat_var_entry->component_generation);
 
-		if (sbat_gen < sbat_var_gen) {
+		if (*sbat_gen < *sbat_var_gen) {
 			dprint(L"component %a, generation %d, was revoked by %s variable\n",
-			       entry->component_name, sbat_gen, SBAT_VAR_NAME);
+			       entry->component_name, *sbat_gen, SBAT_VAR_NAME);
 			LogError(L"image did not pass SBAT verification\n");
 			return EFI_SECURITY_VIOLATION;
 		}
@@ -177,9 +175,10 @@ cleanup_sbat_var(list_t *entries)
 }
 
 EFI_STATUS
-verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry **entries)
+verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry **entries, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name)
 {
 	unsigned int i;
+	UINTN component_name_size;
 	list_t *pos = NULL;
 	EFI_STATUS efi_status = EFI_SUCCESS;
 	struct sbat_var_entry *sbat_var_entry;
@@ -192,26 +191,39 @@ verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry *
 	for (i = 0; i < n; i++) {
 		list_for_each(pos, local_sbat_var) {
 			bool found = false;
+			*sbat_gen_expected = 0;
+			*sbat_gen_found = 0;
 			sbat_var_entry = list_entry(pos, struct sbat_var_entry, list);
-			efi_status = verify_single_entry(entries[i], sbat_var_entry, &found);
-			if (EFI_ERROR(efi_status))
+			efi_status = verify_single_entry(entries[i], sbat_var_entry, &found, sbat_gen_found, sbat_gen_expected);
+			if (EFI_ERROR(efi_status)) {
+				if (found && efi_status == EFI_SECURITY_VIOLATION) {
+					component_name_size = strlen((const char *)entries[i]->component_name);
+					*sbat_component_name = AllocatePool(component_name_size + 1);
+					if (!*sbat_component_name) {
+						efi_status = EFI_OUT_OF_RESOURCES;
+						goto err;
+					}
+					memcpy(*sbat_component_name, (const char *)entries[i]->component_name, component_name_size + 1);
+				}
 				goto out;
+			}
 			if (found)
 				break;
 		}
 	}
 
 out:
 	dprint(L"finished verifying SBAT data: %r\n", efi_status);
+err:
 	return efi_status;
 }
 
 EFI_STATUS
-verify_sbat(size_t n, struct sbat_section_entry **entries)
+verify_sbat(size_t n, struct sbat_section_entry **entries, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name)
 {
 	EFI_STATUS efi_status;
 
-	efi_status = verify_sbat_helper(&sbat_var, n, entries);
+	efi_status = verify_sbat_helper(&sbat_var, n, entries,  sbat_gen_expected, sbat_gen_found, sbat_component_name);
 	return efi_status;
 }
 
@@ -559,9 +571,12 @@ set_sbat_uefi_variable(char *sbat_var_automatic, char *sbat_var_latest)
 		}
 
 #ifndef SHIM_UNIT_TEST
+		UINT16 sbat_gen_expected = 0;  
+		UINT16 sbat_gen_found = 0;
+		CHAR8 *sbat_component_name = NULL;
 		char *sbat_start = (char *)&_sbat;
 		char *sbat_end = (char *)&_esbat;
-		efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1);
+		efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name);
 		if (EFI_ERROR(efi_status)) {
 			CHAR16 *title = L"New SbatLevel would self-revoke current shim. Not applied";
 			CHAR16 *message = L"Press any key to continue";

diff --git a/shim.c b/shim.c
@@ -712,6 +712,10 @@ verify_buffer_sbat (char *data, int datasize,
 {
 	int i;
 	EFI_IMAGE_SECTION_HEADER *Section;
+	EFI_STATUS efi_status;
+	UINT16 sbat_gen_expected = 0;
+	UINT16 sbat_gen_found = 0;
+	CHAR8 *sbat_component_name = NULL;
 	char *SBATBase = NULL;
 	size_t SBATSize = 0;
 
@@ -759,7 +763,11 @@ verify_buffer_sbat (char *data, int datasize,
 		}
 	}
 
-	return verify_sbat_section(SBATBase, SBATSize);
+	efi_status = verify_sbat_section(SBATBase, SBATSize, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name);
+	if (sbat_component_name) {
+		FreePool(sbat_component_name);
+	}
+	return efi_status;
 }
 
 /*
@@ -1841,6 +1849,9 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 {
 	EFI_STATUS efi_status;
 	EFI_HANDLE image_handle;
+	UINT16 sbat_gen_expected = 0;
+	UINT16 sbat_gen_found = 0;
+	CHAR8 *sbat_component_name = NULL;
 
 	verification_method = VERIFIED_BY_NOTHING;
 
@@ -1927,7 +1938,7 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 			goto die;
 		}
 
-		efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1);
+		efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name);
 		if (EFI_ERROR(efi_status)) {
 			perror(L"Verifiying shim SBAT data failed: %r\n",
 			       efi_status);
@@ -1966,6 +1977,23 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 die:
 		console_print(L"Something has gone seriously wrong: %s: %r\n",
 			      msgs[msg], efi_status);
+
+		/*
+		 * Provide additional information when the SBAT self check fails
+		 */
+		if ( msg == SBAT_SELF_CHECK ) {
+			if (sbat_gen_expected == 0 &&  sbat_gen_found == 0 && sbat_component_name == NULL) {
+				// In this case something related to the .sbat section is wrong.
+				console_print(L"\n\nSomething went wrong validating SBAT. Either:\n"
+					"- The shim has no .sbat section or is corrupted\n"
+					"- Something went wrong internally");
+			} else {
+				console_print(L"\n\nYour shim has been revoked via SBAT. "
+				"Please update to a newer shim version.\n"
+				"For component \"%a\" expected at least level: %u. Current shim has: %u.\n",
+				sbat_component_name, sbat_gen_expected, sbat_gen_found);
+			}
+		}
 #if defined(ENABLE_SHIM_DEVEL)
 		devel_egress(COLD_RESET);
 #else
@@ -1974,6 +2002,9 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 				0, NULL);
 #endif
 	}
+	if (sbat_component_name) {
+		FreePool(sbat_component_name);
+	}
 
 	/*
 	 * This variable is supposed to be set by second stages, so ensure it is

diff --git a/test-sbat.c b/test-sbat.c
@@ -468,10 +468,19 @@ test_verify_sbat_null_sbat_section(void)
 	status = parse_sbat_var_data(&test_sbat_var, sbat_var_data, sizeof(sbat_var_data));
 	assert_equal_goto(status, EFI_SUCCESS, err, "got %#x expected %#x\n");
 
-	status = verify_sbat_helper(&test_sbat_var, n, entries);
+	UINT16 sbat_gen_expected = 0;
+	UINT16 sbat_gen_found = 0;
+	CHAR8 *sbat_component_name = NULL;
+	status = verify_sbat_helper(&test_sbat_var, n, entries, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name);
+	assert_equal_goto(sbat_component_name, NULL, err, "got %#x expected %#x\n");
+	assert_equal_goto(sbat_gen_expected, 0, err, "got %#x expected %#x\n");
+	assert_equal_goto(sbat_gen_found, 0, err, "got %#x expected %#x\n");
 	assert_equal_goto(status, EFI_SUCCESS, err, "got %#x expected %#x\n");
 	rc = 0;
 err:
+	if (sbat_component_name) {
+		free(sbat_component_name);
+	}
 	cleanup_sbat_var(&test_sbat_var);
 
 	return rc;
@@ -973,12 +982,22 @@ test_parse_and_verify(void)
 	if (status != EFI_SUCCESS || list_empty(&sbat_var))
 		return -1;
 
-	status = verify_sbat(n_section_entries, section_entries);
+	UINT16 sbat_gen_expected = 0;
+	UINT16 sbat_gen_found = 0;
+	CHAR8 *sbat_component_name = NULL;
+	status = verify_sbat(n_section_entries, section_entries, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name);
+	assert_nonzero_goto(sbat_component_name, err, "not expected component name to be NULL");
+	assert_goto(strcmp(sbat_component_name, "test1") == 0, err, "expected 'test1' got '%s'", sbat_component_name);
+	assert_equal_goto(sbat_gen_expected, 5, err, "expected %#x got %#x\n");
+	assert_equal_goto(sbat_gen_found, 1, err, "expected %#x got %#x\n");
 	assert_equal_goto(status, EFI_SECURITY_VIOLATION, err, "expected %#x got %#x\n");
 
 	rc = 0;
 err:
 	cleanup_sbat_section_entries(n_section_entries, section_entries);
+	if (sbat_component_name) {
+		free(sbat_component_name);
+	}
 	cleanup_sbat_var(&sbat_var);
 
 	return rc;