set_sbat_uefi_variable(): avoid comparing unsafe data

A few cleanups:
- Ensure that the data we get from get_variable() is at least big enough
  to actually be an SBAT variable
- Only try to delete if the variable is actually set
- Don't set the variable again if deleting it failed
- We don't actually need to get the size of the variable, allocate,
  and then get the variable; get_variable() does the allocation for us.
- Don't compare the variable data when get_variable() failed

Signed-off-by: Peter Jones <pjones@redhat.com>

1 files changed, 28 insertions, 45 deletions

diff --git a/sbat.c b/sbat.c
index 3f67136b..77b6f5ab 100644
--- a/sbat.c
+++ b/sbat.c
@@ -302,75 +302,58 @@ set_sbat_uefi_variable(void)
 	UINT8 *sbat = NULL;
 	UINTN sbatsize = 0;
 
-	efi_status = get_variable_size(L"SBAT", SHIM_LOCK_GUID, &sbatsize);
-	if (EFI_ERROR(efi_status))
-		dprint(L"SBAT size probe failed %r\n", efi_status);
-
-	if (sbatsize != 0) {
-		sbat = AllocateZeroPool(sbatsize + 1);
-		if (!sbat)
-			return EFI_OUT_OF_RESOURCES;
-
-		efi_status = get_variable_attr(L"SBAT", &sbat, &sbatsize,
-				SHIM_LOCK_GUID, &attributes);
-	}
-
+	efi_status = get_variable_attr(L"SBAT", &sbat, &sbatsize,
+				       SHIM_LOCK_GUID, &attributes);
 	/*
 	 * Always set the SBAT UEFI variable if it fails to read.
 	 *
-	 * Don't try to set the SBAT UEFI variable if attributes match, the
-	 * signature matches and it has the same or newer version.
+	 * Don't try to set the SBAT UEFI variable if attributes match and
+	 * the signature matches.
 	 */
 	if (EFI_ERROR(efi_status)) {
 		dprint(L"SBAT read failed %r\n", efi_status);
 	} else if ((attributes == UEFI_VAR_NV_BS ||
-				attributes == UEFI_VAR_NV_BS_TIMEAUTH) &&
-                   strncmp(sbat, SBAT_VAR_SIG, strlen(SBAT_VAR_SIG)) == 0) {
+	            attributes == UEFI_VAR_NV_BS_TIMEAUTH) &&
+	           sbatsize >= strlen(SBAT_VAR_SIG "1") &&
+	           strncmp((const char *)sbat, SBAT_VAR_SIG,
+	                   strlen(SBAT_VAR_SIG))) {
 		FreePool(sbat);
 		return EFI_SUCCESS;
-	}
-
-	/* delete previous variable */
-	efi_status = set_variable(L"SBAT", SHIM_LOCK_GUID, attributes, 0, "");
-	if (EFI_ERROR(efi_status))
-		dprint(L"SBAT variable delete failed %r\n", efi_status);
+	} else {
+		FreePool(sbat);
 
-	/* verify that it's gone */
-	efi_status = get_variable(L"SBAT", &sbat, &sbatsize, SHIM_LOCK_GUID);
-	if (EFI_ERROR(efi_status) || (sbatsize != 0))
-		dprint(L"SBAT variable clearing failed %r\n", efi_status);
+		/* delete previous variable */
+		efi_status = set_variable(L"SBAT", SHIM_LOCK_GUID, attributes, 0, "");
+		if (EFI_ERROR(efi_status)) {
+			dprint(L"SBAT variable delete failed %r\n", efi_status);
+			return efi_status;
+		}
+	}
 
 	/* set variable */
-	efi_status = set_variable(L"SBAT", SHIM_LOCK_GUID,
-			UEFI_VAR_NV_BS, sizeof (SBAT_VAR), SBAT_VAR);
-	if (EFI_ERROR(efi_status))
+	efi_status = set_variable(L"SBAT", SHIM_LOCK_GUID, UEFI_VAR_NV_BS,
+	                          sizeof(SBAT_VAR), SBAT_VAR);
+	if (EFI_ERROR(efi_status)) {
 		dprint(L"SBAT variable writing failed %r\n", efi_status);
-
-	FreePool(sbat);
-
-	/* verify that the expected data is there */
-	efi_status = get_variable_size(L"SBAT", SHIM_LOCK_GUID, &sbatsize);
-	if (EFI_ERROR(efi_status) || sbatsize == 0) {
-		dprint(L"SBAT read failed %r\n", efi_status);
-		return EFI_INVALID_PARAMETER;
+		return efi_status;
 	}
 
-	sbat = AllocateZeroPool(sbatsize);
-	if (!sbat)
-		return EFI_OUT_OF_RESOURCES;
-
+	/* verify that the expected data is there */
 	efi_status = get_variable(L"SBAT", &sbat, &sbatsize, SHIM_LOCK_GUID);
-	if (EFI_ERROR(efi_status))
+	if (EFI_ERROR(efi_status)) {
 		dprint(L"SBAT read failed %r\n", efi_status);
+		return efi_status;
+	}
 
-	if (strncmp(sbat, SBAT_VAR, strlen(SBAT_VAR)) != 0) {
+	if (sbatsize != strlen(SBAT_VAR) ||
+	    strncmp((const char *)sbat, SBAT_VAR, strlen(SBAT_VAR)) != 0) {
 		efi_status = EFI_INVALID_PARAMETER;
 	} else {
 		dprint(L"SBAT variable initialization succeeded\n");
-		efi_status = EFI_SUCCESS;
 	}
 
 	FreePool(sbat);
+
 	return efi_status;
 }