Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table

[Bartosz Szczepanek]

Nope, it doesn't work. It compiled (after correcting one more leftover
mapping), but panicked the same way.

I've came up with a set of changes that make it working in my setup,
see attached patch. There was a problem with passing already remapped
address to tpm2_calc_event_log_size(), which tried to remap it for
second time. Few more adjustments were needed in remap-as-you-go code
so that new address is used consequently. Also, I've removed remapping
digest itself because it was never used.

I'm not certain these changes make it 100% correct, but it worked on
35 events I had in final log. Its ending seems fine now:
> [root@localhost ~]# hexdump -C /sys/kernel/security/tpm0/binary_bios_measurements | tail
> 00003720  42 6f 6f 74 20 53 65 72  76 69 63 65 73 20 49 6e  |Boot Services In|
> 00003730  76 6f 63 61 74 69 6f 6e  05 00 00 00 07 00 00 80  |vocation........|
> 00003740  02 00 00 00 04 00 47 55  45 dd c9 78 d7 bf d0 36  |......GUE..x...6|
> 00003750  fa cc 7e 2e 98 7f 48 18  9f 0d 0b 00 b5 4f 75 42  |..~...H......OuB|
> 00003760  cb d8 72 a8 1a 9d 9d ea  83 9b 2b 8d 74 7c 7e bd  |..r.......+.t|~.|
> 00003770  5e a6 61 5c 40 f4 2f 44  a6 db eb a0 28 00 00 00  |^.a\@./D....(...|
> 00003780  45 78 69 74 20 42 6f 6f  74 20 53 65 72 76 69 63  |Exit Boot Servic|
> 00003790  65 73 20 52 65 74 75 72  6e 65 64 20 77 69 74 68  |es Returned with|
> 000037a0  20 53 75 63 63 65 73 73                           | Success|
> 000037a8

Still, some refactoring could help here as __calc_tpm2_event_size has
grown and its logic became hard to follow. IMO it's far too complex
for inline function.

Attached patch should be applied on top of jjs/master.

Bartosz
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index fe48150f06d1..2c912ea08166 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
 		if (event_size == 0)
 			return -1;
 		size += event_size;
+		count--;
 	}
 
 	return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
 	struct linux_efi_tpm_eventlog *log_tbl;
 	struct efi_tcg2_final_events_table *final_tbl;
 	unsigned int tbl_size;
+	int ret = 0;
 
 	if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
 		/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
 
 	tbl_size = sizeof(*log_tbl) + log_tbl->size;
 	memblock_reserve(efi.tpm_log, tbl_size);
-	early_memunmap(log_tbl, sizeof(*log_tbl));
 
 	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
-		return 0;
+		goto out;
 
 	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
 
@@ -71,17 +72,22 @@ int __init efi_tpm_eventlog_init(void)
 		pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
 		       efi.tpm_final_log);
 		efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
-	tbl_size = tpm2_calc_event_log_size(final_tbl->events,
+	tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
+					    + sizeof(final_tbl->version)
+					    + sizeof(final_tbl->nr_events),
 					    final_tbl->nr_events,
-					    (void *)efi.tpm_log);
+					    log_tbl->log);
 	memblock_reserve((unsigned long)final_tbl,
 			 tbl_size + sizeof(*final_tbl));
 	early_memunmap(final_tbl, sizeof(*final_tbl));
 	efi_tpm_final_log_size = tbl_size;
 
-	return 0;
+out:
+	early_memunmap(log_tbl, sizeof(*log_tbl));
+	return ret;
 }
 
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 0ca27bc053af..63238c84dc0b 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -185,8 +185,12 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 			size = 0;
 			goto out;
 		}
+	} else {
+		mapping = marker_start;
 	}
 
+	event = (struct tcg_pcr_event2_head *)mapping;
+
 	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
 
 	/* Check if event is malformed. */
@@ -201,34 +205,24 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 		/* Map the digest's algorithm identifier */
 		if (do_mapping) {
 			TPM_MEMUNMAP(mapping, mapping_size);
-			mapping_size = marker - marker_start + halg_size;
-			mapping = TPM_MEMREMAP((unsigned long)marker_start,
-					       mapping_size);
+			mapping_size = halg_size;
+			mapping = TPM_MEMREMAP((unsigned long)marker,
+					     mapping_size);
 			if (!mapping) {
 				size = 0;
 				goto out;
 			}
+		} else {
+			mapping = marker;
 		}
 
-		memcpy(&halg, marker, halg_size);
+		memcpy(&halg, mapping, halg_size);
 		marker = marker + halg_size;
 
 		for (j = 0; j < efispecid->num_algs; j++) {
 			if (halg == efispecid->digest_sizes[j].alg_id) {
 				marker +=
 					efispecid->digest_sizes[j].digest_size;
-
-				/* Map the digest content itself */
-				if (do_mapping) {
-					TPM_MEMUNMAP(mapping, mapping_size);
-					mapping_size = marker - marker_start;
-					mapping = TPM_MEMREMAP((unsigned long)marker_start,
-							       mapping_size);
-					if (!mapping) {
-						size = 0;
-						goto out;
-					}
-				}
 				break;
 			}
 		}
@@ -239,23 +233,25 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 		}
 	}
 
-	event_field = (struct tcg_event_field *)marker;
-
 	/*
 	 * Map the event size - we don't read from the event itself, so
 	 * we don't need to map it
 	 */
 	if (do_mapping) {
-		TPM_MEMUNMAP(marker_start, mapping_size);
+		TPM_MEMUNMAP(mapping, mapping_size);
 		mapping_size += sizeof(event_field->event_size);
-		mapping = TPM_MEMREMAP((unsigned long)marker_start,
+		mapping = TPM_MEMREMAP((unsigned long)marker,
 				       mapping_size);
 		if (!mapping) {
 			size = 0;
 			goto out;
 		}
+	} else {
+		mapping = marker;
 	}
 
+	event_field = (struct tcg_event_field *)mapping;
+
 	marker = marker + sizeof(event_field->event_size)
 		+ event_field->event_size;
 	size = marker - marker_start;