* [PATCH 1/1] pagemap: fix buffer overflow in add_page_map()
@ 2013-08-09 5:16 yonghua zheng
2013-08-09 20:33 ` Naoya Horiguchi
2013-08-12 23:05 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: yonghua zheng @ 2013-08-09 5:16 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-mm, Motohiro KOSAKI
Hi,
Recently we met quite a lot of random kernel panic issues after enable
CONFIG_PROC_PAGE_MONITOR in kernel, after debuggint sometime we found
this has something to do with following bug in pagemap:
In struc pagemapread:
struct pagemapread {
int pos, len;
pagemap_entry_t *buffer;
bool v2;
};
pos is number of PM_ENTRY_BYTES in buffer, but len is the size of buffer,
it is a mistake to compare pos and len in add_page_map() for checking
buffer is full or not, and this can lead to buffer overflow and random
kernel panic issue.
Correct len to be total number of PM_ENTRY_BYTES in buffer.
Signed-off-by: Yonghua Zheng <younghua.zheng@gmail.com>
---
fs/proc/task_mmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index dbf61f6..cb98853 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1116,8 +1116,8 @@ static ssize_t pagemap_read(struct file *file,
char __user *buf,
goto out_task;
pm.v2 = soft_dirty_cleared;
- pm.len = PM_ENTRY_BYTES * (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);
- pm.buffer = kmalloc(pm.len, GFP_TEMPORARY);
+ pm.len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);
+ pm.buffer = kmalloc(pm.len * PM_ENTRY_BYTES, GFP_TEMPORARY);
ret = -ENOMEM;
if (!pm.buffer)
goto out_task;
--
1.7.9.5
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() 2013-08-09 5:16 [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() yonghua zheng @ 2013-08-09 20:33 ` Naoya Horiguchi 2013-08-10 0:49 ` yonghua zheng 2013-08-12 23:05 ` Andrew Morton 1 sibling, 1 reply; 6+ messages in thread From: Naoya Horiguchi @ 2013-08-09 20:33 UTC (permalink / raw) To: yonghua zheng; +Cc: linux-kernel, linux-mm, Motohiro KOSAKI On Fri, Aug 09, 2013 at 01:16:41PM +0800, yonghua zheng wrote: > Hi, > > Recently we met quite a lot of random kernel panic issues after enable > CONFIG_PROC_PAGE_MONITOR in kernel, after debuggint sometime we found > this has something to do with following bug in pagemap: > > In struc pagemapread: > > struct pagemapread { > int pos, len; > pagemap_entry_t *buffer; > bool v2; > }; > > pos is number of PM_ENTRY_BYTES in buffer, but len is the size of buffer, > it is a mistake to compare pos and len in add_page_map() for checking s/add_page_map/add_to_pagemap/ ? > buffer is full or not, and this can lead to buffer overflow and random > kernel panic issue. > > Correct len to be total number of PM_ENTRY_BYTES in buffer. > > Signed-off-by: Yonghua Zheng <younghua.zheng@gmail.com> You can find coding style violation with scripts/checkpatch.pl. And I think this patch is worth going into -stable trees (maybe since 2.6.34.) The fix itself looks fine to me. Thanks, Naoya Horiguchi > --- > fs/proc/task_mmu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index dbf61f6..cb98853 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1116,8 +1116,8 @@ static ssize_t pagemap_read(struct file *file, > char __user *buf, > goto out_task; > > pm.v2 = soft_dirty_cleared; > - pm.len = PM_ENTRY_BYTES * (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > - pm.buffer = kmalloc(pm.len, GFP_TEMPORARY); > + pm.len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > + pm.buffer = kmalloc(pm.len * PM_ENTRY_BYTES, GFP_TEMPORARY); > ret = -ENOMEM; > if (!pm.buffer) > goto out_task; > > -- > 1.7.9.5 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() 2013-08-09 20:33 ` Naoya Horiguchi @ 2013-08-10 0:49 ` yonghua zheng 2013-08-10 16:47 ` KOSAKI Motohiro 2013-08-12 18:15 ` Naoya Horiguchi 0 siblings, 2 replies; 6+ messages in thread From: yonghua zheng @ 2013-08-10 0:49 UTC (permalink / raw) To: Naoya Horiguchi; +Cc: linux-kernel, linux-mm, Motohiro KOSAKI Update the patch according to Naoya's comment, I also run ./scripts/checkpatch.pl, and it passed ;D. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() 2013-08-10 0:49 ` yonghua zheng @ 2013-08-10 16:47 ` KOSAKI Motohiro 2013-08-12 18:15 ` Naoya Horiguchi 1 sibling, 0 replies; 6+ messages in thread From: KOSAKI Motohiro @ 2013-08-10 16:47 UTC (permalink / raw) To: yonghua zheng; +Cc: Naoya Horiguchi, LKML, linux-mm On Fri, Aug 9, 2013 at 8:49 PM, yonghua zheng <younghua.zheng@gmail.com> wrote: > Update the patch according to Naoya's comment, I also run > ./scripts/checkpatch.pl, and it passed ;D. > > From 96826b0fdf9ec6d6e16c2c595f371dbb841250f7 Mon Sep 17 00:00:00 2001 > From: Yonghua Zheng <younghua.zheng@gmail.com> > Date: Mon, 5 Aug 2013 12:12:24 +0800 > Subject: [PATCH 1/1] pagemap: fix buffer overflow in add_to_pagemap() > > In struc pagemapread: > > struct pagemapread { > int pos, len; > pagemap_entry_t *buffer; > bool v2; > }; > > pos is number of PM_ENTRY_BYTES in buffer, but len is the size of buffer, > it is a mistake to compare pos and len in add_to_pagemap() for checking > buffer is full or not, and this can lead to buffer overflow and random > kernel panic issue. > > Correct len to be total number of PM_ENTRY_BYTES in buffer. > > Signed-off-by: Yonghua Zheng <younghua.zheng@gmail.com> Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() 2013-08-10 0:49 ` yonghua zheng 2013-08-10 16:47 ` KOSAKI Motohiro @ 2013-08-12 18:15 ` Naoya Horiguchi 1 sibling, 0 replies; 6+ messages in thread From: Naoya Horiguchi @ 2013-08-12 18:15 UTC (permalink / raw) To: yonghua zheng; +Cc: linux-kernel, linux-mm, Motohiro KOSAKI On Sat, Aug 10, 2013 at 08:49:34AM +0800, yonghua zheng wrote: > Update the patch according to Naoya's comment, I also run > ./scripts/checkpatch.pl, and it passed ;D. Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> # Sorry if I missed something, but I guess that your MTA might replace tabs # with spaces. Documentation/email-clients.txt can be helpful to solve it. > > From 96826b0fdf9ec6d6e16c2c595f371dbb841250f7 Mon Sep 17 00:00:00 2001 > From: Yonghua Zheng <younghua.zheng@gmail.com> > Date: Mon, 5 Aug 2013 12:12:24 +0800 > Subject: [PATCH 1/1] pagemap: fix buffer overflow in add_to_pagemap() > > In struc pagemapread: > > struct pagemapread { > int pos, len; > pagemap_entry_t *buffer; > bool v2; > }; > > pos is number of PM_ENTRY_BYTES in buffer, but len is the size of buffer, > it is a mistake to compare pos and len in add_to_pagemap() for checking > buffer is full or not, and this can lead to buffer overflow and random > kernel panic issue. > > Correct len to be total number of PM_ENTRY_BYTES in buffer. > > Signed-off-by: Yonghua Zheng <younghua.zheng@gmail.com> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index dbf61f6..cb98853 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1116,8 +1116,8 @@ static ssize_t pagemap_read(struct file *file, > char __user *buf, > goto out_task; > > pm.v2 = soft_dirty_cleared; > - pm.len = PM_ENTRY_BYTES * (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > - pm.buffer = kmalloc(pm.len, GFP_TEMPORARY); > + pm.len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > + pm.buffer = kmalloc(pm.len * PM_ENTRY_BYTES, GFP_TEMPORARY); > ret = -ENOMEM; > if (!pm.buffer) > goto out_task; > -- > 1.7.9.5 > > On Sat, Aug 10, 2013 at 4:33 AM, Naoya Horiguchi > <n-horiguchi@ah.jp.nec.com> wrote: > > On Fri, Aug 09, 2013 at 01:16:41PM +0800, yonghua zheng wrote: > >> Hi, > >> > >> Recently we met quite a lot of random kernel panic issues after enable > >> CONFIG_PROC_PAGE_MONITOR in kernel, after debuggint sometime we found > >> this has something to do with following bug in pagemap: > >> > >> In struc pagemapread: > >> > >> struct pagemapread { > >> int pos, len; > >> pagemap_entry_t *buffer; > >> bool v2; > >> }; > >> > >> pos is number of PM_ENTRY_BYTES in buffer, but len is the size of buffer, > >> it is a mistake to compare pos and len in add_page_map() for checking > > > > s/add_page_map/add_to_pagemap/ ? > > > >> buffer is full or not, and this can lead to buffer overflow and random > >> kernel panic issue. > >> > >> Correct len to be total number of PM_ENTRY_BYTES in buffer. > >> > >> Signed-off-by: Yonghua Zheng <younghua.zheng@gmail.com> > > > > You can find coding style violation with scripts/checkpatch.pl. > > And I think this patch is worth going into -stable trees > > (maybe since 2.6.34.) > > > > The fix itself looks fine to me. > > > > Thanks, > > Naoya Horiguchi > > > >> --- > >> fs/proc/task_mmu.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > >> index dbf61f6..cb98853 100644 > >> --- a/fs/proc/task_mmu.c > >> +++ b/fs/proc/task_mmu.c > >> @@ -1116,8 +1116,8 @@ static ssize_t pagemap_read(struct file *file, > >> char __user *buf, > >> goto out_task; > >> > >> pm.v2 = soft_dirty_cleared; > >> - pm.len = PM_ENTRY_BYTES * (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > >> - pm.buffer = kmalloc(pm.len, GFP_TEMPORARY); > >> + pm.len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > >> + pm.buffer = kmalloc(pm.len * PM_ENTRY_BYTES, GFP_TEMPORARY); > >> ret = -ENOMEM; > >> if (!pm.buffer) > >> goto out_task; > >> > >> -- > >> 1.7.9.5 > >> > >> -- > >> To unsubscribe, send a message with 'unsubscribe linux-mm' in > >> the body to majordomo@kvack.org. For more info on Linux MM, > >> see: http://www.linux-mm.org/ . > >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > >> > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() 2013-08-09 5:16 [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() yonghua zheng 2013-08-09 20:33 ` Naoya Horiguchi @ 2013-08-12 23:05 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Andrew Morton @ 2013-08-12 23:05 UTC (permalink / raw) To: yonghua zheng; +Cc: linux-kernel, linux-mm, Motohiro KOSAKI On Fri, 9 Aug 2013 13:16:41 +0800 yonghua zheng <younghua.zheng@gmail.com> wrote: > Hi, > > Recently we met quite a lot of random kernel panic issues after enable > CONFIG_PROC_PAGE_MONITOR in kernel, after debuggint sometime we found > this has something to do with following bug in pagemap: > > In struc pagemapread: > > struct pagemapread { > int pos, len; > pagemap_entry_t *buffer; > bool v2; > }; > > pos is number of PM_ENTRY_BYTES in buffer, but len is the size of buffer, > it is a mistake to compare pos and len in add_page_map() for checking > buffer is full or not, and this can lead to buffer overflow and random > kernel panic issue. > > Correct len to be total number of PM_ENTRY_BYTES in buffer. > > Signed-off-by: Yonghua Zheng <younghua.zheng@gmail.com> > --- > fs/proc/task_mmu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index dbf61f6..cb98853 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1116,8 +1116,8 @@ static ssize_t pagemap_read(struct file *file, > char __user *buf, > goto out_task; > > pm.v2 = soft_dirty_cleared; > - pm.len = PM_ENTRY_BYTES * (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > - pm.buffer = kmalloc(pm.len, GFP_TEMPORARY); > + pm.len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); > + pm.buffer = kmalloc(pm.len * PM_ENTRY_BYTES, GFP_TEMPORARY); > ret = -ENOMEM; > if (!pm.buffer) > goto out_task; Yes, that's a bug. I'd propose this addition to your fix: From: Andrew Morton <akpm@linux-foundation.org> Subject: pagemap-fix-buffer-overflow-in-add_page_map-fix document pagemapread.pos and .len units, fix PM_ENTRY_BYTES definition Cc: Yonghua Zheng <younghua.zheng@gmail.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- fs/proc/task_mmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff -puN fs/proc/task_mmu.c~pagemap-fix-buffer-overflow-in-add_page_map-fix fs/proc/task_mmu.c --- a/fs/proc/task_mmu.c~pagemap-fix-buffer-overflow-in-add_page_map-fix +++ a/fs/proc/task_mmu.c @@ -868,7 +868,7 @@ typedef struct { } pagemap_entry_t; struct pagemapread { - int pos, len; + int pos, len; /* units: PM_ENTRY_BYTES, not bytes */ pagemap_entry_t *buffer; bool v2; }; @@ -876,7 +876,7 @@ struct pagemapread { #define PAGEMAP_WALK_SIZE (PMD_SIZE) #define PAGEMAP_WALK_MASK (PMD_MASK) -#define PM_ENTRY_BYTES sizeof(u64) +#define PM_ENTRY_BYTES sizeof(pagemap_entry_t) #define PM_STATUS_BITS 3 #define PM_STATUS_OFFSET (64 - PM_STATUS_BITS) #define PM_STATUS_MASK (((1LL << PM_STATUS_BITS) - 1) << PM_STATUS_OFFSET) _ btw, your email client wordwraps the patches and replaces tabs with spaces. Please fix that up for next time? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-12 23:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-09 5:16 [PATCH 1/1] pagemap: fix buffer overflow in add_page_map() yonghua zheng 2013-08-09 20:33 ` Naoya Horiguchi 2013-08-10 0:49 ` yonghua zheng 2013-08-10 16:47 ` KOSAKI Motohiro 2013-08-12 18:15 ` Naoya Horiguchi 2013-08-12 23:05 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox