LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] fix page leak during core dump
@ 2007-03-29 20:39 Brian Pomerantz
  2007-03-30 20:43 ` Andrew Morton
  0 siblings, 1 reply; 7+ messages in thread
From: Brian Pomerantz @ 2007-03-29 20:39 UTC (permalink / raw)
  To: viro; +Cc: linux-kernel


When the dump cannot occur most likely because of a full file system
and the page to be written is the zero page, the call to
page_cache_release() is missed.

Signed-off-by: Brian Pomerantz <bapper@mvista.com>

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index a2fceba..9cc4f0a 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1704,7 +1704,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file)
 				DUMP_SEEK(PAGE_SIZE);
 			} else {
 				if (page == ZERO_PAGE(addr)) {
-					DUMP_SEEK(PAGE_SIZE);
+					if (!dump_seek(file, PAGE_SIZE)) {
+						page_cache_release(page);
+						goto end_coredump;
+					}
 				} else {
 					void *kaddr;
 					flush_cache_page(vma, addr,

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fix page leak during core dump
  2007-03-29 20:39 [PATCH] fix page leak during core dump Brian Pomerantz
@ 2007-03-30 20:43 ` Andrew Morton
  2007-03-30 22:01   ` Hugh Dickins
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2007-03-30 20:43 UTC (permalink / raw)
  To: Brian Pomerantz; +Cc: viro, linux-kernel, Nick Piggin, Hugh Dickins

On Thu, 29 Mar 2007 13:39:13 -0700
Brian Pomerantz <bapper@piratehaven.org> wrote:

> When the dump cannot occur most likely because of a full file system
> and the page to be written is the zero page, the call to
> page_cache_release() is missed.
> 
> Signed-off-by: Brian Pomerantz <bapper@mvista.com>
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index a2fceba..9cc4f0a 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1704,7 +1704,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file)
>  				DUMP_SEEK(PAGE_SIZE);
>  			} else {
>  				if (page == ZERO_PAGE(addr)) {
> -					DUMP_SEEK(PAGE_SIZE);
> +					if (!dump_seek(file, PAGE_SIZE)) {
> +						page_cache_release(page);
> +						goto end_coredump;
> +					}

Oh for gawds sake I wish we could be rid of those idiotic macros :(

This patch looks OK to me, although a refcount leak on the ZERO_PAGE is
special, because that page is PageReserved().

It used to be the case that we'd ignore attempts to change the refcount on
reserved pages (or at least on the ZERO_PAGE), but we changed that, so we
now actually refcount the ZERO_PAGE.  (I think, from a quick read of the
code.  This contradicts my memory of how it works).

So I expect the net effect here is that a sufficiently determined attacker
can overflow the ZERO_PAGE's refcount, thus causing it to be "freed".  The
page allocator won't actually free the page due to PG_Reserved, but it'll
all become very noisy.

Nick, Hugh: agree?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fix page leak during core dump
  2007-03-30 20:43 ` Andrew Morton
@ 2007-03-30 22:01   ` Hugh Dickins
  2007-03-30 22:13     ` Andrew Morton
  2007-03-31 12:57     ` David Howells
  0 siblings, 2 replies; 7+ messages in thread
From: Hugh Dickins @ 2007-03-30 22:01 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Brian Pomerantz, viro, linux-kernel, Nick Piggin

On Fri, 30 Mar 2007, Andrew Morton wrote:
> On Thu, 29 Mar 2007 13:39:13 -0700
> Brian Pomerantz <bapper@piratehaven.org> wrote:
> 
> > When the dump cannot occur most likely because of a full file system
> > and the page to be written is the zero page, the call to
> > page_cache_release() is missed.
> > 
> > Signed-off-by: Brian Pomerantz <bapper@mvista.com>
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index a2fceba..9cc4f0a 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1704,7 +1704,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file)
> >  				DUMP_SEEK(PAGE_SIZE);
> >  			} else {
> >  				if (page == ZERO_PAGE(addr)) {
> > -					DUMP_SEEK(PAGE_SIZE);
> > +					if (!dump_seek(file, PAGE_SIZE)) {
> > +						page_cache_release(page);
> > +						goto end_coredump;
> > +					}
> 
> Oh for gawds sake I wish we could be rid of those idiotic macros :(
> 
> This patch looks OK to me, although a refcount leak on the ZERO_PAGE is
> special, because that page is PageReserved().
> 
> It used to be the case that we'd ignore attempts to change the refcount on
> reserved pages (or at least on the ZERO_PAGE), but we changed that, so we
> now actually refcount the ZERO_PAGE.  (I think, from a quick read of the
> code.  This contradicts my memory of how it works).
> 
> So I expect the net effect here is that a sufficiently determined attacker
> can overflow the ZERO_PAGE's refcount, thus causing it to be "freed".  The
> page allocator won't actually free the page due to PG_Reserved, but it'll
> all become very noisy.
> 
> Nick, Hugh: agree?

I think so - lots of "Bad page state" messages as the count bounces
around the 0 mark, but not actually freed.  But when CONFIG_DEBUG_VM
you'll get BUG_ONs.  And I can't swear bad things won't happen some-
where once the count wraps to negative.  Easier to fix than work out
the consequences.

(Of course, Nick is right now proposing a patch to take us back the
other way, back to not accounting the ZERO_PAGE: so the fix needs
to go in, then he'll need to reverse that again in his patch.)

Doesn't fs/binfmt_elf_fdpic.c need the same fix?  It looks slightly
different there, but I think when you look closer there's exactly
the same issue?

Hugh

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fix page leak during core dump
  2007-03-30 22:01   ` Hugh Dickins
@ 2007-03-30 22:13     ` Andrew Morton
  2007-03-30 23:06       ` Hugh Dickins
  2007-03-31 12:59       ` David Howells
  2007-03-31 12:57     ` David Howells
  1 sibling, 2 replies; 7+ messages in thread
From: Andrew Morton @ 2007-03-30 22:13 UTC (permalink / raw)
  To: Hugh Dickins, David Howells
  Cc: Brian Pomerantz, viro, linux-kernel, Nick Piggin

On Fri, 30 Mar 2007 23:01:45 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> On Fri, 30 Mar 2007, Andrew Morton wrote:
> > On Thu, 29 Mar 2007 13:39:13 -0700
> > Brian Pomerantz <bapper@piratehaven.org> wrote:
> > 
> > > When the dump cannot occur most likely because of a full file system
> > > and the page to be written is the zero page, the call to
> > > page_cache_release() is missed.
> > > 
> > > Signed-off-by: Brian Pomerantz <bapper@mvista.com>
> > > 
> > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > > index a2fceba..9cc4f0a 100644
> > > --- a/fs/binfmt_elf.c
> > > +++ b/fs/binfmt_elf.c
> > > @@ -1704,7 +1704,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file)
> > >  				DUMP_SEEK(PAGE_SIZE);
> > >  			} else {
> > >  				if (page == ZERO_PAGE(addr)) {
> > > -					DUMP_SEEK(PAGE_SIZE);
> > > +					if (!dump_seek(file, PAGE_SIZE)) {
> > > +						page_cache_release(page);
> > > +						goto end_coredump;
> > > +					}
> > 
> > Oh for gawds sake I wish we could be rid of those idiotic macros :(
> > 
> > This patch looks OK to me, although a refcount leak on the ZERO_PAGE is
> > special, because that page is PageReserved().
> > 
> > It used to be the case that we'd ignore attempts to change the refcount on
> > reserved pages (or at least on the ZERO_PAGE), but we changed that, so we
> > now actually refcount the ZERO_PAGE.  (I think, from a quick read of the
> > code.  This contradicts my memory of how it works).
> > 
> > So I expect the net effect here is that a sufficiently determined attacker
> > can overflow the ZERO_PAGE's refcount, thus causing it to be "freed".  The
> > page allocator won't actually free the page due to PG_Reserved, but it'll
> > all become very noisy.
> > 
> > Nick, Hugh: agree?
> 
> I think so - lots of "Bad page state" messages as the count bounces
> around the 0 mark, but not actually freed.  But when CONFIG_DEBUG_VM
> you'll get BUG_ONs.  And I can't swear bad things won't happen some-
> where once the count wraps to negative.  Easier to fix than work out
> the consequences.
> 
> (Of course, Nick is right now proposing a patch to take us back the
> other way, back to not accounting the ZERO_PAGE: so the fix needs
> to go in, then he'll need to reverse that again in his patch.)

OK.

> Doesn't fs/binfmt_elf_fdpic.c need the same fix?  It looks slightly
> different there, but I think when you look closer there's exactly
> the same issue?

Think so.  David, does it look OK?

<would anyone be interested in hearing my opinion on the DUMP_SEEK macro
again?>


From: Brian Pomerantz <bapper@piratehaven.org>

When the dump cannot occur most likely because of a full file system and
the page to be written is the zero page, the call to page_cache_release()
is missed.

Signed-off-by: Brian Pomerantz <bapper@mvista.com>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/binfmt_elf.c       |    5 ++++-
 fs/binfmt_elf_fdpic.c |    6 ++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff -puN fs/binfmt_elf.c~fix-page-leak-during-core-dump fs/binfmt_elf.c
--- a/fs/binfmt_elf.c~fix-page-leak-during-core-dump
+++ a/fs/binfmt_elf.c
@@ -1704,7 +1704,10 @@ static int elf_core_dump(long signr, str
 				DUMP_SEEK(PAGE_SIZE);
 			} else {
 				if (page == ZERO_PAGE(addr)) {
-					DUMP_SEEK(PAGE_SIZE);
+					if (!dump_seek(file, PAGE_SIZE)) {
+						page_cache_release(page);
+						goto end_coredump;
+					}
 				} else {
 					void *kaddr;
 					flush_cache_page(vma, addr,
diff -puN fs/binfmt_elf_fdpic.c~fix-page-leak-during-core-dump fs/binfmt_elf_fdpic.c
--- a/fs/binfmt_elf_fdpic.c~fix-page-leak-during-core-dump
+++ a/fs/binfmt_elf_fdpic.c
@@ -1480,8 +1480,10 @@ static int elf_fdpic_dump_segments(struc
 				DUMP_SEEK(file->f_pos + PAGE_SIZE);
 			}
 			else if (page == ZERO_PAGE(addr)) {
-				DUMP_SEEK(file->f_pos + PAGE_SIZE);
-				page_cache_release(page);
+				if (!dump_seek(file, file->f_pos + PAGE_SIZE)) {
+					page_cache_release(page);
+					return 0;
+				}
 			}
 			else {
 				void *kaddr;
_


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fix page leak during core dump
  2007-03-30 22:13     ` Andrew Morton
@ 2007-03-30 23:06       ` Hugh Dickins
  2007-03-31 12:59       ` David Howells
  1 sibling, 0 replies; 7+ messages in thread
From: Hugh Dickins @ 2007-03-30 23:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: David Howells, Brian Pomerantz, viro, linux-kernel, Nick Piggin

On Fri, 30 Mar 2007, Andrew Morton wrote:
> 
> <would anyone be interested in hearing my opinion on the DUMP_SEEK macro
> again?>

Oooh, yes please.

> diff -puN fs/binfmt_elf_fdpic.c~fix-page-leak-during-core-dump fs/binfmt_elf_fdpic.c
> --- a/fs/binfmt_elf_fdpic.c~fix-page-leak-during-core-dump
> +++ a/fs/binfmt_elf_fdpic.c
> @@ -1480,8 +1480,10 @@ static int elf_fdpic_dump_segments(struc
>  				DUMP_SEEK(file->f_pos + PAGE_SIZE);
>  			}
>  			else if (page == ZERO_PAGE(addr)) {
> -				DUMP_SEEK(file->f_pos + PAGE_SIZE);
> -				page_cache_release(page);
> +				if (!dump_seek(file, file->f_pos + PAGE_SIZE)) {
> +					page_cache_release(page);
> +					return 0;
> +				}
>  			}
>  			else {
>  				void *kaddr;
> _

No, I think that's wrong: whereas the binfmt_elf one did its
page_cache_release down below at the bottom of the block, this
version does it in each subblock, so there you're removing the
dump_seek success one.  Can't we preserve that beauteous macro
here and just do...

--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1480,8 +1480,8 @@ static int elf_fdpic_dump_segments(struc
 				DUMP_SEEK(file->f_pos + PAGE_SIZE);
 			}
 			else if (page == ZERO_PAGE(addr)) {
-				DUMP_SEEK(file->f_pos + PAGE_SIZE);
 				page_cache_release(page);
+				DUMP_SEEK(file->f_pos + PAGE_SIZE);
 			}
 			else {
 				void *kaddr;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fix page leak during core dump 
  2007-03-30 22:01   ` Hugh Dickins
  2007-03-30 22:13     ` Andrew Morton
@ 2007-03-31 12:57     ` David Howells
  1 sibling, 0 replies; 7+ messages in thread
From: David Howells @ 2007-03-31 12:57 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Hugh Dickins, Brian Pomerantz, viro, linux-kernel, Nick Piggin

Andrew Morton <akpm@linux-foundation.org> wrote:

> <would anyone be interested in hearing my opinion on the DUMP_SEEK macro
> again?>

I can guess.  And it's very probably right.  Macros containing return
statements like that are dodgy as they help people screw up the error handling.

However, ...

Andrew Morton <akpm@linux-foundation.org> wrote:

> -				DUMP_SEEK(file->f_pos + PAGE_SIZE);
> -				page_cache_release(page);
> +				if (!dump_seek(file, file->f_pos + PAGE_SIZE)) {
> +					page_cache_release(page);
> +					return 0;
> +				}

Is not correct as you've then eliminated the page_cache_release() on the
success path.  What you probably intended was:

	int tmp;
	...
				tmp = dump_seek(file, file->f_pos + PAGE_SIZE);
				page_cache_release(page);
				if (!tmp)
					return 0;

David

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fix page leak during core dump 
  2007-03-30 22:13     ` Andrew Morton
  2007-03-30 23:06       ` Hugh Dickins
@ 2007-03-31 12:59       ` David Howells
  1 sibling, 0 replies; 7+ messages in thread
From: David Howells @ 2007-03-31 12:59 UTC (permalink / raw)
  To: Hugh Dickins
  Cc: Andrew Morton, Brian Pomerantz, viro, linux-kernel, Nick Piggin

Hugh Dickins <hugh@veritas.com> wrote:

> No, I think that's wrong: whereas the binfmt_elf one did its
> page_cache_release down below at the bottom of the block, this
> version does it in each subblock,

All but the first, which is why there's not a common one post if-complex.

> so there you're removing the dump_seek success one.  Can't we preserve that
> beauteous macro here and just do...
> ...
> -				DUMP_SEEK(file->f_pos + PAGE_SIZE);
>  				page_cache_release(page);
> +				DUMP_SEEK(file->f_pos + PAGE_SIZE);

Actually, that works just as well, I think.  I like it better than my
suggestion (see email in response to Andrew).

David

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-03-31 12:59 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-03-29 20:39 [PATCH] fix page leak during core dump Brian Pomerantz
2007-03-30 20:43 ` Andrew Morton
2007-03-30 22:01   ` Hugh Dickins
2007-03-30 22:13     ` Andrew Morton
2007-03-30 23:06       ` Hugh Dickins
2007-03-31 12:59       ` David Howells
2007-03-31 12:57     ` David Howells

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).