* [PATCH] free pages in remove_arg_zero()
@ 2007-02-20 13:29 Petr Tesařík
2007-02-21 23:41 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Petr Tesařík @ 2007-02-20 13:29 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
Hello,
I've found a bug when executing scripts:
When a script is loaded, load_script() replaces argv[0] with the
name of the interpreter and the filename passed to the exec syscall.
However, there is no guarantee that the length of the interpreter
name plus the length of the filename is greater than the length of
the original argv[0]. If the difference happens to cross a page boundary,
setup_arg_pages() will call install_arg_page() with an address outside
the VMA.
Therefore, remove_arg_zero() must free all pages which would be unused
after the argument is removed.
Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
--- linux-sles9.orig/fs/exec.c
+++ linux-sles9/fs/exec.c
@@ -1000,6 +1000,8 @@ void remove_arg_zero(struct linux_binprm
continue;
offset = 0;
kunmap_atomic(kaddr, KM_USER0);
+ __free_page(page);
+ bprm->page[bprm->p/PAGE_SIZE - 1] = NULL;
inside:
page = bprm->page[bprm->p/PAGE_SIZE];
kaddr = kmap_atomic(page, KM_USER0);
[-- Attachment #2: Toto je digitálně podepsaná část zprávy --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] free pages in remove_arg_zero()
2007-02-20 13:29 [PATCH] free pages in remove_arg_zero() Petr Tesařík
@ 2007-02-21 23:41 ` Andrew Morton
2007-03-07 22:09 ` [PATCH] remove_arg_zero() rewrite Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2007-02-21 23:41 UTC (permalink / raw)
To: Petr Tesa__ík; +Cc: Linux Kernel Mailing List
On Tue, 20 Feb 2007 14:29:04 +0100
Petr Tesa____k <ptesarik@suse.cz> wrote:
> I've found a bug when executing scripts:
You've found more than that.
> When a script is loaded, load_script() replaces argv[0] with the
> name of the interpreter and the filename passed to the exec syscall.
> However, there is no guarantee that the length of the interpreter
> name plus the length of the filename is greater than the length of
> the original argv[0]. If the difference happens to cross a page boundary,
> setup_arg_pages() will call install_arg_page() with an address outside
> the VMA.
>
> Therefore, remove_arg_zero() must free all pages which would be unused
> after the argument is removed.
>
> Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
>
> --- linux-sles9.orig/fs/exec.c
> +++ linux-sles9/fs/exec.c
> @@ -1000,6 +1000,8 @@ void remove_arg_zero(struct linux_binprm
> continue;
> offset = 0;
> kunmap_atomic(kaddr, KM_USER0);
> + __free_page(page);
> + bprm->page[bprm->p/PAGE_SIZE - 1] = NULL;
> inside:
> page = bprm->page[bprm->p/PAGE_SIZE];
> kaddr = kmap_atomic(page, KM_USER0);
I am not surprised that remove_arg_zero() is buggy. Let us look at the
stupid thing:
void remove_arg_zero(struct linux_binprm *bprm)
{
if (bprm->argc) {
unsigned long offset;
char * kaddr;
struct page *page;
offset = bprm->p % PAGE_SIZE;
goto inside;
while (bprm->p++, *(kaddr+offset++)) {
if (offset != PAGE_SIZE)
continue;
offset = 0;
kunmap_atomic(kaddr, KM_USER0);
inside:
page = bprm->page[bprm->p/PAGE_SIZE];
kaddr = kmap_atomic(page, KM_USER0);
}
kunmap_atomic(kaddr, KM_USER0);
bprm->argc--;
}
}
I mean.... what the hell?
As you appear to have managed to work out what the sorry thing is trying to
do, would you have time to simply rip it out and completely rewrite it,
including a nice comment telling the world what this function's function is?
Because what we have there is beyond repairing.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] remove_arg_zero() rewrite
2007-02-21 23:41 ` Andrew Morton
@ 2007-03-07 22:09 ` Peter Zijlstra
2007-03-07 22:41 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2007-03-07 22:09 UTC (permalink / raw)
To: Andrew Morton; +Cc: Petr Tesa__ík, Linux Kernel Mailing List
On Wed, 2007-02-21 at 15:41 -0800, Andrew Morton wrote:
> I mean.... what the hell?
>
> As you appear to have managed to work out what the sorry thing is trying to
> do, would you have time to simply rip it out and completely rewrite it,
> including a nice comment telling the world what this function's function is?
>
> Because what we have there is beyond repairing.
Something like so?
Boots uml seemingly without errors.
---
Rewrite remove_arg_zero() to be more parseable by untwisted minds.
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
fs/exec.c | 40 +++++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 17 deletions(-)
Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c 2007-03-07 21:54:17.000000000 +0100
+++ linux-2.6/fs/exec.c 2007-03-07 23:08:27.000000000 +0100
@@ -987,28 +987,34 @@ void compute_creds(struct linux_binprm *
EXPORT_SYMBOL(compute_creds);
+/*
+ * Arguments are '\0' separated strings found at the location bprm->p
+ * points to; chop off the first by relocating brpm->p to right after
+ * the first '\0' encountered.
+ */
void remove_arg_zero(struct linux_binprm *bprm)
{
- if (bprm->argc) {
- unsigned long offset;
- char * kaddr;
- struct page *page;
+ unsigned long offset;
+ char *kaddr;
+ struct page *page;
- offset = bprm->p % PAGE_SIZE;
- goto inside;
+ if (!bprm->argc)
+ return;
+
+ do {
+ offset = bprm->p & ~PAGE_MASK;
+ page = bprm->page[bprm->p >> PAGE_SHIFT];
+ kaddr = kmap_atomic(page, KM_USER0);
+
+ for (; offset < PAGE_SIZE && kaddr[offset];
+ offset++, bprm->p++)
+ ;
- while (bprm->p++, *(kaddr+offset++)) {
- if (offset != PAGE_SIZE)
- continue;
- offset = 0;
- kunmap_atomic(kaddr, KM_USER0);
-inside:
- page = bprm->page[bprm->p/PAGE_SIZE];
- kaddr = kmap_atomic(page, KM_USER0);
- }
kunmap_atomic(kaddr, KM_USER0);
- bprm->argc--;
- }
+ } while (offset == PAGE_SIZE);
+
+ bprm->p++;
+ bprm->argc--;
}
EXPORT_SYMBOL(remove_arg_zero);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] remove_arg_zero() rewrite
2007-03-07 22:09 ` [PATCH] remove_arg_zero() rewrite Peter Zijlstra
@ 2007-03-07 22:41 ` Andrew Morton
2007-03-07 22:55 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2007-03-07 22:41 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Petr Tesa__ík, Linux Kernel Mailing List, Nick Piggin
On Wed, 07 Mar 2007 23:09:57 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Wed, 2007-02-21 at 15:41 -0800, Andrew Morton wrote:
>
> > I mean.... what the hell?
> >
> > As you appear to have managed to work out what the sorry thing is trying to
> > do, would you have time to simply rip it out and completely rewrite it,
> > including a nice comment telling the world what this function's function is?
> >
> > Because what we have there is beyond repairing.
>
> Something like so?
> Boots uml seemingly without errors.
>
> ---
> Rewrite remove_arg_zero() to be more parseable by untwisted minds.
>
Sorry, Nick already had a go at this:
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc2/2.6.21-rc2-mm2/broken-out/exec-fix-remove_arg_zero.patch
But your patch has a nice comment and has to be better ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remove_arg_zero() rewrite
2007-03-07 22:41 ` Andrew Morton
@ 2007-03-07 22:55 ` Peter Zijlstra
2007-03-07 23:05 ` Randy Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2007-03-07 22:55 UTC (permalink / raw)
To: Andrew Morton; +Cc: Petr Tesa__ík, Linux Kernel Mailing List, Nick Piggin
On Wed, 2007-03-07 at 14:41 -0800, Andrew Morton wrote:
> On Wed, 07 Mar 2007 23:09:57 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>
> > On Wed, 2007-02-21 at 15:41 -0800, Andrew Morton wrote:
> >
> > > I mean.... what the hell?
> > >
> > > As you appear to have managed to work out what the sorry thing is trying to
> > > do, would you have time to simply rip it out and completely rewrite it,
> > > including a nice comment telling the world what this function's function is?
> > >
> > > Because what we have there is beyond repairing.
> >
> > Something like so?
> > Boots uml seemingly without errors.
> >
> > ---
> > Rewrite remove_arg_zero() to be more parseable by untwisted minds.
> >
>
> Sorry, Nick already had a go at this:
Darn, must've fell off of lkml... or I missed it catching up my 4000+
email backlog.
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc2/2.6.21-rc2-mm2/broken-out/exec-fix-remove_arg_zero.patch
>
> But your patch has a nice comment and has to be better ;)
Ah, but now I finally see the initial problem.
My patch is bug compatible and doesn't free the head page when that
becomes unused.
This should do on top:
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c 2007-03-07 23:51:31.000000000 +0100
+++ linux-2.6/fs/exec.c 2007-03-07 23:50:41.000000000 +0100
@@ -1011,6 +1011,11 @@ void remove_arg_zero(struct linux_binprm
;
kunmap_atomic(kaddr, KM_USER0);
+
+ if (offset == PAGE_SIZE) {
+ __free_page(page);
+ bprm->page[(bprm->p >> PAGE_SHIFT) - 1] = NULL;
+ }
} while (offset == PAGE_SIZE);
bprm->p++;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] remove_arg_zero() rewrite
2007-03-07 22:55 ` Peter Zijlstra
@ 2007-03-07 23:05 ` Randy Dunlap
0 siblings, 0 replies; 6+ messages in thread
From: Randy Dunlap @ 2007-03-07 23:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andrew Morton, Petr Tesa__ík, Linux Kernel Mailing List,
Nick Piggin
On Wed, 07 Mar 2007 23:55:12 +0100 Peter Zijlstra wrote:
> On Wed, 2007-03-07 at 14:41 -0800, Andrew Morton wrote:
> > On Wed, 07 Mar 2007 23:09:57 +0100
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> >
> > > On Wed, 2007-02-21 at 15:41 -0800, Andrew Morton wrote:
> > >
> > > > I mean.... what the hell?
> > > >
> > > > As you appear to have managed to work out what the sorry thing is trying to
> > > > do, would you have time to simply rip it out and completely rewrite it,
> > > > including a nice comment telling the world what this function's function is?
> > > >
> > > > Because what we have there is beyond repairing.
> > >
> > > Something like so?
> > > Boots uml seemingly without errors.
> > >
> > > ---
> > > Rewrite remove_arg_zero() to be more parseable by untwisted minds.
> > >
> >
> > Sorry, Nick already had a go at this:
>
> Darn, must've fell off of lkml... or I missed it catching up my 4000+
> email backlog.
Hm, I've had a similar issue. Mail problems or what...
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-03-07 23:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-02-20 13:29 [PATCH] free pages in remove_arg_zero() Petr Tesařík
2007-02-21 23:41 ` Andrew Morton
2007-03-07 22:09 ` [PATCH] remove_arg_zero() rewrite Peter Zijlstra
2007-03-07 22:41 ` Andrew Morton
2007-03-07 22:55 ` Peter Zijlstra
2007-03-07 23:05 ` Randy Dunlap
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox