? 2013?02?22? 11:41, Sasha Levin ??:
> 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.
Oh, I missed this.
>
> Either way, this issue you've pointed out should be fixed in a separate
> patch.
>
>
OK?I will send another patch.
Thanks
Zhang
