| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20091204110522.GR18989@one.firstfloor.org>
Date: Fri, 4 Dec 2009 12:05:22 +0100
From: Andi Kleen <andi@...stfloor.org>
To: Shane Wang <shane.wang@...el.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
"Cihula, Joseph" <joseph.cihula@...el.com>,
"arjan@...ux.intel.com" <arjan@...ux.intel.com>,
Andi Kleen <andi@...stfloor.org>,
"chrisw@...s-sol.org" <chrisw@...s-sol.org>,
"jmorris@...ei.org" <jmorris@...ei.org>,
"jbeulich@...ell.com" <jbeulich@...ell.com>,
"peterm@...hat.com" <peterm@...hat.com>, len.brown@...el.com,
Pavel Machek <pavel@....cz>, "Rafael J. Wysocki" <rjw@...k.pl>,
linux-pm@...ts.linux-foundation.org
Subject: Re: [PATCH] intel_txt: add s3 userspace memory integrity verification
On Fri, Dec 04, 2009 at 05:12:11PM +0800, Shane Wang wrote:
> diff -r c878d454dc8b arch/x86/kernel/entry_64.S
> --- a/arch/x86/kernel/entry_64.S Wed Dec 02 01:06:32 2009 -0800
> +++ b/arch/x86/kernel/entry_64.S Thu Dec 03 07:22:17 2009 -0800
> @@ -1275,6 +1275,26 @@ ENTRY(call_softirq)
> CFI_ENDPROC
> END(call_softirq)
>
> +#ifdef CONFIG_INTEL_TXT
> +/* void tboot_switch_stack_call(void (*target_func)(void), u64 new_rsp) */
> +ENTRY(tboot_switch_stack_call)
I would drop the tboot_ prefix, stack switching can be useful for other
things too.
> + CFI_STARTPROC
> + push %rbp
> + CFI_ADJUST_CFA_OFFSET 8
> + CFI_REL_OFFSET rbp,0
> + mov %rsp, %rbp
> + CFI_DEF_CFA_REGISTER rbp
Did you verify that the dwarf2 really works and gdb can backtrace through
it?
> +{
> + BUG_ON((new_stack != NULL) || (new_stack_ptr != NULL));
> +
> + /*
> + * as long as thread info is above 4G, then switch stack,
> + * since tboot can't access >4G stack for MACing
> + */
> + if (!((PFN_PHYS(PFN_DOWN(virt_to_phys(current_thread_info())))
> + + (PFN_UP(THREAD_SIZE) << PAGE_SHIFT))
> + & 0xffffffff00000000UL))
> + return -1;
All the PFN_*s seem somewhat redundant, it would be easier to keep
everything shifted up in the first place.
> +
> + new_stack = (char *)__get_free_pages(GFP_DMA32, IRQ_STACK_ORDER);
> +
> + BUG_ON(new_stack == NULL);
GFP_REPEAT at least? BUG_ON is a nasty way to handle out of memory
> + memset(new_stack, 0, IRQ_STACK_SIZE);
GFP_ZERO
> + new_stack_ptr = new_stack + IRQ_STACK_SIZE - 64;
Why - 64?
> +
> + return 0;
> +}
> +
> +static void tboot_post_stack_switch(void)
> +{
> + BUG_ON((new_stack == NULL) || (new_stack_ptr == NULL));
> +
> + free_pages((unsigned long)new_stack, IRQ_STACK_ORDER);
> + new_stack = NULL;
> + new_stack_ptr = NULL;
> +}
> +
> +extern void tboot_switch_stack_call(void (*target_func)(void), u64
> new_rsp);
Typically those should be in some header.
> + struct page *page;
> + uint64_t paddr, rstart, rend;
> + unsigned long pfn;
> + uint8_t zeroed_key[VMAC_KEY_LEN];
> +
> + if (!tfm)
> + tfm = crypto_alloc_hash("vmac(aes)", 0, CRYPTO_ALG_ASYNC);
> +
> + if (IS_ERR(tfm)) {
> + tfm = NULL;
> + return -ENOMEM;
> + }
> +
> + desc.tfm = tfm;
> + desc.flags = 0;
> +
> + sg_init_table(sg, 1);
> +
> + ret = crypto_hash_init(&desc);
> + if (ret)
> + return ret;
> + ret = crypto_hash_setkey(desc.tfm, key, VMAC_KEY_LEN);
> + if (ret)
> + return ret;
> +
> + for_each_online_pgdat(pgdat) {
No locking against memory hotplug? Even if user space is still down
doing that would be safer
> + unsigned long flags;
> +
> + pgdat_resize_lock(pgdat, &flags);
> + 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;
You probably should consider a faster way to skip holes, doing
them piece by piece might well be a performance problem on very
holey systems. Especially page_is_ram() is quite slow.
> + || (rend <= paddr))
> + continue;
> + break;
> + }
> +
> + if (j == tboot->num_mac_regions) {
> + sg_set_page(sg, page, PAGE_SIZE, 0);
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> + /*
> + * check if the page we are going to MAC is marked as
> + * present in the kernel page tables.
> + */
> + if (!kernel_page_present(page)) {
> + kernel_map_pages(page, 1, 1);
> + ret = crypto_hash_update(&desc, sg,
> PAGE_SIZE);
> + kernel_map_pages(page, 1, 0);
Nasty, might be racy -- better use a separate mapping in this case.
> +#ifdef CONFIG_X86_64
> + /*
> + * for stack > 4G, we should MAC the stack in the kernel after
> switch,
> + * for stack < 4G, the stack is MACed by tboot
> + */
This special case seems quite ugly, with all its ifdefs and lots
of special code.
Can't you just MAC the stack always? Shouldn't be that expensive.
> + else
> +#endif
> + add_mac_region(virt_to_phys(current_thread_info()),
> + THREAD_SIZE); /* < 4G */
> +
> + /* MAC userspace memory not handled by tboot */
> + get_random_bytes(tboot->s3_key, sizeof(tboot->s3_key));
That's early in boot isn't it? It's quite doubtful you'll get
anything even vaguely random out of get_random_bytes at this point, may be not
even the time.
> }
>
> tboot_shutdown(acpi_shutdown_map[sleep_state]);
> +}
> +
> +static void tboot_sx_resume(void)
> +{
> + vmac_t mac;
> +
> + if (tboot_gen_mem_integrity(tboot->s3_key, &mac))
> + panic("tboot: vmac generation failed\n");
> + else if (mac != mem_mac)
> +#ifdef CONFIG_DEBUG_KERNEL
> + pr_debug("tboot: memory integrity %llx -> %llx\n",
> + mem_mac, mac);
> +#else
> + panic("tboot: memory integrity was lost on resume\n");
> +#endif
I don't think DEBUG_KERNEL is supposed to be used like this, better
probably a separate option if it makes sense.
Typical problem with panics at this point: is anything even visible
on the screen?
>
> +#ifdef CONFIG_INTEL_TXT
> +/* void tboot_switch_stack_call(void (*target_func)(void), u64 new_rsp) */
> +ENTRY(tboot_switch_stack_call)
Hmm, I thought i had seen that earlier already? Is it a copy?
BTW there's no reason all of this has to be in entry*.S, that
file is already quite crowded.
The patch is duplicated here?
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists