Re: [PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
On Thu, Apr 18, 2019 at 8:32 PM Borislav Petkov wrote: > > On Thu, Apr 18, 2019 at 03:56:09PM +0800, Pingfan Liu wrote: > > Then in my case, either no @offset or invalid argument will keep > > "*crash_base = 0", and KASLR does not care about either of them. > > Ok. > > > It is not elegant. Will try a separate patch to fix it firstly. > > That's appreciated, thanks. It is about time that whole kexec/kaslr/... > code gets some much needed cleaning up and streamlining. > I had tried it v1 on https://patchwork.kernel.org/patch/10909627/ and v2 on https://patchwork.kernel.org/patch/10914169/. It seems no more feed back and hard to push forward. Since "x86/boot/KASLR: skip the specified crashkernel region" has no dependency on the above patch, I would like to send the next version for it. Regards, Pingfan
Re: [PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
On Thu, Apr 18, 2019 at 03:56:09PM +0800, Pingfan Liu wrote: > Then in my case, either no @offset or invalid argument will keep > "*crash_base = 0", and KASLR does not care about either of them. Ok. > It is not elegant. Will try a separate patch to fix it firstly. That's appreciated, thanks. It is about time that whole kexec/kaslr/... code gets some much needed cleaning up and streamlining. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
On Thu, Apr 18, 2019 at 12:06 AM Borislav Petkov wrote:
>
> On Wed, Apr 17, 2019 at 01:53:37PM +0800, Pingfan Liu wrote:
> > Take __parse_crashkernel()->parse_crashkernel_simple() for example. If
> > no offset given, then it still return 0, but crash_base is dangling.
Sorry for misleading, I made a mistake. In
parse_crashkernel()->__parse_crashkernel(), { *crash_size = 0;
*crash_base = 0;}. Hence no need to initialize crash_base in
handle_crashkernel_options().
>
> Well, that is bad design. parse_crashkernel_simple() should return a
> *separate* distinct value which denotes that @offset hasn't been passed.
Then in my case, either no @offset or invalid argument will keep
"*crash_base = 0", and KASLR does not care about either of them.
>
> Please fix that by having it return 1 or something else positive to
> denote that there wasn't an [@offset] given.
>
> And then correct that crap here:
>
> static void __init reserve_crashkernel(void)
> {
> ...
>
> ret = parse_crashkernel(boot_command_line, total_mem, _size,
> _base);
> if (ret != 0 || crash_size <= 0) {
It is not elegant. Will try a separate patch to fix it firstly.
Thanks,
Pingfan
>
> where *two*! variables are used as return values from a single function.
> That's just sloppy.
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
On Wed, Apr 17, 2019 at 01:53:37PM +0800, Pingfan Liu wrote:
> Take __parse_crashkernel()->parse_crashkernel_simple() for example. If
> no offset given, then it still return 0, but crash_base is dangling.
Well, that is bad design. parse_crashkernel_simple() should return a
*separate* distinct value which denotes that @offset hasn't been passed.
Please fix that by having it return 1 or something else positive to
denote that there wasn't an [@offset] given.
And then correct that crap here:
static void __init reserve_crashkernel(void)
{
...
ret = parse_crashkernel(boot_command_line, total_mem, _size,
_base);
if (ret != 0 || crash_size <= 0) {
where *two*! variables are used as return values from a single function.
That's just sloppy.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
On Wed, Apr 17, 2019 at 3:01 AM Borislav Petkov wrote:
>
> On Mon, Apr 08, 2019 at 01:58:35PM +0800, Pingfan Liu wrote:
> > crashkernel=x@y or or =range1:size1[,range2:size2,...]@offset option may
> > fail to reserve the required memory region if KASLR puts kernel into the
> > region. To avoid this uncertainty, asking KASLR to skip the required
> > region.
> >
> > Signed-off-by: Pingfan Liu
> > Cc: Thomas Gleixner
> > Cc: Ingo Molnar
> > Cc: Borislav Petkov
> > Cc: "H. Peter Anvin"
> > Cc: Baoquan He
> > Cc: Will Deacon
> > Cc: Nicolas Pitre
> > Cc: Vivek Goyal
> > Cc: Chao Fan
> > Cc: "Kirill A. Shutemov"
> > Cc: Ard Biesheuvel
> > CC: Hari Bathini
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > arch/x86/boot/compressed/kaslr.c | 40
> >
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c
> > b/arch/x86/boot/compressed/kaslr.c
> > index 2e53c05..765a593 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -107,6 +107,7 @@ enum mem_avoid_index {
> > MEM_AVOID_BOOTPARAMS,
> > MEM_AVOID_MEMMAP_BEGIN,
> > MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS -
> > 1,
> > + MEM_AVOID_CRASHKERNEL,
> > MEM_AVOID_MAX,
> > };
> >
> > @@ -131,6 +132,11 @@ char *skip_spaces(const char *str)
> > }
> > #include "../../../../lib/ctype.c"
> > #include "../../../../lib/cmdline.c"
> > +#ifdef CONFIG_CRASH_CORE
> > +#define printk
> > +#define _BOOT_KASLR
> > +#include "../../../../lib/parse_crashkernel.c"
> > +#endif
> >
> > static int
> > parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > @@ -292,6 +298,39 @@ static void handle_mem_options(void)
> > return;
> > }
> >
> > +static u64 mem_ram_size(void)
> > +{
> > + struct boot_e820_entry *entry;
> > + u64 total_sz = 0;
> > + int i;
> > +
> > + for (i = 0; i < boot_params->e820_entries; i++) {
> > + entry = _params->e820_table[i];
> > + /* Skip non-RAM entries. */
> > + if (entry->type != E820_TYPE_RAM)
> > + continue;
> > + total_sz += entry->size;
> > + }
> > + return total_sz;
> > +}
> > +
> > +/*
> > + * For crashkernel=size@offset or =range1:size1[,range2:size2,...]@offset
> > + * options, recording mem_avoid for them.
> > + */
> > +static void handle_crashkernel_options(void)
> > +{
> > + unsigned long long crash_size, crash_base = 0;
> > + char *cmdline = (char *)get_cmd_line_ptr();
> > + u64 total_sz = mem_ram_size();
> > +
> > + parse_crashkernel(cmdline, total_sz, _size, _base);
>
> That function has a return value which you could test. And then you
> don't need to set crash_base to 0 above.
>
Take __parse_crashkernel()->parse_crashkernel_simple() for example. If
no offset given, then it still return 0, but crash_base is dangling.
Regards,
Pingfan
Re: [PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
On Mon, Apr 08, 2019 at 01:58:35PM +0800, Pingfan Liu wrote:
> crashkernel=x@y or or =range1:size1[,range2:size2,...]@offset option may
> fail to reserve the required memory region if KASLR puts kernel into the
> region. To avoid this uncertainty, asking KASLR to skip the required
> region.
>
> Signed-off-by: Pingfan Liu
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Borislav Petkov
> Cc: "H. Peter Anvin"
> Cc: Baoquan He
> Cc: Will Deacon
> Cc: Nicolas Pitre
> Cc: Vivek Goyal
> Cc: Chao Fan
> Cc: "Kirill A. Shutemov"
> Cc: Ard Biesheuvel
> CC: Hari Bathini
> Cc: linux-kernel@vger.kernel.org
> ---
> arch/x86/boot/compressed/kaslr.c | 40
>
> 1 file changed, 40 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c
> b/arch/x86/boot/compressed/kaslr.c
> index 2e53c05..765a593 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -107,6 +107,7 @@ enum mem_avoid_index {
> MEM_AVOID_BOOTPARAMS,
> MEM_AVOID_MEMMAP_BEGIN,
> MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
> + MEM_AVOID_CRASHKERNEL,
> MEM_AVOID_MAX,
> };
>
> @@ -131,6 +132,11 @@ char *skip_spaces(const char *str)
> }
> #include "../../../../lib/ctype.c"
> #include "../../../../lib/cmdline.c"
> +#ifdef CONFIG_CRASH_CORE
> +#define printk
> +#define _BOOT_KASLR
> +#include "../../../../lib/parse_crashkernel.c"
> +#endif
>
> static int
> parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> @@ -292,6 +298,39 @@ static void handle_mem_options(void)
> return;
> }
>
> +static u64 mem_ram_size(void)
> +{
> + struct boot_e820_entry *entry;
> + u64 total_sz = 0;
> + int i;
> +
> + for (i = 0; i < boot_params->e820_entries; i++) {
> + entry = _params->e820_table[i];
> + /* Skip non-RAM entries. */
> + if (entry->type != E820_TYPE_RAM)
> + continue;
> + total_sz += entry->size;
> + }
> + return total_sz;
> +}
> +
> +/*
> + * For crashkernel=size@offset or =range1:size1[,range2:size2,...]@offset
> + * options, recording mem_avoid for them.
> + */
> +static void handle_crashkernel_options(void)
> +{
> + unsigned long long crash_size, crash_base = 0;
> + char *cmdline = (char *)get_cmd_line_ptr();
> + u64 total_sz = mem_ram_size();
> +
> + parse_crashkernel(cmdline, total_sz, _size, _base);
That function has a return value which you could test. And then you
don't need to set crash_base to 0 above.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[PATCH v4 2/2] x86/boot/KASLR: skip the specified crashkernel region
crashkernel=x@y or or =range1:size1[,range2:size2,...]@offset option may
fail to reserve the required memory region if KASLR puts kernel into the
region. To avoid this uncertainty, asking KASLR to skip the required
region.
Signed-off-by: Pingfan Liu
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: "H. Peter Anvin"
Cc: Baoquan He
Cc: Will Deacon
Cc: Nicolas Pitre
Cc: Vivek Goyal
Cc: Chao Fan
Cc: "Kirill A. Shutemov"
Cc: Ard Biesheuvel
CC: Hari Bathini
Cc: linux-kernel@vger.kernel.org
---
arch/x86/boot/compressed/kaslr.c | 40
1 file changed, 40 insertions(+)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 2e53c05..765a593 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -107,6 +107,7 @@ enum mem_avoid_index {
MEM_AVOID_BOOTPARAMS,
MEM_AVOID_MEMMAP_BEGIN,
MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
+ MEM_AVOID_CRASHKERNEL,
MEM_AVOID_MAX,
};
@@ -131,6 +132,11 @@ char *skip_spaces(const char *str)
}
#include "../../../../lib/ctype.c"
#include "../../../../lib/cmdline.c"
+#ifdef CONFIG_CRASH_CORE
+#define printk
+#define _BOOT_KASLR
+#include "../../../../lib/parse_crashkernel.c"
+#endif
static int
parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
@@ -292,6 +298,39 @@ static void handle_mem_options(void)
return;
}
+static u64 mem_ram_size(void)
+{
+ struct boot_e820_entry *entry;
+ u64 total_sz = 0;
+ int i;
+
+ for (i = 0; i < boot_params->e820_entries; i++) {
+ entry = _params->e820_table[i];
+ /* Skip non-RAM entries. */
+ if (entry->type != E820_TYPE_RAM)
+ continue;
+ total_sz += entry->size;
+ }
+ return total_sz;
+}
+
+/*
+ * For crashkernel=size@offset or =range1:size1[,range2:size2,...]@offset
+ * options, recording mem_avoid for them.
+ */
+static void handle_crashkernel_options(void)
+{
+ unsigned long long crash_size, crash_base = 0;
+ char *cmdline = (char *)get_cmd_line_ptr();
+ u64 total_sz = mem_ram_size();
+
+ parse_crashkernel(cmdline, total_sz, _size, _base);
+ if (crash_base) {
+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
+ }
+}
+
/*
* In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
* The mem_avoid array is used to store the ranges that need to be avoided
@@ -414,6 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned
long input_size,
/* Mark the memmap regions we need to avoid */
handle_mem_options();
+ handle_crashkernel_options();
/* Enumerate the immovable memory regions */
num_immovable_mem = count_immovable_mem_regions();
--
2.7.4
