x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
diff mbox series

Message ID 725fa3dc1da2737f0f6188a1a9701bead257ea9d.camel@gmx.de
State New
Headers show
Series
  • x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
Related show

Commit Message

Mike Galbraith April 16, 2021, 12:02 p.m. UTC
[   15.428011] BUG: KASAN: vmalloc-out-of-bounds in crash_setup_memmap_entries+0x17e/0x3a0
[   15.428018] Write of size 8 at addr ffffc90000426008 by task kexec/1187

(gdb) list *crash_setup_memmap_entries+0x17e
0xffffffff8107cafe is in crash_setup_memmap_entries (arch/x86/kernel/crash.c:322).
317                                      unsigned long long mend)
318     {
319             unsigned long start, end;
320
321             cmem->ranges[0].start = mstart;
322             cmem->ranges[0].end = mend;
323             cmem->nr_ranges = 1;
324
325             /* Exclude elf header region */
326             start = image->arch.elf_load_addr;
(gdb)

Append missing struct crash_mem_range to cmem.

Signed-off-by: Mike Galbraith <efault@gmx.de>
---
 arch/x86/kernel/crash.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Borislav Petkov April 16, 2021, 12:16 p.m. UTC | #1
On Fri, Apr 16, 2021 at 02:02:07PM +0200, Mike Galbraith wrote:
> [   15.428011] BUG: KASAN: vmalloc-out-of-bounds in crash_setup_memmap_entries+0x17e/0x3a0
> [   15.428018] Write of size 8 at addr ffffc90000426008 by task kexec/1187
> 
> (gdb) list *crash_setup_memmap_entries+0x17e
> 0xffffffff8107cafe is in crash_setup_memmap_entries (arch/x86/kernel/crash.c:322).
> 317                                      unsigned long long mend)
> 318     {
> 319             unsigned long start, end;
> 320
> 321             cmem->ranges[0].start = mstart;
> 322             cmem->ranges[0].end = mend;
> 323             cmem->nr_ranges = 1;
> 324
> 325             /* Exclude elf header region */
> 326             start = image->arch.elf_load_addr;
> (gdb)
> 
> Append missing struct crash_mem_range to cmem.

This is winning this year's contest for most laconic patch commit
message! :-)

Please be more verbose and structure your commit message like this:

Problem is A.

It happens because of B.

Fix it by doing C.

(Potentially do D).

For more detailed info, see
Documentation/process/submitting-patches.rst, Section "2) Describe your
changes".

Thx.
Mike Galbraith April 16, 2021, 1:16 p.m. UTC | #2
On Fri, 2021-04-16 at 14:16 +0200, Borislav Petkov wrote:
>
> Please be more verbose and structure your commit message like this:

Hrmph, I thought it was too verbose for dinky one-liner if anything.  I
showed the complaint along with an 8x10 color glossy crime scene photo,
then explained why it happened and what to do about it with a perhaps
terse but perfectly clear sentence.

	-Mike
Borislav Petkov April 16, 2021, 2:44 p.m. UTC | #3
On Fri, Apr 16, 2021 at 03:16:07PM +0200, Mike Galbraith wrote:
> On Fri, 2021-04-16 at 14:16 +0200, Borislav Petkov wrote:
> >
> > Please be more verbose and structure your commit message like this:
> 
> Hrmph, I thought it was too verbose for dinky one-liner if anything. 

Please look at how other commit messages in tip have free text - not
only tools output.

Also, this looks like a fix for some previous commit. Please dig out
which commit introduced the issue and put its commit ID in a Fixes: tag
above your S-o-B.

If you don't have time or desire to do that, you can say so and I'll do
it myself when I get a chance.

Thx.
Mike Galbraith April 16, 2021, 3:13 p.m. UTC | #4
On Fri, 2021-04-16 at 16:44 +0200, Borislav Petkov wrote:
> On Fri, Apr 16, 2021 at 03:16:07PM +0200, Mike Galbraith wrote:
> > On Fri, 2021-04-16 at 14:16 +0200, Borislav Petkov wrote:
> > >
> > > Please be more verbose and structure your commit message like this:
> >
> > Hrmph, I thought it was too verbose for dinky one-liner if anything.
>
> Please look at how other commit messages in tip have free text - not
> only tools output.
>
> Also, this looks like a fix for some previous commit. Please dig out
> which commit introduced the issue and put its commit ID in a Fixes: tag
> above your S-o-B.
>
> If you don't have time or desire to do that, you can say so and I'll do
> it myself when I get a chance.

Ok, bin it for the nonce.

	-Mike
Thomas Gleixner April 16, 2021, 9:44 p.m. UTC | #5
On Fri, Apr 16 2021 at 17:13, Mike Galbraith wrote:
> On Fri, 2021-04-16 at 16:44 +0200, Borislav Petkov wrote:
>> On Fri, Apr 16, 2021 at 03:16:07PM +0200, Mike Galbraith wrote:
>> > On Fri, 2021-04-16 at 14:16 +0200, Borislav Petkov wrote:
>> > >
>> > > Please be more verbose and structure your commit message like this:
>> >
>> > Hrmph, I thought it was too verbose for dinky one-liner if anything.
>>
>> Please look at how other commit messages in tip have free text - not
>> only tools output.
>>
>> Also, this looks like a fix for some previous commit. Please dig out
>> which commit introduced the issue and put its commit ID in a Fixes: tag
>> above your S-o-B.
>>
>> If you don't have time or desire to do that, you can say so and I'll do
>> it myself when I get a chance.
>
> Ok, bin it for the nonce.

Can all of you involved stop this sandpit fight and do something useful
to fix that obvious bug already?

OMG
Mike Galbraith April 17, 2021, 12:05 a.m. UTC | #6
On Fri, 2021-04-16 at 23:44 +0200, Thomas Gleixner wrote:
>
> Can all of you involved stop this sandpit fight and do something useful
> to fix that obvious bug already?

?? We're not fighting afaik.  Boris hated my changelog enough to offer
to write a better one, and I'm fine with that.  It's a seven year old
*latent* buglet of microscopic proportions, hardly a pressing issue.

	-Mike
Borislav Petkov April 19, 2021, 8:52 a.m. UTC | #7
Here's an attempt to explain what this fixes:

---
From: Mike Galbraith <efault@gmx.de>
Date: Fri, 16 Apr 2021 14:02:07 +0200
Subject: [PATCH] x86/crash: Fix crash_setup_memmap_entries() out-of-bounds
 access

Commit in Fixes: added support for kexec-ing a kernel on panic using a
new system call. As part of it, it does prepare a memory map for the new
kernel.

However, while doing so, it wrongly accesses memory it has not
allocated: it accesses the first element of the cmem->ranges[] array in
memmap_exclude_ranges() but it has not allocated the memory for it in
crash_setup_memmap_entries(). As KASAN reports:

  BUG: KASAN: vmalloc-out-of-bounds in crash_setup_memmap_entries+0x17e/0x3a0
  Write of size 8 at addr ffffc90000426008 by task kexec/1187

  (gdb) list *crash_setup_memmap_entries+0x17e
  0xffffffff8107cafe is in crash_setup_memmap_entries (arch/x86/kernel/crash.c:322).
  317                                      unsigned long long mend)
  318     {
  319             unsigned long start, end;
  320
  321             cmem->ranges[0].start = mstart;
  322             cmem->ranges[0].end = mend;
  323             cmem->nr_ranges = 1;
  324
  325             /* Exclude elf header region */
  326             start = image->arch.elf_load_addr;
  (gdb)

Make sure the ranges array becomes a single element allocated.

 [ bp: Write a proper commit message. ]

Fixes: dd5f726076cc ("kexec: support for kexec on panic using new system call")
Signed-off-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/725fa3dc1da2737f0f6188a1a9701bead257ea9d.camel@gmx.de
---
 arch/x86/kernel/crash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index a8f3af257e26..b1deacbeb266 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -337,7 +337,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
 	struct crash_memmap_data cmd;
 	struct crash_mem *cmem;
 
-	cmem = vzalloc(sizeof(struct crash_mem));
+	cmem = vzalloc(struct_size(cmem, ranges, 1));
 	if (!cmem)
 		return -ENOMEM;
DaveYoung April 19, 2021, 9:37 a.m. UTC | #8
On 04/19/21 at 10:52am, Borislav Petkov wrote:
> Here's an attempt to explain what this fixes:
> 
> ---
> From: Mike Galbraith <efault@gmx.de>
> Date: Fri, 16 Apr 2021 14:02:07 +0200
> Subject: [PATCH] x86/crash: Fix crash_setup_memmap_entries() out-of-bounds
>  access
> 
> Commit in Fixes: added support for kexec-ing a kernel on panic using a
> new system call. As part of it, it does prepare a memory map for the new
> kernel.
> 
> However, while doing so, it wrongly accesses memory it has not
> allocated: it accesses the first element of the cmem->ranges[] array in
> memmap_exclude_ranges() but it has not allocated the memory for it in
> crash_setup_memmap_entries(). As KASAN reports:
> 
>   BUG: KASAN: vmalloc-out-of-bounds in crash_setup_memmap_entries+0x17e/0x3a0
>   Write of size 8 at addr ffffc90000426008 by task kexec/1187
> 
>   (gdb) list *crash_setup_memmap_entries+0x17e
>   0xffffffff8107cafe is in crash_setup_memmap_entries (arch/x86/kernel/crash.c:322).
>   317                                      unsigned long long mend)
>   318     {
>   319             unsigned long start, end;
>   320
>   321             cmem->ranges[0].start = mstart;
>   322             cmem->ranges[0].end = mend;
>   323             cmem->nr_ranges = 1;
>   324
>   325             /* Exclude elf header region */
>   326             start = image->arch.elf_load_addr;
>   (gdb)
> 
> Make sure the ranges array becomes a single element allocated.
> 
>  [ bp: Write a proper commit message. ]

Reviewed-by: Dave Young <dyoung@redhat.com>

> 
> Fixes: dd5f726076cc ("kexec: support for kexec on panic using new system call")
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: https://lkml.kernel.org/r/725fa3dc1da2737f0f6188a1a9701bead257ea9d.camel@gmx.de
> ---
>  arch/x86/kernel/crash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index a8f3af257e26..b1deacbeb266 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -337,7 +337,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>  	struct crash_memmap_data cmd;
>  	struct crash_mem *cmem;
>  
> -	cmem = vzalloc(sizeof(struct crash_mem));
> +	cmem = vzalloc(struct_size(cmem, ranges, 1));
>  	if (!cmem)
>  		return -ENOMEM;
>  
> -- 
> 2.29.2
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

Thanks
Dave

Patch
diff mbox series

--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -337,7 +337,7 @@  int crash_setup_memmap_entries(struct ki
 	struct crash_memmap_data cmd;
 	struct crash_mem *cmem;

-	cmem = vzalloc(sizeof(struct crash_mem));
+	cmem = vzalloc(struct_size(cmem, ranges, 1));
 	if (!cmem)
 		return -ENOMEM;