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