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

[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.

Either way, this issue you've pointed out should be fixed in a separate
patch.


Thanks,
Sasha

--
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/