intel_txt: fix the build errors of intel_txt patch on non-X86 platforms (resend)

This patch added verification for userspace memory integrity after s3 resume. Integrity verification for other memory (say kernel itself) has been done by tboot.

arch/x86/kernel/tboot.c +19 -0

+	for_each_online_pgdat(pgdat) {
+		for (i = 0, pfn = pgdat->node_start_pfn;
+			i < pgdat->node_spanned_pages;
+			i++, pfn = pgdat->node_start_pfn + i) {
+
+			if (!pfn_valid(pfn) || !page_is_ram(pfn))
+				continue;
+
+			page = pfn_to_page(pfn);
+			paddr = page_to_phys(page);
+
+			/* If pg will be MACed by tboot, no need to MAC here */
+			for (j = 0; j < tboot->num_mac_regions; j++) {
+				rstart = tboot->mac_regions[j].start;
+				rend = rstart +	tboot->mac_regions[j].size;
+				if (((paddr + PAGE_SIZE) <= rstart)
+					|| (rend <= paddr))
+					continue;
+				break;

arch/x86/kernel/tboot.c +10 -0

@@ -197,6 +277,16 @@ static int tboot_setup_sleep(void)
 	/* kernel code + data + bss */
 	add_mac_region(virt_to_phys(_text), _end - _text);
 
+	/* stack */
+	add_mac_region(virt_to_phys(current_thread_info()), THREAD_SIZE);
+
+	/* MAC userspace memory not handled by tboot */
+	get_random_bytes(tboot->s3_key, sizeof(tboot->s3_key));
+	if (tboot_gen_mem_integrity(tboot->s3_key, &mem_mac)) {
+		panic("tboot: vmac generation failed\n");
+		return -1;
+	}
+
 	tboot->acpi_sinfo.kernel_s3_resume_vector = acpi_wakeup_address;
 
 	return 0;

arch/x86/kernel/tboot.c +5 -0

+#ifdef CONFIG_X86_32
+	asm volatile (
+		" and %0, %%esp;        "
+		" add %1, %%esp;        "
+		: : "i" (THREAD_SIZE - 1), "r" (new_ti));

So I corrupt memory, but also corrupt tboot_enabled() to return 0....

And... does panic kill the machine quickly enough that no 'bad stuff' happens? (Whats bad stuff in this context, anyway?).

Quoted 4 lines · Shane Wang

This patch added verification for userspace memory integrity after s3 resume. Integrity verification for other memory (say kernel itself) has been done by tboot.

AFAICT, it verifies userspace _and_ kernel memory, that's why it does magic stack switching. Why not verify everything in tboot?

Is kernel<->tboot abi described somewhere?

arch/x86/kernel/tboot.c static void tboot_create_trampoline(void · line 177

Quoted 5 lines

arch/x86/kernel/tboot.c +2 -0

@@ -168,6 +174,80 @@ static void tboot_create_trampoline(void  		   
 map_base, map_size); }
 
+static vmac_t mem_mac;
+static struct crypto_hash *tfm;

Could these be automatic?

Quoted 6 lines

arch/x86/kernel/tboot.c +6 -0

+void tboot_sx_resume(void)
+{
+	vmac_t mac;
+
+	if (!tboot_enabled())
+		return;

arch/x86/kernel/tboot.c void tboot_sleep(u8 sleep_state, u32 pm1 · line 461

Quoted 6 lines

arch/x86/kernel/tboot.c +6 -0

+
+	if (tboot_gen_mem_integrity(tboot->s3_key, &mac))
+		panic("tboot: vmac generation failed\n");
+	else if (mac != mem_mac)
+		panic("tboot: memory integrity was lost on resume\n"); +	else
+		pr_info("memory integrity OK\n");

So I corrupt memory, but also corrupt tboot_enabled() to return 0....

And... does panic kill the machine quickly enough that no 'bad stuff' happens? (Whats bad stuff in this context, anyway?).

Quoted 4 lines · Shane Wang

This patch added verification for userspace memory integrity after s3 resume. Integrity verification for other memory (say kernel itself) has been done by tboot.

AFAICT, it verifies userspace _and_ kernel memory, that's why it does magic stack switching. Why not verify everything in tboot?

arch/x86/kernel/tboot.c static void tboot_create_trampoline(void · line 177

Quoted 5 lines

arch/x86/kernel/tboot.c +2 -0

@@ -168,6 +174,80 @@ static void tboot_create_trampoline(void  		   
 map_base, map_size); }
 
+static vmac_t mem_mac;
+static struct crypto_hash *tfm;

Could these be automatic?

Quoted 23 lines

arch/x86/kernel/tboot.c +22 -0

+	for_each_online_pgdat(pgdat) {
+		for (i = 0, pfn = pgdat->node_start_pfn;
+			i < pgdat->node_spanned_pages;
+			i++, pfn = pgdat->node_start_pfn + i) {
+
+			if (!pfn_valid(pfn) || !page_is_ram(pfn))
+				continue;
+
+			page = pfn_to_page(pfn);
+			paddr = page_to_phys(page);
+
+			/* If pg will be MACed by tboot, no need to MAC here */
+			for (j = 0; j < tboot->num_mac_regions; j++) {
+				rstart = tboot->mac_regions[j].start;
+				rend = rstart +	tboot->mac_regions[j].size;
+				if (((paddr + PAGE_SIZE) <= rstart)
+					|| (rend <= paddr))
+					continue;
+				break;
+			}
+
+			if (j == tboot->num_mac_regions) {

arch/x86/kernel/tboot.c static void tboot_create_trampoline(void · line 239

Quoted 6 lines

arch/x86/kernel/tboot.c +6 -0

+				sg_set_page(sg, page, PAGE_SIZE, 0);
+				ret = crypto_hash_update(&desc, sg, PAGE_SIZE); +				if (ret)
+					return ret;
+			}
+		}
+	}

This looks over all memory, afaict. Does it correctly handle highmem?

Quoted 19 lines

arch/x86/kernel/tboot.c +9 -0

@@ -197,6 +277,16 @@ static int tboot_setup_sleep(void)
 	/* kernel code + data + bss */
 	add_mac_region(virt_to_phys(_text), _end - _text);
 
+	/* stack */
+	add_mac_region(virt_to_phys(current_thread_info()), THREAD_SIZE); +
+	/* MAC userspace memory not handled by tboot */
+	get_random_bytes(tboot->s3_key, sizeof(tboot->s3_key));
+	if (tboot_gen_mem_integrity(tboot->s3_key, &mem_mac)) {
+		panic("tboot: vmac generation failed\n");
+		return -1;
+	}
+
 	tboot->acpi_sinfo.kernel_s3_resume_vector = acpi_wakeup_address;
 
 	return 0;

@@ -212,6 +302,68 @@ static int tboot_setup_sleep(void)  }
 

arch/x86/kernel/tboot.c static int tboot_setup_sleep(void) · line 311

Quoted 8 lines

arch/x86/kernel/tboot.c +7 -0

 #endif
+
+static struct thread_info *low_ti, *current_ti;
+
+static void tboot_do_stack_switch(struct thread_info *new_ti,
+				struct thread_info *old_ti)
+{
+	memcpy(new_ti, old_ti, THREAD_SIZE);

Memcpy does some rather odd stuff with fpu in some configs. Like... you may get copy of thread info with fpu marked enabled, while it really is disabled when memcpy exits.

Quoted 10 lines

arch/x86/kernel/tboot.c +9 -0

+#ifdef CONFIG_X86_32
+	asm volatile (
+		" and %0, %%esp;        "
+		" add %1, %%esp;        "
+		: : "i" (THREAD_SIZE - 1), "r" (new_ti));
+#else
+	asm volatile (
+		" and %0, %%rsp;        "
+		" add %1, %%rsp;        "

arch/x86/kernel/tboot.c static int tboot_setup_sleep(void) · line 326

Quoted 6 lines

arch/x86/kernel/tboot.c +6 -0

+		: : "i" (THREAD_SIZE - 1), "r" (new_ti));
+	percpu_write(kernel_stack, (unsigned long)new_ti -
+				KERNEL_STACK_OFFSET + THREAD_SIZE);
+#endif
+	current->stack = new_ti;
+}

This is pretty subtle&fragile -- see memcpy above. Why is it needed?

arch/x86/kernel/tboot.c static int tboot_setup_sleep(void) · line 339

Quoted 8 lines

arch/x86/kernel/tboot.c +7 -0

+	current_ti = current_thread_info();
+
+	/* If thread info is above 4G, then switch stack */
+	if (!((PFN_PHYS(PFN_DOWN(virt_to_phys(current_ti)))
+		+ (PFN_UP(THREAD_SIZE) << PAGE_SHIFT))
+		& 0xffffffff00000000ULL))
+		return;

How can thread info be above 4G on 32-bit?

Why does 4G limit matter on 64-bit?

arch/x86/kernel/tboot.c void tboot_sleep(u8 sleep_state, u32 pm1 · line 461

Quoted 13 lines

arch/x86/kernel/tboot.c +12 -0

+void tboot_sx_resume(void)
+{
+	vmac_t mac;
+
+	if (!tboot_enabled())
+		return;
+
+	if (tboot_gen_mem_integrity(tboot->s3_key, &mac))
+		panic("tboot: vmac generation failed\n");
+	else if (mac != mem_mac)
+		panic("tboot: memory integrity was lost on resume\n"); +	else
+		pr_info("memory integrity OK\n");

So I corrupt memory, but also corrupt tboot_enabled() to return 0....

And... does panic kill the machine quickly enough that no 'bad stuff' happens? (Whats bad stuff in this context, anyway?).

Quoted 3 lines

(quoted diff) +0 -0

@@ -244,7 +245,10 @@ static int acpi_suspend_enter(suspend_st 
 break; 
 

drivers/acpi/sleep.c static int acpi_suspend_enter(suspend_st · line 254

Quoted 8 lines

drivers/acpi/sleep.c +3 -0

 	case ACPI_STATE_S3:
+		tboot_switch_stack();
 		do_suspend_lowlevel();
+		tboot_sx_resume();
+		tboot_restore_stack();
 		break;
 	}
 

Did you audit all code before sx_resume()? If it trusts data not checksummed by tboot, attacker may be able to hijack code execution and bypass your protection, no?

What about S1?

And what about hibernation, btw

Pavel

Quoted 2 lines

AFAICT, it verifies userspace _and_ kernel memory, that's why it does magic stack switching. Why not verify everything in tboot?

Because tboot only can access <4G mem without paging. And the memory is sparse. We can't/needn't set unlimited sparse mem ranges to the MAC array with limited elements in the shared page, in order to pass the parameters. On the other hand, it is reasonable for tboot to verify kernel, and kernel to verify userspace memory.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static vmac_t mem_mac;
+static struct crypto_hash *tfm;

Could these be automatic?

Maybe, but I don't wish other files can access the variables and take tfm as an example, I'd like to allocate memory to it once and then initialize it once in order to avoid impact of memory change to MACing.

Quoted 1 line

Why does 4G limit matter on 64-bit?

tboot can't access >4G, see above.

Quoted 10 lines

arch/x86/kernel/tboot.c void tboot_sleep(u8 sleep_state, u32 pm1 · line 461

Quoted 5 lines

arch/x86/kernel/tboot.c +5 -0

+	if (tboot_gen_mem_integrity(tboot->s3_key, &mac))
+		panic("tboot: vmac generation failed\n");
+	else if (mac != mem_mac)
+		panic("tboot: memory integrity was lost on resume\n"); +	else
+		pr_info("memory integrity OK\n");

So I corrupt memory, but also corrupt tboot_enabled() to return 0....

And... does panic kill the machine quickly enough that no 'bad stuff' happens? (Whats bad stuff in this context, anyway?).

Quoted 15 lines

drivers/acpi/sleep.c static int acpi_suspend_enter(suspend_st · line 254

Quoted 11 lines

drivers/acpi/sleep.c +3 -0

@@ -244,7 +245,10 @@ static int acpi_suspend_enter(suspend_st 
 break; 
 
 	case ACPI_STATE_S3:
+		tboot_switch_stack();
 		do_suspend_lowlevel();
+		tboot_sx_resume();
+		tboot_restore_stack();
 		break;
 	}
 

Did you audit all code before sx_resume()? If it trusts data not checksummed by tboot, attacker may be able to hijack code execution and bypass your protection, no?

Yes, kernel code is audited by tboot before resume.

From: Pavel Machek [mailto:[email protected]]
Sent: Friday, December 04, 2009 12:20 AM

Hi!

Please wrap mails at column 72 (or so).

Quoted 3 lines

Quoted 2 lines

AFAICT, it verifies userspace _and_ kernel memory, that's why it does magic stack switching. Why not verify everything in tboot?

Because tboot only can access <4G mem without paging. And the memory is sparse. We

can't/needn't set unlimited sparse mem ranges to the MAC array with limited elements in the shared page, in order to pass the parameters.

Quoted 1 line

On the other hand, it is reasonable for tboot to verify kernel, and kernel to verify

userspace memory.

Quoted 1 line

Are you sure x86-64 kernel & modules is always below 4GB? I don't think so.

Quoted 5 lines

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static vmac_t mem_mac;
+static struct crypto_hash *tfm;

Could these be automatic?

Maybe, but I don't wish other files can access the variables and take tfm as an example, I'd

like to allocate memory to it once and then initialize it once in order to avoid impact of memory change to MACing.

Quoted 1 line

You use stack, anyway.

Quoted 2 lines

Quoted 1 line

Why does 4G limit matter on 64-bit?

tboot can't access >4G, see above.

Too bad, then its broken by design.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

arch/x86/kernel/tboot.c +5 -0

+        if (tboot_gen_mem_integrity(tboot->s3_key, &mac))
+                panic("tboot: vmac generation failed\n");
+        else if (mac != mem_mac)
+                panic("tboot: memory integrity was lost on resume\n"); +        else
+                pr_info("memory integrity OK\n");

Quoted 4 lines

Quoted 4 lines

So I corrupt memory, but also corrupt tboot_enabled() to return 0....

And... does panic kill the machine quickly enough that no 'bad stuff' happens? (Whats bad stuff in this context, anyway?).

I'd really like you to answer that.

Quoted 15 lines

Quoted 15 lines

drivers/acpi/sleep.c static int acpi_suspend_enter(suspend_st · line 254

Quoted 11 lines

drivers/acpi/sleep.c +3 -0

@@ -244,7 +245,10 @@ static int acpi_suspend_enter(suspend_st
 break;
 
         case ACPI_STATE_S3:
+                tboot_switch_stack();
                 do_suspend_lowlevel();
+                tboot_sx_resume();
+                tboot_restore_stack();
                 break;
         }
 

Did you audit all code before sx_resume()? If it trusts data not checksummed by tboot, attacker may be able to hijack code execution and bypass your protection, no?

Quoted 1 line

Yes, kernel code is audited by tboot before resume.

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

"bad stuff" would be the execution of any code (or use of any data that affects execution) that was not verified by tboot. As long as panic() is within the code ranges MAC'ed by tboot (see above), it would be covered. Do you know of some panic() code paths that are outside of this?

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

checksummed by tboot, attacker may be able to hijack code execution and bypass your protection, no?

Yes, kernel code is audited by tboot before resume.

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

We aren't aware of any code or data used by the resume path that is outside of the tboot-MAC'ed regions above--if you can point out any then we will gladly address them.

From: Andi Kleen [mailto:[email protected]]
Sent: Friday, December 04, 2009 9:14 AM

Quoted 1 line

"bad stuff" would be the execution of any code (or use of any data that affects execution)

that was not verified by tboot. As long as panic() is within the code ranges MAC'ed by tboot (see above), it would be covered. Do you know of some panic() code paths that are outside of this?

Not code path, but the code called by panic (console drivers, debuggers etc.) can well use data that is stored >4GB

This can include structures with indirect pointers, like notifier chains.

Notifier chains have a special checker than can check for <4GB, but there are other call vectors too.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

checksummed by tboot, attacker may be able to hijack code execution and bypass your protection, no?

Yes, kernel code is audited by tboot before resume.

Quoted 4 lines

Quoted 2 lines

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

We aren't aware of any code or data used by the resume path that is outside of the tboot-

MAC'ed regions above--if you can point out any then we will gladly address them.

Code coverage is not enough, you need data coverage too. If someone modifies kernel data it's typically easy to subvert code as a next step.

-Andi
--
[email protected] -- Speaking for myself only.

Quoted 11 lines

From: Andi Kleen [mailto:[email protected]]
Sent: Friday, December 04, 2009 9:14 AM

Quoted 1 line

"bad stuff" would be the execution of any code (or use of any data that affects execution)

that was not verified by tboot. As long as panic() is within the code ranges MAC'ed by tboot (see above), it would be covered. Do you know of some panic() code paths that are outside of this?

Not code path, but the code called by panic (console drivers, debuggers etc.) can well use data that is stored >4GB

Quoted 4 lines

This can include structures with indirect pointers, like notifier chains.

Notifier chains have a special checker than can check for <4GB, but there are other call vectors too.

Since, as you pointed out in a previous email, it is doubtful that there will be any user-visible output at this point, we can change this path to a tboot reset (which will give us some serial output at least). Is it going to be similarly unsafe to do a printk()?

From: Andi Kleen [mailto:[email protected]]
Sent: Friday, December 04, 2009 12:10 PM

Quoted 4 lines · Joseph

Quoted 4 lines

From: Andi Kleen [mailto:[email protected]]
Sent: Friday, December 04, 2009 9:14 AM

Quoted 1 line

"bad stuff" would be the execution of any code (or use of any data that affects

execution)

Quoted 1 line

Quoted 1 line

that was not verified by tboot. As long as panic() is within the code ranges MAC'ed by

tboot

Quoted 1 line

Quoted 1 line

(see above), it would be covered. Do you know of some panic() code paths that are outside

of

Quoted 11 lines

Quoted 9 lines

this?

Not code path, but the code called by panic (console drivers, debuggers etc.) can well use data that is stored >4GB

This can include structures with indirect pointers, like notifier chains.

Notifier chains have a special checker than can check for <4GB, but there are other call vectors too.

Since, as you pointed out in a previous email, it is doubtful that there will be any user-

visible output at this point, we can change this path to a tboot reset (which will give us some serial output at least). Is it going to be similarly unsafe to do a printk()?

Yes printk is similarly unsafe. It calls all the console machinery, which has a lot of data.

Perhaps early_printk(), that is relatively self contained, but doesn't always work.

Of course you would need to have a timeout before reset, and at this point the delay loops are not calibrated yet, so you don't know how to wait.

-Andi

--
[email protected] -- Speaking for myself only.

Quoted 2 lines

Of course you would need to have a timeout before reset, and at this point the delay loops are not calibrated yet, so you don't know how to wait.

I would expect that early_printk() coupled with tboot's serial output would be sufficient for a case such as this. If we've done our work correctly, loss of integrity should only occur when the system is attacked across the S3 transition--which should be fairly rare and which should place a premium on prevention of the attacked code from executing. Esp. on servers, there may not be anyone to see console output anyway. Does early_printk() and a tboot reset seem like a reasonable approach?

I would expect that early_printk() coupled with tboot's serial output would be sufficient for a case such as this. If we've done our work correctly, loss of integrity should only occur when the system is attacked across the S3 transition--which should be fairly rare and which should place a premium on prevention of the attacked code from executing. Esp. on servers, there may not be anyone to see console output anyway. Does early_printk() and a tboot reset seem like a reasonable approach?

Quoted 5 lines

Quoted 3 lines

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

We aren't aware of any code or data used by the resume path that is outside of the tboot-MAC'ed regions above--if you can point out any then we will gladly address them.

Code coverage is not enough, you need data coverage too. If someone modifies kernel data it's typically easy to subvert code as a next step.

Quoted 6 lines · Andi Kleen

Quoted 5 lines

Quoted 3 lines

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

We aren't aware of any code or data used by the resume path that is outside of the tboot-MAC'ed regions above--if you can point out any then we will gladly address them.

Quoted 3 lines

Code coverage is not enough, you need data coverage too. If someone modifies kernel data it's typically easy to subvert code as a next step.

The only function pointers that are invoked on the do_suspend_lowlevel path are some paravirt_crap pointers, but those are located inside kernel static data.

Quoted 6 lines · Andi Kleen

Quoted 5 lines

Quoted 3 lines

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

We aren't aware of any code or data used by the resume path that is outside of the tboot-MAC'ed regions above--if you can point out any then we will gladly address them.

Quoted 3 lines

Code coverage is not enough, you need data coverage too. If someone modifies kernel data it's typically easy to subvert code as a next step.

The only function pointers that are invoked on the do_suspend_lowlevel path are some paravirt_crap pointers, but those are located inside kernel static data.

Quoted 10 lines

From: Pavel Machek [mailto:[email protected]]
Sent: Friday, December 04, 2009 12:20 AM

Hi!

Please wrap mails at column 72 (or so).

Quoted 3 lines

Quoted 2 lines

AFAICT, it verifies userspace _and_ kernel memory, that's why it does magic stack switching. Why not verify everything in tboot?

Because tboot only can access <4G mem without paging. And the memory is sparse. We

Quoted 8 lines

can't/needn't set unlimited sparse mem ranges to the MAC array with limited elements in the shared page, in order to pass the parameters.

Quoted 1 line

On the other hand, it is reasonable for tboot to verify kernel, and kernel to verify

userspace memory.

Quoted 1 line

Are you sure x86-64 kernel & modules is always below 4GB? I don't think so.

The only part of the kernel that really needs to be below 4GB is what is used by the code on resume to do the MAC verification of the remainder of the memory. This is all static code and variables. The regions we pass to tboot for MAC'ing are:

S3 real mode resume code:  acpi_wakeup_address, WAKEUP_SIZE
AP trampoline code:  virt_to_phys(trampoline_base), TRAMPOLINE_SIZE
kernel static code and data:  virt_to_phys(_text), _end - _text

the stack we use (either new one or existing depending--see code):
virt_to_phys(new_stack), IRQ_STACK_SIZE virt_to_phys(current_thread_info()), THREAD_SIZE

Can you post a .config and platform configuration for which the tboot, MAC, or do_suspend_lowlevel() are not within the regions above? We'll be happy to address the issue once we understand the conditions.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static vmac_t mem_mac;
+static struct crypto_hash *tfm;

Could these be automatic?

Quoted 6 lines

Quoted 1 line

Maybe, but I don't wish other files can access the variables and take tfm as an example, I'd

like to allocate memory to it once and then initialize it once in order to avoid impact of memory change to MACing.

Quoted 1 line

You use stack, anyway.

Do you mean as static local variables? If so, I'm not sure if that would be any better.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

So I corrupt memory, but also corrupt tboot_enabled() to return 0....

And... does panic kill the machine quickly enough that no 'bad stuff' happens? (Whats bad stuff in this context, anyway?).

I'd really like you to answer that.

"bad stuff" would be the execution of any code (or use of any data that affects execution) that was not verified by tboot. As long as panic() is within the code ranges MAC'ed by tboot (see above), it would be covered. Do you know of some panic() code paths that are outside of this?

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Did you audit all code before sx_resume()? If it trusts data not checksummed by tboot, attacker may be able to hijack code execution and bypass your protection, no?

Yes, kernel code is audited by tboot before resume.

So no, you did not audit do_suspend_lowlevel to make sure it does not follow function pointers. Bad.

We aren't aware of any code or data used by the resume path that is outside of the tboot-MAC'ed regions above--if you can point out any then we will gladly address them.

Quoted 3 lines

Quoted 3 lines

Are you sure x86-64 kernel & modules is always below 4GB? I don't think so.

Quoted 3 lines · Pavel Machek

Quoted 3 lines

Quoted 3 lines

Are you sure x86-64 kernel & modules is always below 4GB? I don't think so.

The x86-64 kernel is run where it is loaded by the boot loader. For most boot loaders, that will mean < 4 GB. This is not the case for modules, and they cannot and should not rely on modules inside restricted zone.

This effectively becomes a constraint on whatever boot loader is used to load the kernel for it to be compatible with tboot.

Having "security" technology that silently fails with funny bootloader is pretty bad, I'd say.

v2: Based on a complexity analysis and tradeoff, we moved all MAC'ing into tboot.

This patch adds support for S3 memory integrity protection within an Intel(R) TXT launched kernel, for all kernel and userspace memory. All RAM used by the kernel and userspace, as indicated by memory ranges of type E820_RAM and E820_RESERVED_KERN in the e820 table, will be integrity protected.

The MAINTAINERS file is also updated to reflect the maintainers of
the TXT-related code.

Signed-off-by Shane Wang <[email protected]>

Signed-off-by Joseph Cihula <[email protected]>

v2: Based on a complexity analysis and tradeoff, we moved all MAC'ing into tboot.

This patch adds support for S3 memory integrity protection within an Intel(R) TXT launched kernel, for all kernel and userspace memory. All RAM used by the kernel and userspace, as indicated by memory ranges of type E820_RAM and E820_RESERVED_KERN in the e820 table, will be integrity protected.

The MAINTAINERS file is also updated to reflect the maintainers of
the TXT-related code.

Signed-off-by Shane Wang <[email protected]>

Signed-off-by Joseph Cihula <[email protected]>

<compared with v2, this patch adds a check of array size in tboot.c, and a note to specify which c/s of tboot supports this kind of MACing in intel_txt.txt>

v3: Based on a complexity analysis and tradeoff, we moved all MAC'ing into tboot.

This patch adds support for S3 memory integrity protection within an Intel(R) TXT launched kernel, for all kernel and userspace memory. All RAM used by the kernel and userspace, as indicated by memory ranges of type E820_RAM and E820_RESERVED_KERN in the e820 table, will be integrity protected.

The MAINTAINERS file is also updated to reflect the maintainers of the TXT-related code.

Signed-off-by Shane Wang <[email protected]>

Signed-off-by Joseph Cihula <[email protected]>

  Documentation/intel_txt.txt |   16 +++++++++-------
  MAINTAINERS                 |   11 +++++++++++
  arch/x86/include/asm/e820.h |    7 ++++++-
  arch/x86/kernel/tboot.c     |   20 +++++++++++---------
  4 files changed, 37 insertions(+), 17 deletions(-)

diff -r d2911aa1461d Documentation/intel_txt.txt

Documentation/intel_txt.txt +9 -7

@@ -161,13 +161,15 @@ o  In order to put a system into any of
        has been restored, it will restore the TPM PCRs and then
        transfer control back to the kernel's S3 resume vector.
        In order to preserve system integrity across S3, the kernel
-      provides tboot with a set of memory ranges (kernel
-      code/data/bss, S3 resume code, and AP trampoline) that tboot
-      will calculate a MAC (message authentication code) over and then
-      seal with the TPM.  On resume and once the measured environment
-      has been re-established, tboot will re-calculate the MAC and
-      verify it against the sealed value.  Tboot's policy determines
-      what happens if the verification fails.
+      provides tboot with a set of memory ranges (RAM and RESERVED_KERN
+      in the e820 table, but not any memory that BIOS might alter over
+      the S3 transition) that tboot will calculate a MAC (message
+      authentication code) over and then seal with the TPM. On resume
+      and once the measured environment has been re-established, tboot
+      will re-calculate the MAC and verify it against the sealed value.
+      Tboot's policy determines what happens if the verification fails.
+      Note that the c/s 194 of tboot which has the new MAC code supports
+      this.
 
  That's pretty much it for TXT support.
 

diff -r d2911aa1461d MAINTAINERS

MAINTAINERS +11 -0

@@ -2891,6 +2891,17 @@ F:	Documentation/networking/README.ipw22
  F:	Documentation/networking/README.ipw2200
  F:	drivers/net/wireless/ipw2x00/ipw2200.*
 
+INTEL(R) TRUSTED EXECUTION TECHNOLOGY (TXT)
+M:	Joseph Cihula <[email protected]>
+M:	Shane Wang <[email protected]>
+L:	[email protected]
+W:	http://tboot.sourceforge.net
+T:	Mercurial http://www.bughost.org/repos.hg/tboot.hg
+S:	Supported
+F:	Documentation/intel_txt.txt
+F:	include/linux/tboot.h
+F:	arch/x86/kernel/tboot.c
+
  INTEL WIRELESS WIMAX CONNECTION 2400
  M:	Inaky Perez-Gonzalez <[email protected]>
  M:	[email protected]

diff -r d2911aa1461d arch/x86/include/asm/e820.h

arch/x86/include/asm/e820.h +6 -1

@@ -45,7 +45,12 @@
  #define E820_NVS	4
  #define E820_UNUSABLE	5
 
-/* reserved RAM used by kernel itself */
+/*
+ * reserved RAM used by kernel itself
+ * if CONFIG_INTEL_TXT is enabled, memory of this type will be
+ * included in the S3 integrity calculation and so should not include
+ * any memory that BIOS might alter over the S3 transition
+ */
  #define E820_RESERVED_KERN        128
 
  #ifndef __ASSEMBLY__

diff -r d2911aa1461d arch/x86/kernel/tboot.c

arch/x86/kernel/tboot.c +11 -9

@@ -130,6 +130,9 @@ static void add_mac_region(phys_addr_t s
  	struct tboot_mac_region *mr;
  	phys_addr_t end = start + size;
 
+	if (tboot->num_mac_regions >= MAX_TB_MAC_REGIONS)
+		panic("tboot: Too many MAC regions\n");
+
  	if (start && size) {
  		mr = &tboot->mac_regions[tboot->num_mac_regions++];
  		mr->start = round_down(start, PAGE_SIZE);

@@ -139,18 +142,17 @@ static void add_mac_region(phys_addr_t s
 
  static void __init tboot_setup_sleep(void)
  {
+	int i;
+
  	tboot->num_mac_regions = 0;
 
-	/* S3 resume code */
-	add_mac_region(acpi_wakeup_address, WAKEUP_SIZE);
+	for (i = 0; i < e820.nr_map; i++) {
+		if ((e820.map[i].type != E820_RAM)
+		 && (e820.map[i].type != E820_RESERVED_KERN))
+			continue;
 
-#ifdef CONFIG_X86_TRAMPOLINE
-	/* AP trampoline code */
-	add_mac_region(virt_to_phys(trampoline_base), TRAMPOLINE_SIZE);
-#endif
-
-	/* kernel code + data + bss */
-	add_mac_region(virt_to_phys(_text), _end - _text);
+		add_mac_region(e820.map[i].addr, e820.map[i].size);
+	}
 
  	tboot->acpi_sinfo.kernel_s3_resume_vector = acpi_wakeup_address;
  }

<compared with v2, this patch adds a check of array size in tboot.c, and a note to specify which c/s of tboot supports this kind of MACing in intel_txt.txt>

v3: Based on a complexity analysis and tradeoff, we moved all MAC'ing into tboot.

This patch adds support for S3 memory integrity protection within an Intel(R) TXT launched kernel, for all kernel and userspace memory. All RAM used by the kernel and userspace, as indicated by memory ranges of type E820_RAM and E820_RESERVED_KERN in the e820 table, will be integrity protected.

The MAINTAINERS file is also updated to reflect the maintainers of the TXT-related code.

Signed-off-by Shane Wang <[email protected]>

Signed-off-by Joseph Cihula <[email protected]>

  Documentation/intel_txt.txt |   16 +++++++++-------
  MAINTAINERS                 |   11 +++++++++++
  arch/x86/include/asm/e820.h |    7 ++++++-
  arch/x86/kernel/tboot.c     |   20 +++++++++++---------
  4 files changed, 37 insertions(+), 17 deletions(-)

diff -r d2911aa1461d Documentation/intel_txt.txt

Documentation/intel_txt.txt +9 -7

@@ -161,13 +161,15 @@ o  In order to put a system into any of
        has been restored, it will restore the TPM PCRs and then
        transfer control back to the kernel's S3 resume vector.
        In order to preserve system integrity across S3, the kernel
-      provides tboot with a set of memory ranges (kernel
-      code/data/bss, S3 resume code, and AP trampoline) that tboot
-      will calculate a MAC (message authentication code) over and then
-      seal with the TPM.  On resume and once the measured environment
-      has been re-established, tboot will re-calculate the MAC and
-      verify it against the sealed value.  Tboot's policy determines
-      what happens if the verification fails.
+      provides tboot with a set of memory ranges (RAM and RESERVED_KERN
+      in the e820 table, but not any memory that BIOS might alter over
+      the S3 transition) that tboot will calculate a MAC (message
+      authentication code) over and then seal with the TPM. On resume
+      and once the measured environment has been re-established, tboot
+      will re-calculate the MAC and verify it against the sealed value.
+      Tboot's policy determines what happens if the verification fails.
+      Note that the c/s 194 of tboot which has the new MAC code supports
+      this.
 
  That's pretty much it for TXT support.
 

diff -r d2911aa1461d MAINTAINERS

MAINTAINERS +11 -0

@@ -2891,6 +2891,17 @@ F:	Documentation/networking/README.ipw22
  F:	Documentation/networking/README.ipw2200
  F:	drivers/net/wireless/ipw2x00/ipw2200.*
 
+INTEL(R) TRUSTED EXECUTION TECHNOLOGY (TXT)
+M:	Joseph Cihula <[email protected]>
+M:	Shane Wang <[email protected]>
+L:	[email protected]
+W:	http://tboot.sourceforge.net
+T:	Mercurial http://www.bughost.org/repos.hg/tboot.hg
+S:	Supported
+F:	Documentation/intel_txt.txt
+F:	include/linux/tboot.h
+F:	arch/x86/kernel/tboot.c
+
  INTEL WIRELESS WIMAX CONNECTION 2400
  M:	Inaky Perez-Gonzalez <[email protected]>
  M:	[email protected]

diff -r d2911aa1461d arch/x86/include/asm/e820.h

arch/x86/include/asm/e820.h +6 -1

@@ -45,7 +45,12 @@
  #define E820_NVS	4
  #define E820_UNUSABLE	5
 
-/* reserved RAM used by kernel itself */
+/*
+ * reserved RAM used by kernel itself
+ * if CONFIG_INTEL_TXT is enabled, memory of this type will be
+ * included in the S3 integrity calculation and so should not include
+ * any memory that BIOS might alter over the S3 transition
+ */
  #define E820_RESERVED_KERN        128
 
  #ifndef __ASSEMBLY__

diff -r d2911aa1461d arch/x86/kernel/tboot.c

arch/x86/kernel/tboot.c +11 -9

@@ -130,6 +130,9 @@ static void add_mac_region(phys_addr_t s
  	struct tboot_mac_region *mr;
  	phys_addr_t end = start + size;
 
+	if (tboot->num_mac_regions >= MAX_TB_MAC_REGIONS)
+		panic("tboot: Too many MAC regions\n");
+
  	if (start && size) {
  		mr = &tboot->mac_regions[tboot->num_mac_regions++];
  		mr->start = round_down(start, PAGE_SIZE);

@@ -139,18 +142,17 @@ static void add_mac_region(phys_addr_t s
 
  static void __init tboot_setup_sleep(void)
  {
+	int i;
+
  	tboot->num_mac_regions = 0;
 
-	/* S3 resume code */
-	add_mac_region(acpi_wakeup_address, WAKEUP_SIZE);
+	for (i = 0; i < e820.nr_map; i++) {
+		if ((e820.map[i].type != E820_RAM)
+		 && (e820.map[i].type != E820_RESERVED_KERN))
+			continue;
 
-#ifdef CONFIG_X86_TRAMPOLINE
-	/* AP trampoline code */
-	add_mac_region(virt_to_phys(trampoline_base), TRAMPOLINE_SIZE);
-#endif
-
-	/* kernel code + data + bss */
-	add_mac_region(virt_to_phys(_text), _end - _text);
+		add_mac_region(e820.map[i].addr, e820.map[i].size);
+	}
 
  	tboot->acpi_sinfo.kernel_s3_resume_vector = acpi_wakeup_address;
  }