On 02/21/2013 09:46 PM, Zhang Yanfei wrote:
> ? 2013?02?22? 09:55, Eric W. Biederman ??:
>> Sasha Levin <sasha.levin at oracle.com> writes:
>>
>>> If kimage_normal_alloc() fails to initialize an allocated kimage, it will free
>>> the image but would still set 'rimage', as a result kexec_load will try
>>> to free it again.
>>>
>>> This would explode as part of the freeing process is accessing internal
>>> members which point to uninitialized memory.
>>
>> Agreed.
>>
>> I don't think that failure path has ever actually been exercised.
>>
>> The code is wrong, and it is worth fixing.
>>
>> Andrew I do you think you could queue this up? I don't have a handy tree.
>
>
> I still found another malloc/free problem in this function. So I update the patch.
>
> ---------------------
>
> From 1fb76a35e4109e1435f55048c20ea58622e7f87b Mon Sep 17 00:00:00 2001
> From: Zhang Yanfei <zhangyanfei at cn.fujitsu.com>
> Date: Fri, 22 Feb 2013 10:34:02 +0800
> Subject: [PATCH] kexec: fix allocation problems in function kimage_normal_alloc
>
> The function kimage_normal_alloc() has 2 allocation problems that may cause
> failures:
>
> 1. If kimage_normal_alloc() fails to initialize an allocated kimage, it will
> free the image but would still set 'rimage', as a result kexec_load will
> try to free it again.
>
> This would explode as part of the freeing process is accessing internal
> members which point to uninitialized memory.
>
> 2. If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
> should call kimage_free_page_list() to free allocated pages in
> image->control_pages list before it frees image.
>
> Signed-off-by: Sasha Levin <sasha.levin at oracle.com>
> Signed-off-by: Zhang Yanfei <zhangyanfei at cn.fujitsu.com>
> ---
> kernel/kexec.c | 10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 5e4bd78..f219357 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -223,6 +223,8 @@ out:
>
> }
>
> +static void kimage_free_page_list(struct list_head *list);
> +
> static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
> unsigned long nr_segments,
> struct kexec_segment __user *segments)
> @@ -236,8 +238,6 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
> if (result)
> goto out;
>
> - *rimage = image;
> -
> /*
> * Find a location for the control code buffer, and add it
> * the vector of segments so that it's pages will also be
> @@ -259,10 +259,12 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>
> result = 0;
> out:
> - if (result == 0)
> + if (result == 0) {
> *rimage = image;
> - else
> + } else {
> + kimage_free_page_list(&image->control_pages);
> kfree(image);
> + }
>
> return result;
> }
And if do_kimage_alloc() fails instead of kimage_alloc_control_pages()
you will NULL deref 'image', so now instead of leaking pages the kernel
will explode.
Either way, this issue you've pointed out should be fixed in a separate
patch.
Thanks,
Sasha
