free pages in remove_arg_zero()
Subsystems (2)
PATCH 1/1 free pages in remove_arg_zero()
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.
fs/exec.c +2 -0
@@ -1000,6 +1000,8 @@ void remove_arg_zero(struct linux_binprmcontinue ; 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 );
Quoted 1 line · On Tue, 20 Feb 2007 14:29:04 +0100 Petr Tesa____k <[email protected]> wrote:
I've found a bug when executing scripts:
You've found more than that.
Quoted 16 lines
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 callinstall_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 <[email protected]>
fs/exec.c void remove_arg_zero(struct linux_binprm · line 1007
Quoted 8 lines
fs/exec.c +2
-0@@ -1000,6 +1000,8 @@ void remove_arg_zero(struct linux_binprmcontinue ; 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.
PATCH 1/1 remove_arg_zero() rewrite
Quoted 7 lines · Andrew Morton
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.
--- fs/exec.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)
fs/exec.c +23 -17
@@ -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);
Re: [PATCH] remove_arg_zero() rewrite
Quoted 8 lines · On Wed, 07 Mar 2007 23:09:57 +0100 Peter Zijlstra <[email protected]>…
Quoted 6 lines · Andrew Morton
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?
Quoted 8 lines
Quoted 1 line
Because what we have there is beyond repairing.
Something like so?
Boots uml seemingly without errors.---
Rewriteremove_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 ;)
Re: [PATCH] remove_arg_zero() rewrite
Quoted 13 lines · Andrew Morton
Quoted 10 lines · On Wed, 07 Mar 2007 23:09:57 +0100 Peter Zijlstra <[email protected]>…
Quoted 7 lines · Andrew Morton
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.
Quoted 8 lines
Quoted 6 lines
Something like so?
Boots uml seemingly without errors.---
Rewriteremove_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.
Quoted 3 lines
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:
---
fs/exec.c +5 -0
@@ -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++;
Re: [PATCH] remove_arg_zero() rewrite
Quoted 17 lines · On Wed, 07 Mar 2007 23:55:12 +0100 Peter Zijlstra wrote:
Quoted 16 lines · Andrew Morton
Quoted 13 lines · On Wed, 07 Mar 2007 23:09:57 +0100 Peter Zijlstra <[email protected]>…
Quoted 7 lines · Andrew Morton
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
---
Rewriteremove_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 ***