| Message ID | 20230102103917.20987-1-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/kexec: fix double vfree of image->elf_headers | expand |
On 1/2/23 11:39, Vlastimil Babka wrote: > An investigation of a "Trying to vfree() nonexistent vm area" bug > occurring in arch_kimage_file_post_load_cleanup() doing a > vfree(image->elf_headers) in our 5.14-based kernel yielded the following > double vfree() scenario, also present in mainline: > > SYSCALL_DEFINE5(kexec_file_load) > kimage_file_alloc_init() > kimage_file_prepare_segments() > arch_kexec_kernel_image_probe() > kexec_image_load_default() > kexec_bzImage64_ops.load() > bzImage64_load() > crash_load_segments() > prepare_elf_headers(image, &kbuf.buffer, &kbuf.bufsz); > image->elf_headers = kbuf.buffer; > ret = kexec_add_buffer(&kbuf); > if (ret) vfree((void *)image->elf_headers); // first vfree() > if (ret) kimage_file_post_load_cleanup() > vfree(image->elf_headers); // second vfree() > > AFAICS the scenario is possible since v5.19 commit b3e34a47f989 > ("x86/kexec: fix memory leak of elf header buffer") that was marked for > stable and also was backported to our kernel. > > Fix the problem by setting the pointer to NULL after the first vfree(). > Also set elf_headers_sz to 0, as kimage_file_post_load_cleanup() does. > > Fixes: b3e34a47f989 ("x86/kexec: fix memory leak of elf header buffer") > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > Cc: Baoquan He <bhe@redhat.com> > Cc: Dave Young <dyoung@redhat.com> > Cc: <stable@vger.kernel.org> Takashi told me he sent a slightly different fix already in November: https://lore.kernel.org/all/20221122115122.13937-1-tiwai@suse.de/ Seems it wasn't picked up? You might pick his then, as Baoquan acked it, and it's removing code, not adding it. > --- > arch/x86/kernel/crash.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c > index 9730c88530fc..0d651c05a49e 100644 > --- a/arch/x86/kernel/crash.c > +++ b/arch/x86/kernel/crash.c > @@ -403,6 +403,8 @@ int crash_load_segments(struct kimage *image) > ret = kexec_add_buffer(&kbuf); > if (ret) { > vfree((void *)image->elf_headers); > + image->elf_headers = NULL; > + image->elf_headers_sz = 0; > return ret; > } > image->elf_load_addr = kbuf.mem;
* Vlastimil Babka <vbabka@suse.cz> wrote: > On 1/2/23 11:39, Vlastimil Babka wrote: > > An investigation of a "Trying to vfree() nonexistent vm area" bug > > occurring in arch_kimage_file_post_load_cleanup() doing a > > vfree(image->elf_headers) in our 5.14-based kernel yielded the following > > double vfree() scenario, also present in mainline: > > > > SYSCALL_DEFINE5(kexec_file_load) > > kimage_file_alloc_init() > > kimage_file_prepare_segments() > > arch_kexec_kernel_image_probe() > > kexec_image_load_default() > > kexec_bzImage64_ops.load() > > bzImage64_load() > > crash_load_segments() > > prepare_elf_headers(image, &kbuf.buffer, &kbuf.bufsz); > > image->elf_headers = kbuf.buffer; > > ret = kexec_add_buffer(&kbuf); > > if (ret) vfree((void *)image->elf_headers); // first vfree() > > if (ret) kimage_file_post_load_cleanup() > > vfree(image->elf_headers); // second vfree() > > > > AFAICS the scenario is possible since v5.19 commit b3e34a47f989 > > ("x86/kexec: fix memory leak of elf header buffer") that was marked for > > stable and also was backported to our kernel. > > > > Fix the problem by setting the pointer to NULL after the first vfree(). > > Also set elf_headers_sz to 0, as kimage_file_post_load_cleanup() does. > > > > Fixes: b3e34a47f989 ("x86/kexec: fix memory leak of elf header buffer") > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > Cc: Baoquan He <bhe@redhat.com> > > Cc: Dave Young <dyoung@redhat.com> > > Cc: <stable@vger.kernel.org> > > Takashi told me he sent a slightly different fix already in November: > https://lore.kernel.org/all/20221122115122.13937-1-tiwai@suse.de/ > > Seems it wasn't picked up? You might pick his then, as Baoquan acked it, and > it's removing code, not adding it. Thanks, indeed we missed that fix - Boris picked up that version in tip:x86/urgent, via: d00dd2f2645d ("x86/kexec: Fix double-free of elf header buffer") Ingo
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c index 9730c88530fc..0d651c05a49e 100644 --- a/arch/x86/kernel/crash.c +++ b/arch/x86/kernel/crash.c @@ -403,6 +403,8 @@ int crash_load_segments(struct kimage *image) ret = kexec_add_buffer(&kbuf); if (ret) { vfree((void *)image->elf_headers); + image->elf_headers = NULL; + image->elf_headers_sz = 0; return ret; } image->elf_load_addr = kbuf.mem;
An investigation of a "Trying to vfree() nonexistent vm area" bug occurring in arch_kimage_file_post_load_cleanup() doing a vfree(image->elf_headers) in our 5.14-based kernel yielded the following double vfree() scenario, also present in mainline: SYSCALL_DEFINE5(kexec_file_load) kimage_file_alloc_init() kimage_file_prepare_segments() arch_kexec_kernel_image_probe() kexec_image_load_default() kexec_bzImage64_ops.load() bzImage64_load() crash_load_segments() prepare_elf_headers(image, &kbuf.buffer, &kbuf.bufsz); image->elf_headers = kbuf.buffer; ret = kexec_add_buffer(&kbuf); if (ret) vfree((void *)image->elf_headers); // first vfree() if (ret) kimage_file_post_load_cleanup() vfree(image->elf_headers); // second vfree() AFAICS the scenario is possible since v5.19 commit b3e34a47f989 ("x86/kexec: fix memory leak of elf header buffer") that was marked for stable and also was backported to our kernel. Fix the problem by setting the pointer to NULL after the first vfree(). Also set elf_headers_sz to 0, as kimage_file_post_load_cleanup() does. Fixes: b3e34a47f989 ("x86/kexec: fix memory leak of elf header buffer") Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Cc: Baoquan He <bhe@redhat.com> Cc: Dave Young <dyoung@redhat.com> Cc: <stable@vger.kernel.org> --- arch/x86/kernel/crash.c | 2 ++ 1 file changed, 2 insertions(+)