| 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
| ||
|
Date: Fri, 28 Sep 2018 10:38:52 +0200
From: Borislav Petkov <bp@...e.de>
To: Lianbo Jiang <lijiang@...hat.com>
Cc: linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
x86@...nel.org, akpm@...ux-foundation.org,
dan.j.williams@...el.com, thomas.lendacky@....com,
bhelgaas@...gle.com, baiyaowei@...s.chinamobile.com, tiwai@...e.de,
brijesh.singh@....com, dyoung@...hat.com, bhe@...hat.com,
jroedel@...e.de
Subject: Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory
with SME enabled
On Thu, Sep 27, 2018 at 03:19:54PM +0800, Lianbo Jiang wrote:
> In kdump kernel, we need to dump the old memory into vmcore file,if SME
> is enabled in the first kernel, we have to remap the old memory with the
> memory encryption mask, which will be automatically decrypted when we
> read from DRAM.
>
> For SME kdump, there are two cases that doesn't support:
... and which are simply silly to support.
>
> ----------------------------------------------
> | first-kernel | second-kernel | kdump support |
> | (mem_encrypt=on|off) | (yes|no) |
> |--------------+---------------+---------------|
> | on | on | yes |
> | off | off | yes |
> | on | off | no |
> | off | on | no |
> |______________|_______________|_______________|
>
> 1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
> In this case, because the old memory is encrypted, we can't decrypt the
> old memory.
>
> 2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
> On the one hand, the old memory is unencrypted, the old memory can be dumped
s/unencrypted/decrypted/g
But I mentioned that already.
> as usual, we don't need to enable SME in kdump kernel; On the other hand, it
> will increase the complexity of the code, we will have to consider how to
> pass the SME flag from the first kernel to the kdump kernel, it is really
> too expensive to do this.
>
> This patches are only for SME kdump, the patches don't support SEV kdump.
Please rewrite that commit message in passive voice. I.e., get rid of
that "we".
>
> Signed-off-by: Lianbo Jiang <lijiang@...hat.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
> ---
> arch/x86/kernel/Makefile | 1 +
> arch/x86/kernel/crash_dump_encrypt.c | 53 ++++++++++++++++++++++++++++
> fs/proc/vmcore.c | 21 +++++++----
> include/linux/crash_dump.h | 12 +++++++
> 4 files changed, 81 insertions(+), 6 deletions(-)
> create mode 100644 arch/x86/kernel/crash_dump_encrypt.c
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 8824d01c0c35..dfbeae0e35ce 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -97,6 +97,7 @@ obj-$(CONFIG_KEXEC_CORE) += machine_kexec_$(BITS).o
> obj-$(CONFIG_KEXEC_CORE) += relocate_kernel_$(BITS).o crash.o
> obj-$(CONFIG_KEXEC_FILE) += kexec-bzimage64.o
> obj-$(CONFIG_CRASH_DUMP) += crash_dump_$(BITS).o
> +obj-$(CONFIG_AMD_MEM_ENCRYPT) += crash_dump_encrypt.o
No no.
This will build even in the CONFIG_CRASH_DUMP=n case.
Why does this need to be even a separate compilation unit? It is a file
containing a single function?!?!
I would love to know what the logic behind this was...
> obj-y += kprobes/
> obj-$(CONFIG_MODULES) += module.o
> obj-$(CONFIG_DOUBLEFAULT) += doublefault.o
> diff --git a/arch/x86/kernel/crash_dump_encrypt.c b/arch/x86/kernel/crash_dump_encrypt.c
> new file mode 100644
> index 000000000000..e1b1a577f197
> --- /dev/null
> +++ b/arch/x86/kernel/crash_dump_encrypt.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Memory preserving reboot related code.
> + *
> + * Created by: Lianbo Jiang (lijiang@...hat.com)
> + * Copyright (C) RedHat Corporation, 2018. All rights reserved
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/crash_dump.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +
> +/**
> + * copy_oldmem_page_encrypted - copy one page from "oldmem encrypted"
> + * @pfn: page frame number to be copied
> + * @buf: target memory address for the copy; this can be in kernel address
> + * space or user address space (see @userbuf)
> + * @csize: number of bytes to copy
> + * @offset: offset in bytes into the page (based on pfn) to begin the copy
> + * @userbuf: if set, @buf is in user address space, use copy_to_user(),
> + * otherwise @buf is in kernel address space, use memcpy().
> + *
> + * Copy a page from "oldmem encrypted". For this page, there is no pte
What is "oldmem encrypted"? Why can't you explain that in plain english?
Note that those comments are not write-only but are meant for other
people to read in the future.
> + * mapped in the current kernel. We stitch up a pte, similar to
> + * kmap_atomic.
> + */
> +
> +ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
> + size_t csize, unsigned long offset, int userbuf)
Align arguments on the opening brace.
> +{
> + void *vaddr;
> +
> + if (!csize)
> + return 0;
> +
> + vaddr = (__force void *)ioremap_encrypted(pfn << PAGE_SHIFT,
> + PAGE_SIZE);
Let it stick out.
> + if (!vaddr)
> + return -ENOMEM;
> +
> + if (userbuf) {
> + if (copy_to_user((void __user *)buf, vaddr + offset, csize)) {
> + iounmap((void __iomem *)vaddr);
> + return -EFAULT;
> + }
> + } else
> + memcpy(buf, vaddr + offset, csize);
> +
> + set_iounmap_nonlazy();
> + iounmap((void __iomem *)vaddr);
> + return csize;
> +}
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index cbde728f8ac6..3065c8bada6a 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -25,6 +25,9 @@
> #include <linux/pagemap.h>
> #include <linux/uaccess.h>
> #include <asm/io.h>
> +#include <linux/io.h>
> +#include <linux/mem_encrypt.h>
> +#include <asm/pgtable.h>
Do you not see how the order of the include files is? First linux/ then
asm/, then local headers.
> #include "internal.h"
And you don't need that if you drop that silly crash_dump_encrypt.c thing.
>
> /* List representing chunks of contiguous memory areas and their offsets in
> @@ -98,7 +101,8 @@ static int pfn_is_ram(unsigned long pfn)
>
> /* Reads a page from the oldmem device from given offset. */
> static ssize_t read_from_oldmem(char *buf, size_t count,
> - u64 *ppos, int userbuf)
> + u64 *ppos, int userbuf,
> + bool encrypted)
> {
> unsigned long pfn, offset;
> size_t nr_bytes;
> @@ -120,8 +124,11 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
> if (pfn_is_ram(pfn) == 0)
> memset(buf, 0, nr_bytes);
> else {
> - tmp = copy_oldmem_page(pfn, buf, nr_bytes,
> - offset, userbuf);
> + tmp = encrypted ? copy_oldmem_page_encrypted(pfn,
> + buf, nr_bytes, offset, userbuf)
> + : copy_oldmem_page(pfn, buf, nr_bytes,
> + offset, userbuf);
Make that a simple if-else so that it can actually be readable.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
Powered by blists - more mailing lists