* [PATCH] kexec: prevent double free on image allocation failure
@ 2013-02-22 1:26 Sasha Levin
2013-02-22 1:55 ` Eric W. Biederman
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2013-02-22 1:26 UTC (permalink / raw)
To: ebiederm; +Cc: Sasha Levin, kexec, linux-kernel
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.
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
kernel/kexec.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/kexec.c b/kernel/kexec.c
index 2348bd6..855bfbb 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -242,8 +242,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
--
1.8.1.2
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kexec: prevent double free on image allocation failure
2013-02-22 1:26 [PATCH] kexec: prevent double free on image allocation failure Sasha Levin
@ 2013-02-22 1:55 ` Eric W. Biederman
2013-02-22 2:33 ` Sasha Levin
2013-02-22 2:46 ` Zhang Yanfei
0 siblings, 2 replies; 6+ messages in thread
From: Eric W. Biederman @ 2013-02-22 1:55 UTC (permalink / raw)
To: Sasha Levin; +Cc: Andrew Morton, kexec, linux-kernel
Sasha Levin <sasha.levin@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.
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
> kernel/kexec.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 2348bd6..855bfbb 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -242,8 +242,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
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kexec: prevent double free on image allocation failure
2013-02-22 1:55 ` Eric W. Biederman
@ 2013-02-22 2:33 ` Sasha Levin
2013-02-22 2:46 ` Zhang Yanfei
1 sibling, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2013-02-22 2:33 UTC (permalink / raw)
To: ebiederm; +Cc: Dave Jones, Andrew Morton, kexec, linux-kernel
On 02/21/2013 08:55 PM, ebiederm@xmission.com wrote:
> Sasha Levin <sasha.levin@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.
trinity is actually quite good at hitting that, which is how I discovered
it:
[ 418.138251] Could not allocate control_code_buffer
[ 418.143739] general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 418.147131] Dumping ftrace buffer:
[ 418.147901] (ftrace buffer empty)
[ 418.148697] Modules linked in:
[ 418.153440] CPU 1
[ 418.153440] Pid: 18098, comm: trinity Tainted: G W 3.8.0-next-20130220-sasha-00037-gc07b3b2-dirty #7
[ 418.153440] RIP: 0010:[<ffffffff8119b5a6>] [<ffffffff8119b5a6>] kimage_free_page_list+0x16/0x50
[ 418.153440] RSP: 0018:ffff88009bfade78 EFLAGS: 00010292
[ 418.153440] RAX: 0000000000180004 RBX: 0000000000000002 RCX: 0000000000000000
[ 418.153440] RDX: ffff88009c1a0000 RSI: 0000000000000001 RDI: 6b6b6b6b6b6b6b6b
[ 418.153440] RBP: ffff88009bfade98 R08: 0000000000002782 R09: 0000000000000000
[ 418.153440] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88009c6cb4d0
[ 418.153440] R13: ffff88009c6cb720 R14: ffff88009c6cb4d0 R15: 00000000000000f6
[ 418.153440] FS: 00007fb7eb95b700(0000) GS:ffff8800bb800000(0000) knlGS:0000000000000000
[ 418.153440] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 418.153440] CR2: 00000000004808e0 CR3: 000000009eaaa000 CR4: 00000000000406e0
[ 418.153440] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 418.153440] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 418.153440] Process trinity (pid: 18098, threadinfo ffff88009bfac000, task ffff88009c1a0000)
[ 418.153440] Stack:
[ 418.153440] ffffffff8546e948 0000000000000002 ffff88009c6cb4d0 0000000000000000
[ 418.153440] ffff88009bfaded8 ffffffff8119b60f 0000000000000002 0000000000000002
[ 418.153440] ffff88009c6cb4d0 ffff88009c6cb4d0 fffffffffffffff4 00000000000000f6
[ 418.153440] Call Trace:
[ 418.153440] [<ffffffff8119b60f>] kimage_free+0x2f/0x100
[ 418.153440] [<ffffffff8119c563>] sys_kexec_load+0x593/0x660
[ 418.153440] [<ffffffff8118363d>] ? trace_hardirqs_on+0xd/0x10
[ 418.153440] [<ffffffff83dab7d8>] tracesys+0xe1/0xe6
[ 418.153440] Code: c1 ef 0c 55 48 c1 e7 06 48 89 e5 48 01 c7 e8 82 ff ff ff 5d c3 55 48 89 e5 41 55 49 89 fd 41 54 53 48 83 ec
08 48 8b 3f 49 39 fd <48> 8b 1f 75 08 eb 22 0f 1f 00 48 89 d3 4c 8d 67 e0 e8 54 a6 8a
[ 418.153440] RIP [<ffffffff8119b5a6>] kimage_free_page_list+0x16/0x50
[ 418.153440] RSP <ffff88009bfade78>
[ 418.219646] ---[ end trace 0adb1d6b71fefb29 ]---
Thanks,
Sasha
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kexec: prevent double free on image allocation failure
2013-02-22 1:55 ` Eric W. Biederman
2013-02-22 2:33 ` Sasha Levin
@ 2013-02-22 2:46 ` Zhang Yanfei
2013-02-22 3:41 ` Sasha Levin
1 sibling, 1 reply; 6+ messages in thread
From: Zhang Yanfei @ 2013-02-22 2:46 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Sasha Levin, Andrew Morton, kexec, linux-kernel
于 2013年02月22日 09:55, Eric W. Biederman 写道:
> Sasha Levin <sasha.levin@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@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@oracle.com>
Signed-off-by: Zhang Yanfei <zhangyanfei@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;
}
--
1.7.1
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kexec: prevent double free on image allocation failure
2013-02-22 2:46 ` Zhang Yanfei
@ 2013-02-22 3:41 ` Sasha Levin
2013-02-22 3:58 ` Zhang Yanfei
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2013-02-22 3:41 UTC (permalink / raw)
To: Zhang Yanfei; +Cc: Andrew Morton, kexec, Eric W. Biederman, linux-kernel
On 02/21/2013 09:46 PM, Zhang Yanfei wrote:
> 于 2013年02月22日 09:55, Eric W. Biederman 写道:
>> Sasha Levin <sasha.levin@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@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@oracle.com>
> Signed-off-by: Zhang Yanfei <zhangyanfei@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
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kexec: prevent double free on image allocation failure
2013-02-22 3:41 ` Sasha Levin
@ 2013-02-22 3:58 ` Zhang Yanfei
0 siblings, 0 replies; 6+ messages in thread
From: Zhang Yanfei @ 2013-02-22 3:58 UTC (permalink / raw)
To: Sasha Levin; +Cc: Andrew Morton, kexec, Eric W. Biederman, linux-kernel
于 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@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@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@oracle.com>
>> Signed-off-by: Zhang Yanfei <zhangyanfei@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
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-22 4:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-22 1:26 [PATCH] kexec: prevent double free on image allocation failure Sasha Levin
2013-02-22 1:55 ` Eric W. Biederman
2013-02-22 2:33 ` Sasha Levin
2013-02-22 2:46 ` Zhang Yanfei
2013-02-22 3:41 ` Sasha Levin
2013-02-22 3:58 ` Zhang Yanfei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox