crashkernel=x@y option may fail to reserve the required memory region if
KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
skip the required region.
Signed-off-by: Pingfan Liu <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Nicolas Pitre <[email protected]>
Cc: Pingfan Liu <[email protected]>
Cc: Chao Fan <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: [email protected]
---
arch/x86/boot/compressed/kaslr.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709..728bc4b 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -109,6 +109,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,
};
@@ -240,6 +241,27 @@ static void parse_gb_huge_pages(char *param, char *val)
}
}
+/* parse crashkernel=x@y option */
+static int mem_avoid_crashkernel_simple(char *option)
+{
+ char *cur = option;
+ unsigned long long crash_size, crash_base;
+
+ crash_size = memparse(option, &cur);
+ if (option == cur)
+ return -EINVAL;
+
+ if (*cur == '@') {
+ option = cur + 1;
+ crash_base = memparse(option, &cur);
+ if (option == cur)
+ return -EINVAL;
+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
+ }
+
+ return 0;
+}
static void handle_mem_options(void)
{
@@ -250,7 +272,7 @@ static void handle_mem_options(void)
u64 mem_size;
if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
- !strstr(args, "hugepages"))
+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
return;
tmp_cmdline = malloc(len + 1);
@@ -286,6 +308,8 @@ static void handle_mem_options(void)
goto out;
mem_limit = mem_size;
+ } else if (strstr(param, "crashkernel")) {
+ mem_avoid_crashkernel_simple(val);
}
}
--
2.7.4
On Mon, Feb 25, 2019 at 03:59:56PM +0800, Pingfan Liu wrote:
>crashkernel=x@y option may fail to reserve the required memory region if
>KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
>skip the required region.
>
>Signed-off-by: Pingfan Liu <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>Cc: Ingo Molnar <[email protected]>
>Cc: Borislav Petkov <[email protected]>
>Cc: "H. Peter Anvin" <[email protected]>
>Cc: Baoquan He <[email protected]>
>Cc: Will Deacon <[email protected]>
>Cc: Nicolas Pitre <[email protected]>
>Cc: Pingfan Liu <[email protected]>
>Cc: Chao Fan <[email protected]>
>Cc: "Kirill A. Shutemov" <[email protected]>
>Cc: Ard Biesheuvel <[email protected]>
>Cc: [email protected]
>---
> arch/x86/boot/compressed/kaslr.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
Hi Pingfan,
Some not important comments:
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 9ed9709..728bc4b 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -109,6 +109,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,
> };
>
>@@ -240,6 +241,27 @@ static void parse_gb_huge_pages(char *param, char *val)
> }
> }
>
>+/* parse crashkernel=x@y option */
>+static int mem_avoid_crashkernel_simple(char *option)
>+{
>+ char *cur = option;
>+ unsigned long long crash_size, crash_base;
Change the position of two lines above.
>+
>+ crash_size = memparse(option, &cur);
>+ if (option == cur)
>+ return -EINVAL;
>+
>+ if (*cur == '@') {
>+ option = cur + 1;
>+ crash_base = memparse(option, &cur);
>+ if (option == cur)
>+ return -EINVAL;
>+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
>+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
>+ }
>+
>+ return 0;
You just call this function and don't use its return value.
So why not change it as void type.
>+}
>
> static void handle_mem_options(void)
If you want to change this function, I think you could change the
function name and the comment:
/* Mark the memmap regions we need to avoid */
handle_mem_options();
> {
>@@ -250,7 +272,7 @@ static void handle_mem_options(void)
> u64 mem_size;
>
> if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>- !strstr(args, "hugepages"))
>+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> return;
>
> tmp_cmdline = malloc(len + 1);
>@@ -286,6 +308,8 @@ static void handle_mem_options(void)
> goto out;
>
> mem_limit = mem_size;
>+ } else if (strstr(param, "crashkernel")) {
>+ mem_avoid_crashkernel_simple(val);
I am wondering why you call this function mem_avoid_crashkernel_*simple*().
Thanks,
Chao Fan
> }
> }
>
>--
>2.7.4
>
>
>
On Mon, Feb 25, 2019 at 03:59:56PM +0800, Pingfan Liu wrote:
> crashkernel=x@y option may fail to reserve the required memory region if
> KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> skip the required region.
Lemme see if I understand this correctly: supplying crashkernel=X@Y
influences where KASLR would put the randomized kernel. And it should be
the other way around, IMHO. crashkernel= will have to "work" with KASLR
to find a suitable range and if the reservation at Y fails, then we tell
the user to try the more relaxed variant crashkernel=M.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 02/25/19 at 10:45am, Borislav Petkov wrote:
> On Mon, Feb 25, 2019 at 03:59:56PM +0800, Pingfan Liu wrote:
> > crashkernel=x@y option may fail to reserve the required memory region if
> > KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> > skip the required region.
>
> Lemme see if I understand this correctly: supplying crashkernel=X@Y
> influences where KASLR would put the randomized kernel. And it should be
> the other way around, IMHO. crashkernel= will have to "work" with KASLR
> to find a suitable range and if the reservation at Y fails, then we tell
> the user to try the more relaxed variant crashkernel=M.
Hmm, asking user to try crashkernel=M is an option. Users may want to
specify a region for crashkernel reservation, Just I forget in what case
they want crashkernel=x@y set. In crashkernel=x@y specified case, we may
truly need to avoid the already specified region.
Not sure if Dave still remember it. If no need, removing it is also
good.
Thanks
Baoquan
On Mon, Feb 25, 2019 at 5:45 PM Borislav Petkov <[email protected]> wrote:
>
> On Mon, Feb 25, 2019 at 03:59:56PM +0800, Pingfan Liu wrote:
> > crashkernel=x@y option may fail to reserve the required memory region if
> > KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> > skip the required region.
>
> Lemme see if I understand this correctly: supplying crashkernel=X@Y
> influences where KASLR would put the randomized kernel. And it should be
Yes, you get it.
> the other way around, IMHO. crashkernel= will have to "work" with KASLR
> to find a suitable range and if the reservation at Y fails, then we tell
> the user to try the more relaxed variant crashkernel=M.
>
I follow Baoquan's opinion. Due to the randomness caused by KASLR, a
user may be surprised to find crashkernel=x@y not working sometime. If
kernel can help them out of this corner automatically, then no need to
bother them with the message to use alternative method crashkernel=M.
Anyway it is a cheap method already used by other options like
hugepages and memmap in handle_mem_options().
If commitment, then do it without failure. Or just removing
crashkernel=x@y option on x86.
Thanks and regards,
Pingfan
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
On Mon, Feb 25, 2019 at 4:23 PM Chao Fan <[email protected]> wrote:
>
> On Mon, Feb 25, 2019 at 03:59:56PM +0800, Pingfan Liu wrote:
> >crashkernel=x@y option may fail to reserve the required memory region if
> >KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> >skip the required region.
> >
> >Signed-off-by: Pingfan Liu <[email protected]>
> >Cc: Thomas Gleixner <[email protected]>
> >Cc: Ingo Molnar <[email protected]>
> >Cc: Borislav Petkov <[email protected]>
> >Cc: "H. Peter Anvin" <[email protected]>
> >Cc: Baoquan He <[email protected]>
> >Cc: Will Deacon <[email protected]>
> >Cc: Nicolas Pitre <[email protected]>
> >Cc: Pingfan Liu <[email protected]>
> >Cc: Chao Fan <[email protected]>
> >Cc: "Kirill A. Shutemov" <[email protected]>
> >Cc: Ard Biesheuvel <[email protected]>
> >Cc: [email protected]
> >---
> > arch/x86/boot/compressed/kaslr.c | 26 +++++++++++++++++++++++++-
> > 1 file changed, 25 insertions(+), 1 deletion(-)
> >
>
> Hi Pingfan,
>
> Some not important comments:
>
> >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> >index 9ed9709..728bc4b 100644
> >--- a/arch/x86/boot/compressed/kaslr.c
> >+++ b/arch/x86/boot/compressed/kaslr.c
> >@@ -109,6 +109,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,
> > };
> >
> >@@ -240,6 +241,27 @@ static void parse_gb_huge_pages(char *param, char *val)
> > }
> > }
> >
> >+/* parse crashkernel=x@y option */
> >+static int mem_avoid_crashkernel_simple(char *option)
> >+{
> >+ char *cur = option;
> >+ unsigned long long crash_size, crash_base;
>
> Change the position of two lines above.
>
Yes, it is better.
> >+
> >+ crash_size = memparse(option, &cur);
> >+ if (option == cur)
> >+ return -EINVAL;
> >+
> >+ if (*cur == '@') {
> >+ option = cur + 1;
> >+ crash_base = memparse(option, &cur);
> >+ if (option == cur)
> >+ return -EINVAL;
> >+ mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
> >+ mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
> >+ }
> >+
> >+ return 0;
>
> You just call this function and don't use its return value.
> So why not change it as void type.
>
OK.
> >+}
> >
> > static void handle_mem_options(void)
>
> If you want to change this function, I think you could change the
> function name and the comment:
>
> /* Mark the memmap regions we need to avoid */
> handle_mem_options();
>
Yes, it is outdated, should fix the comment.
> > {
> >@@ -250,7 +272,7 @@ static void handle_mem_options(void)
> > u64 mem_size;
> >
> > if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> >- !strstr(args, "hugepages"))
> >+ !strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> > return;
> >
> > tmp_cmdline = malloc(len + 1);
> >@@ -286,6 +308,8 @@ static void handle_mem_options(void)
> > goto out;
> >
> > mem_limit = mem_size;
> >+ } else if (strstr(param, "crashkernel")) {
> >+ mem_avoid_crashkernel_simple(val);
>
> I am wondering why you call this function mem_avoid_crashkernel_*simple*().
>
It follows the name of parse_crashkernel_simple()
Thanks,
Pingfan
On 02/25/19 at 09:05pm, Baoquan He wrote:
> On 02/25/19 at 10:45am, Borislav Petkov wrote:
> > On Mon, Feb 25, 2019 at 03:59:56PM +0800, Pingfan Liu wrote:
> > > crashkernel=x@y option may fail to reserve the required memory region if
> > > KASLR puts kernel into the region. To avoid this uncertainty, making KASLR
> > > skip the required region.
> >
> > Lemme see if I understand this correctly: supplying crashkernel=X@Y
> > influences where KASLR would put the randomized kernel. And it should be
> > the other way around, IMHO. crashkernel= will have to "work" with KASLR
> > to find a suitable range and if the reservation at Y fails, then we tell
> > the user to try the more relaxed variant crashkernel=M.
>
> Hmm, asking user to try crashkernel=M is an option. Users may want to
> specify a region for crashkernel reservation, Just I forget in what case
> they want crashkernel=x@y set. In crashkernel=x@y specified case, we may
> truly need to avoid the already specified region.
>
> Not sure if Dave still remember it. If no need, removing it is also
> good.
I do not know the exact use cases, but long time ago the kernel is not
relocatable this might be a reason. Even now, there could be some non
linux use cases, if the loaded binary is not relocatable then the param
is still useful.
Also this is a general param instead of x86 only, some other arches
still use it, and no crashkernel=X implemented.
Thanks
Dave
On Tue, Feb 26, 2019 at 11:08:42AM +0800, Pingfan Liu wrote:
> I follow Baoquan's opinion. Due to the randomness caused by KASLR, a
> user may be surprised to find crashkernel=x@y not working sometime.
And she/he will get told in dmesg that the allocation failed.
> If kernel can help them out of this corner automatically, then no
> need to bother them with the message to use alternative method
> crashkernel=M. Anyway it is a cheap method already used by other
> options like hugepages and memmap in handle_mem_options().
> If commitment, then do it without failure. Or just removing
> crashkernel=x@y option on x86.
I can't parse what you're trying to say here but let me repeat myself:
specifying a crashkernel region should not have an influence on KASLR
because this way you limit the kernel where it selects the offset. It
should be other other way around: KASLR offset should be selected and
*then* crashkernel region.
IOW, KASLR offset selection needs to have higher priority than
crashkernel region selection.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 02/26/19 at 11:46am, Borislav Petkov wrote:
> On Tue, Feb 26, 2019 at 11:08:42AM +0800, Pingfan Liu wrote:
> > I follow Baoquan's opinion. Due to the randomness caused by KASLR, a
> > user may be surprised to find crashkernel=x@y not working sometime.
>
> And she/he will get told in dmesg that the allocation failed.
>
> > If kernel can help them out of this corner automatically, then no
> > need to bother them with the message to use alternative method
> > crashkernel=M. Anyway it is a cheap method already used by other
> > options like hugepages and memmap in handle_mem_options().
> > If commitment, then do it without failure. Or just removing
> > crashkernel=x@y option on x86.
>
> I can't parse what you're trying to say here but let me repeat myself:
> specifying a crashkernel region should not have an influence on KASLR
> because this way you limit the kernel where it selects the offset. It
> should be other other way around: KASLR offset should be selected and
> *then* crashkernel region.
Agree that 'crashkernel=x' should be encouraged to use as the first
choice when reserve crashkernel. If we decide to not obsolete
'crashkernel=x@y', it will leave a unstable kernel parameter. Another
worry is that KASLR won't always fail 'crashkernel=x@y', customer may
set and check in testing stage, then later in production environment one
time of neglect to not check may cause carashed kernel uncaptured.
IMHO, 'crashkernel=x@y' is similar to those specified memmap=ss[#$!]nn
which have been avoided in boot stage KASLR.
And in kernel life cycle, we specify kernel parameter before kernel boot,
later KASLR lives to choose position. So KASLR can avoid firstly
specified regions which are being reserved for usage, 'crashkernel=x@y'
have no way to compromise with KASLR if it's still allowed to exist in
kernel.
Personal thought.
Thanks
Baoquan
+ Kees.
@Kees, you might want to go upthread a bit for context.
On Wed, Feb 27, 2019 at 09:30:34AM +0800, Baoquan He wrote:
> Agree that 'crashkernel=x' should be encouraged to use as the first
> choice when reserve crashkernel. If we decide to not obsolete
> 'crashkernel=x@y', it will leave a unstable kernel parameter.
Is anyone even talking about obsoleting this?
And if anyone is, anyone can think a bit why we can't do this.
> Another worry is that KASLR won't always fail 'crashkernel=x@y',
> customer may set and check in testing stage, then later in production
> environment one time of neglect to not check may cause carashed kernel
> uncaptured.
>
> IMHO, 'crashkernel=x@y' is similar to those specified memmap=ss[#$!]nn
> which have been avoided in boot stage KASLR.
So my worry is that by specifying too many exclusion ranges, we might
limit the kaslr space too much and make it too predictable. Especially
since distros slap those things automatically and most users take them
for granted.
But I might be way off here because of something else I'm missing ...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Wed, Feb 27, 2019 at 3:40 PM Borislav Petkov <[email protected]> wrote:
>
> + Kees.
>
> @Kees, you might want to go upthread a bit for context.
>
Seems not reply from Kees.
> On Wed, Feb 27, 2019 at 09:30:34AM +0800, Baoquan He wrote:
> > Agree that 'crashkernel=x' should be encouraged to use as the first
> > choice when reserve crashkernel. If we decide to not obsolete
> > 'crashkernel=x@y', it will leave a unstable kernel parameter.
>
> Is anyone even talking about obsoleting this?
>
> And if anyone is, anyone can think a bit why we can't do this.
>
As Dave said, some un-relocatable code should be loaded to a specified
space. Also the param is used by archs beside x86
> > Another worry is that KASLR won't always fail 'crashkernel=x@y',
> > customer may set and check in testing stage, then later in production
> > environment one time of neglect to not check may cause carashed kernel
> > uncaptured.
> >
> > IMHO, 'crashkernel=x@y' is similar to those specified memmap=ss[#$!]nn
> > which have been avoided in boot stage KASLR.
>
> So my worry is that by specifying too many exclusion ranges, we might
> limit the kaslr space too much and make it too predictable. Especially
> since distros slap those things automatically and most users take them
> for granted.
>
Kernel has already done this excluding 1gb pages. Do we need to worry
about 200-400 MB for crashkernel? And I think if a user specify the
region, then he/she should be aware of the limit of KASLR (can printk
to warn him/her).
> But I might be way off here because of something else I'm missing ...
>
So how do you think about this now? Just leaving a unstable kernel
parameter, or printk some info when crashkernel=x@y fails.
Thanks,
Pingfan
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
On 02/27/19 at 08:39am, Borislav Petkov wrote:
> + Kees.
>
> @Kees, you might want to go upthread a bit for context.
>
> On Wed, Feb 27, 2019 at 09:30:34AM +0800, Baoquan He wrote:
> > Agree that 'crashkernel=x' should be encouraged to use as the first
> > choice when reserve crashkernel. If we decide to not obsolete
> > 'crashkernel=x@y', it will leave a unstable kernel parameter.
>
> Is anyone even talking about obsoleting this?
>
> And if anyone is, anyone can think a bit why we can't do this.
Oh, just a saying about that. If we can't prove this is a good fix,
we may need to persuit resolving it in other ways. Like obsoleting, or
adding a doc to notice user.
As for 'crashkernel=x@y', I rethink about it later, it's userful for user
in some cases. E.g those kinds of big server with hundreds of CPU and
tons of PCI-e devices (we did get report about kdump issue on this kind
of machine, and 'crashkernel=x@y' is needed), they can make use of
'crashkernel=x@y' to specify a memory region to make crash kernel be very
far away from 4G position, e.g 20G. Then if kernel crashed because of some
driver issues, etc, we can exclude the suspicion of crashkernel region.
> > Another worry is that KASLR won't always fail 'crashkernel=x@y',
> > customer may set and check in testing stage, then later in production
> > environment one time of neglect to not check may cause carashed kernel
> > uncaptured.
> >
> > IMHO, 'crashkernel=x@y' is similar to those specified memmap=ss[#$!]nn
> > which have been avoided in boot stage KASLR.
>
> So my worry is that by specifying too many exclusion ranges, we might
> limit the kaslr space too much and make it too predictable. Especially
> since distros slap those things automatically and most users take them
> for granted.
Indeed. There have been so many avoided regions that the original KASLR
code has been covered with a bundle of atches. This add difficulity to
code maintaining of kernel text KASLR. However, most of time they won't
be specified at one time in one system. And on small systems, like virt
guest system, memmap=, mem= may not be added. As for crashkernel=x@y, it
may not expect too much memory, basically 256 MB can cover more than 90%
of systems. Have to admit that code complixity will be increased a
little because of 'crashkernel=x@y' handling, yet it won't weaken KASLR
much.
Personal opinion.
Thanks
Baoquan