Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
On 04/19/21 at 10:52am, Borislav Petkov wrote: > Here's an attempt to explain what this fixes: > > --- > From: Mike Galbraith > 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 c9426008 by task kexec/1187 > > (gdb) list *crash_setup_memmap_entries+0x17e > 0x8107cafe 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 > > Fixes: dd5f726076cc ("kexec: support for kexec on panic using new system > call") > Signed-off-by: Mike Galbraith > Signed-off-by: Borislav Petkov > Link: > https://lkml.kernel.org/r/725fa3dc1da2737f0f6188a1a9701bead257ea9d.ca...@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
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
Here's an attempt to explain what this fixes: --- From: Mike Galbraith 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 c9426008 by task kexec/1187 (gdb) list *crash_setup_memmap_entries+0x17e 0x8107cafe 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 Signed-off-by: Borislav Petkov Link: https://lkml.kernel.org/r/725fa3dc1da2737f0f6188a1a9701bead257ea9d.ca...@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
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
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
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
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
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
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
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
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. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
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
Re: [patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
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 c9426008 by task kexec/1187 > > (gdb) list *crash_setup_memmap_entries+0x17e > 0x8107cafe 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. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
[patch] x86/crash: fix crash_setup_memmap_entries() out-of-bounds access
[ 15.428011] BUG: KASAN: vmalloc-out-of-bounds in crash_setup_memmap_entries+0x17e/0x3a0 [ 15.428018] Write of size 8 at addr c9426008 by task kexec/1187 (gdb) list *crash_setup_memmap_entries+0x17e 0x8107cafe 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 --- arch/x86/kernel/crash.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- 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;