Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

2018-09-29 Thread lijiang 
在 2018年09月29日 16:30, Borislav Petkov 写道:
> On Sat, Sep 29, 2018 at 02:24:52PM +0800, lijiang wrote:
>> At first, i added an input parameter for read_from_oldmem() because of 
>> encryption(SME). But
>> for avoiding to also add the same parameter for copy_oldmem_page(), so i 
>> added a new function
>> copy_oldmem_page_encrypted(). Maybe you had noticed that these functions 
>> were very similar.
> 
> If you have two very similar functions, you add a *static* workhorse function:
> 
> static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize, 
> unsigned long offset,
> int userbuf, bool encrypted)
> 
> and you define two wrappers:
> 
> copy_oldmem_page()
> copy_oldmem_page_encrypted()
> 
> which both call __copy_oldmem_page() with the appropriate parameters.
> 

Great! Previously i was afraid that the maintainer might disagree with
rewriting the function copy_oldmem_page().

That's really great. I will modify this patch and post the series again.

Thanks.
Lianbo
> But you do *not* do a separate compilation unit just because. None of
> the reasons you've mentioned warrant having a separate compilation
> unit while you already have *the* perfect place to put everything -
> arch/x86/kernel/crash_dump_64.c
> 
>> So sorry, here "oldmem encrypted" means the "old memory is encrypted".
> 
> I know what it means - I'm trying to explain to you to write it out
> in plain english and not use some strange constructs like "oldmem
> encrypted".
> 
> A reader would wonder: why is this thing semi-abbreviated and in
> quotation marks? Does that mean anything special?
> 
> Our comments should not be write-only. So after you've written it, try
> to read it as someone who sees the code for the first time and think
> hard whether she/he will understand it.
> 
> Do you catch my drift now?
> 
Yes, got it. Thanks for your valuable time and patience.

Regards,
Lianbo

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

2018-09-29 Thread Borislav Petkov 
On Sat, Sep 29, 2018 at 02:24:52PM +0800, lijiang wrote:
> At first, i added an input parameter for read_from_oldmem() because of 
> encryption(SME). But
> for avoiding to also add the same parameter for copy_oldmem_page(), so i 
> added a new function
> copy_oldmem_page_encrypted(). Maybe you had noticed that these functions were 
> very similar.

If you have two very similar functions, you add a *static* workhorse function:

static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize, 
unsigned long offset,
  int userbuf, bool encrypted)

and you define two wrappers:

copy_oldmem_page()
copy_oldmem_page_encrypted()

which both call __copy_oldmem_page() with the appropriate parameters.

But you do *not* do a separate compilation unit just because. None of
the reasons you've mentioned warrant having a separate compilation
unit while you already have *the* perfect place to put everything -
arch/x86/kernel/crash_dump_64.c

> So sorry, here "oldmem encrypted" means the "old memory is encrypted".

I know what it means - I'm trying to explain to you to write it out
in plain english and not use some strange constructs like "oldmem
encrypted".

A reader would wonder: why is this thing semi-abbreviated and in
quotation marks? Does that mean anything special?

Our comments should not be write-only. So after you've written it, try
to read it as someone who sees the code for the first time and think
hard whether she/he will understand it.

Do you catch my drift now?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

2018-09-28 Thread lijiang 
在 2018年09月28日 16:38, Borislav Petkov 写道:
> 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.
> 

But the root cause is that the encryption key is not visible to any software
running on the CPU cores(AMD cpu with SME), and is randomly generated on each
system reset. That is to say, kdump kernel won't have a chance to get the
encryption key. So the encrypted memory can not be decrypted unless SME is
active. 

Thanks.
>>
>>  --
>> | 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.
> 

Thanks for your valuable advice to improve these patches.
The patch log and all code style issue will be improved.

>> 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 
>> Reviewed-by: Tom Lendacky 
>> ---
>>  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...
> 
Good question. It's my pleasure.

At first, i added an input parameter for read_from_oldmem() because of 
encryption(SME). But
for avoiding to also add the same parameter for copy_oldmem_page(), so i added 
a new function
copy_oldmem_page_encrypted(). Maybe you had noticed that these functions were 
very similar. 

Sometimes, it is hard to make a choice about this. The reasons are as follows:

A. If i add the same input parameter for copy_oldmem_page(), which will cause 
more code changes,
   because the interface changes will affect other architectures, such as 
ARM/MIPS/POWERPC/S390.

B. If not A, i have to add a new function copy_oldmem_page_encrypted(), and 
also put it into a
   same file(arch/x86/kernel/crash_dump_64.c).
 
   Should i merge the similar functions into one function?
   ( copy_oldmem_page() and copy_oldmem_page_encrypted() )

   a. If the answer is no, i will have two choices:
  1). put these functions into a same 
file(arch/x86/kernel/crash_dump_64.c). But someone
  thought that these functions were too similar, these functions should 
be merged or
  rebuilt. This is contradictory to the case a.
  2). add a new file for copy_oldmem_page_encrypted(), then which will 
avoid the case '1)'.
  That is just current solution.

   b. If the answer is yes, i have to add an input parameter for 
copy_oldmem_page(), this is back
  to the case A.

Could you give me any advice about this? Any advice will be appreciated.

Thanks.
Lianbo

>>  obj-y   += kprobes/
>>  obj-$(CONFIG_MODULES)   += module.o
>>  obj-$(CONFIG_D

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

2018-09-28 Thread Borislav Petkov 
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 
> Reviewed-by: Tom Lendacky 
> ---
>  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 ..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 (liji...@redhat.com)
> + *   Copyright (C) RedHat Corporation, 2018. All rights reserved
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * 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;
>

[PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

2018-09-27 Thread Lianbo Jiang 
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:

 --
| 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
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.

Signed-off-by: Lianbo Jiang 
Reviewed-by: Tom Lendacky 
---
 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
 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 ..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 (liji...@redhat.com)
+ * Copyright (C) RedHat Corporation, 2018. All rights reserved
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+/**
+ * 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
+ * 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)
+{
+   void  *vaddr;
+
+   if (!csize)
+   return 0;
+
+   vaddr = (__force void *)ioremap_encrypted(pfn << PAGE_SHIFT,
+ PAGE_SIZE);
+   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 
 #include 
 #include 
+#include 
+#include 
+#include 
 #include "internal.h"
 
 /* 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;