linux-kernel - Re: [PATCH] kexec: fix memory leak in function kimage_normal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 23 Feb 2013 21:48:48 +0800
From:	Yanfei Zhang <zhangyanfei.yes@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Zhang Yanfei <zhangyanfei@...fujitsu.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc

2013/2/23 Andrew Morton <akpm@...ux-foundation.org>:
> On Fri, 22 Feb 2013 12:36:13 +0800
> Zhang Yanfei <zhangyanfei@...fujitsu.com> wrote:
>
>> 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.
>>
>> ...
>>
>> --- 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)
>> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>>                                          get_order(KEXEC_CONTROL_PAGE_SIZE));
>>       if (!image->control_code_page) {
>>               printk(KERN_ERR "Could not allocate control_code_buffer\n");
>> -             goto out;
>> +             goto out_free;
>>       }
>>
>>       image->swap_page = kimage_alloc_control_pages(image, 0);
>>       if (!image->swap_page) {
>>               printk(KERN_ERR "Could not allocate swap buffer\n");
>> -             goto out;
>> +             goto out_free;
>>       }
>>
>> -     result = 0;
>> - out:
>> -     if (result == 0)
>> -             *rimage = image;
>> -     else
>> -             kfree(image);
>> +     *rimage = image;
>> +     return 0;
>>
>> +out_free:
>> +     kimage_free_page_list(&image->control_pages);
>> +     kfree(image);
>> +out:
>>       return result;
>>  }
>
> kimage_alloc_normal_control_pages() won't add any pages to the image if
> one of its allocation attemtps failed.  So afaict the first `goto
> out_free' could be just `goto out'.

Nop, the first 'goto out_free' is to free allocated 'image' if the allocation of
image->control_code_page failed. If changed to 'goto out', 'image' cannot
be freed and leads to memory leak.

>
> The second `goto out_free' does appear to be needed: it frees the pages
> allocated by the first call to kimage_alloc_control_pages().  I think.
> The kimage_alloc_control_pages() handling of image->type is a bit
> twisty.

Yeah, it could just call kimage_alloc_normal_control_pages directly.

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