Re: [PATCH] kexec: prevent double free on image allocation failure

[Zhang Yanfei]

于 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Zhang Yanfei <zhangyanfei@xxxxxxxxxxxxxx>
>> ---
>>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/