* [PATCH 0/5] exec: more cleanups
@ 2013-08-02 19:27 Oleg Nesterov
2013-08-02 19:27 ` [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm() Oleg Nesterov
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-02 19:27 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
On top of "[PATCH 0/3] exec: minor cleanups + minor fix" I sent
yesterday.
Perhaps too many patches for the poor search_binary_handler(),
but I do not know how to document the changes if I join them.
Oleg.
fs/exec.c | 82 ++++++++++++++++++++++++++----------------------------------
1 files changed, 36 insertions(+), 46 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm()
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
@ 2013-08-02 19:27 ` Oleg Nesterov
2013-08-03 19:27 ` Kees Cook
2013-08-02 19:27 ` [PATCH 2/5] exec: kill ->load_binary != NULL check in search_binary_handler() Oleg Nesterov
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-02 19:27 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
When search_binary_handler() succeeds it does allow_write_access()
and fput(), then it clears bprm->file to ensure the caller will not
do the same.
We can simply move this code to exec_binprm() which is called only
once. In fact we could move this to free_bprm() and remove the same
code in do_execve_common's error path.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index ad7d624..ef70320 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1400,10 +1400,6 @@ int search_binary_handler(struct linux_binprm *bprm)
bprm->recursion_depth--;
if (retval >= 0) {
put_binfmt(fmt);
- allow_write_access(bprm->file);
- if (bprm->file)
- fput(bprm->file);
- bprm->file = NULL;
return retval;
}
read_lock(&binfmt_lock);
@@ -1455,6 +1451,11 @@ static int exec_binprm(struct linux_binprm *bprm)
ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
current->did_exec = 1;
proc_exec_connector(current);
+
+ if (bprm->file) {
+ allow_write_access(bprm->file);
+ fput(bprm->file);
+ }
}
return ret;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/5] exec: kill ->load_binary != NULL check in search_binary_handler()
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
2013-08-02 19:27 ` [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm() Oleg Nesterov
@ 2013-08-02 19:27 ` Oleg Nesterov
2013-08-02 19:27 ` [PATCH 3/5] exec: cleanup the CONFIG_MODULES logic Oleg Nesterov
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-02 19:27 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
search_binary_handler() checks ->load_binary != NULL for no reason,
this method should be always defined. Turn this check into WARN_ON()
and move it into __register_binfmt().
Also, kill the function pointer. The current code looks confusing,
as if ->load_binary can go away after read_unlock(&binfmt_lock).
But we rely on module_get(fmt->module), this fmt can't be changed
or unregistered, otherwise this code is buggy anyway.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index ef70320..9f41e7d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -74,6 +74,8 @@ static DEFINE_RWLOCK(binfmt_lock);
void __register_binfmt(struct linux_binfmt * fmt, int insert)
{
BUG_ON(!fmt);
+ if (WARN_ON(!fmt->load_binary))
+ return;
write_lock(&binfmt_lock);
insert ? list_add(&fmt->lh, &formats) :
list_add_tail(&fmt->lh, &formats);
@@ -1389,14 +1391,11 @@ int search_binary_handler(struct linux_binprm *bprm)
for (try=0; try<2; try++) {
read_lock(&binfmt_lock);
list_for_each_entry(fmt, &formats, lh) {
- int (*fn)(struct linux_binprm *) = fmt->load_binary;
- if (!fn)
- continue;
if (!try_module_get(fmt->module))
continue;
read_unlock(&binfmt_lock);
bprm->recursion_depth++;
- retval = fn(bprm);
+ retval = fmt->load_binary(bprm);
bprm->recursion_depth--;
if (retval >= 0) {
put_binfmt(fmt);
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/5] exec: cleanup the CONFIG_MODULES logic
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
2013-08-02 19:27 ` [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm() Oleg Nesterov
2013-08-02 19:27 ` [PATCH 2/5] exec: kill ->load_binary != NULL check in search_binary_handler() Oleg Nesterov
@ 2013-08-02 19:27 ` Oleg Nesterov
2013-08-02 19:27 ` [PATCH 4/5] exec: don't retry if request_module() fails Oleg Nesterov
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-02 19:27 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
search_binary_handler() uses "for (try=0; try<2; try++)" to avoid
"goto" but the code looks too complicated and horrible imho. We
still need to check "try == 0" before request_module() and add the
additional "break" for !CONFIG_MODULES case.
Kill this loop and use a simple "bool need_retry" + "goto retry".
The code looks much simpler and we do not even need ifdef's, gcc
can optimize out the "if (need_retry)" block if !IS_ENABLED().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 68 +++++++++++++++++++++++++++---------------------------------
1 files changed, 31 insertions(+), 37 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 9f41e7d..48344a2 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1367,13 +1367,15 @@ out:
}
EXPORT_SYMBOL(remove_arg_zero);
+#define printable(c) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))
/*
* cycle the list of binary formats handler, until one recognizes the image
*/
int search_binary_handler(struct linux_binprm *bprm)
{
- int try, retval;
+ bool need_retry = IS_ENABLED(CONFIG_MODULES);
struct linux_binfmt *fmt;
+ int retval;
/* This allows 4 levels of binfmt rewrites before failing hard. */
if (bprm->recursion_depth > 5)
@@ -1388,47 +1390,39 @@ int search_binary_handler(struct linux_binprm *bprm)
return retval;
retval = -ENOENT;
- for (try=0; try<2; try++) {
- read_lock(&binfmt_lock);
- list_for_each_entry(fmt, &formats, lh) {
- if (!try_module_get(fmt->module))
- continue;
- read_unlock(&binfmt_lock);
- bprm->recursion_depth++;
- retval = fmt->load_binary(bprm);
- bprm->recursion_depth--;
- if (retval >= 0) {
- put_binfmt(fmt);
- return retval;
- }
- read_lock(&binfmt_lock);
+ retry:
+ read_lock(&binfmt_lock);
+ list_for_each_entry(fmt, &formats, lh) {
+ if (!try_module_get(fmt->module))
+ continue;
+ read_unlock(&binfmt_lock);
+ bprm->recursion_depth++;
+ retval = fmt->load_binary(bprm);
+ bprm->recursion_depth--;
+ if (retval >= 0) {
put_binfmt(fmt);
- if (retval != -ENOEXEC || bprm->mm == NULL)
- break;
- if (!bprm->file) {
- read_unlock(&binfmt_lock);
- return retval;
- }
+ return retval;
}
- read_unlock(&binfmt_lock);
-#ifdef CONFIG_MODULES
- if (retval != -ENOEXEC || bprm->mm == NULL) {
+ read_lock(&binfmt_lock);
+ put_binfmt(fmt);
+ if (retval != -ENOEXEC || bprm->mm == NULL)
break;
- } else {
-#define printable(c) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))
- if (printable(bprm->buf[0]) &&
- printable(bprm->buf[1]) &&
- printable(bprm->buf[2]) &&
- printable(bprm->buf[3]))
- break; /* -ENOEXEC */
- if (try)
- break; /* -ENOEXEC */
- request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
+ if (!bprm->file) {
+ read_unlock(&binfmt_lock);
+ return retval;
}
-#else
- break;
-#endif
}
+ read_unlock(&binfmt_lock);
+
+ if (need_retry && retval == -ENOEXEC && bprm->mm) {
+ if (printable(bprm->buf[0]) && printable(bprm->buf[1]) &&
+ printable(bprm->buf[2]) && printable(bprm->buf[3]))
+ return retval;
+ request_module("binfmt-%04x", *(ushort *)(bprm->buf + 2));
+ need_retry = false;
+ goto retry;
+ }
+
return retval;
}
EXPORT_SYMBOL(search_binary_handler);
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/5] exec: don't retry if request_module() fails
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
` (2 preceding siblings ...)
2013-08-02 19:27 ` [PATCH 3/5] exec: cleanup the CONFIG_MODULES logic Oleg Nesterov
@ 2013-08-02 19:27 ` Oleg Nesterov
2013-08-02 19:27 ` [PATCH 5/5] exec: cleanup the error handling in search_binary_handler() Oleg Nesterov
2013-08-03 19:28 ` [PATCH 0/5] exec: more cleanups Kees Cook
5 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-02 19:27 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
A separate one-liner for better documentation.
It doesn't make sense to retry if request_module() fails to exec
/sbin/modprobe, add the addition "request_module() < 0" check.
However, this logic still doesn't look exactly right:
1. It would be better to check "request_module() != 0", the user
space modprobe process should report the correct exit code.
But I didn't dare to add the user-visible change.
2. The whole ENOEXEC logic looks suboptimal. Suppose that we try
to exec a "#!path-to-unsupported-binary" script. In this case
request_module() + "retry" will be done twice: first by the
"depth == 1" code, and then again by the "depth == 0" caller
which doesn't make sense.
3. And note that in the case above bprm->buf was already changed
by load_script()->prepare_binprm(), so this looks even more
ugly.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 48344a2..d9fd32c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1418,7 +1418,8 @@ int search_binary_handler(struct linux_binprm *bprm)
if (printable(bprm->buf[0]) && printable(bprm->buf[1]) &&
printable(bprm->buf[2]) && printable(bprm->buf[3]))
return retval;
- request_module("binfmt-%04x", *(ushort *)(bprm->buf + 2));
+ if (request_module("binfmt-%04x", *(ushort *)(bprm->buf + 2)) < 0)
+ return retval;
need_retry = false;
goto retry;
}
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/5] exec: cleanup the error handling in search_binary_handler()
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
` (3 preceding siblings ...)
2013-08-02 19:27 ` [PATCH 4/5] exec: don't retry if request_module() fails Oleg Nesterov
@ 2013-08-02 19:27 ` Oleg Nesterov
2013-08-03 19:28 ` [PATCH 0/5] exec: more cleanups Kees Cook
5 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-02 19:27 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
The error hanling and ret-from-loop look confusing and inconsistent.
- "retval >= 0" simply returns
- "!bprm->file" returns too but with read_unlock() because
binfmt_lock was already re-acquired
- "retval != -ENOEXEC || bprm->mm == NULL" does "break" and
relies on the same check after the main loop
Consolidate these checks into a single if/return statement.
need_retry still checks "retval == -ENOEXEC", but this and -ENOENT
before the main loop are not needed. This is only for pathological
and impossible list_empty(&formats) case.
It is not clear why do we check "bprm->mm == NULL", probably this
should be removed.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 11 +++--------
1 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index d9fd32c..7ab2120 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1399,22 +1399,17 @@ int search_binary_handler(struct linux_binprm *bprm)
bprm->recursion_depth++;
retval = fmt->load_binary(bprm);
bprm->recursion_depth--;
- if (retval >= 0) {
+ if (retval >= 0 || retval != -ENOEXEC ||
+ bprm->mm == NULL || bprm->file == NULL) {
put_binfmt(fmt);
return retval;
}
read_lock(&binfmt_lock);
put_binfmt(fmt);
- if (retval != -ENOEXEC || bprm->mm == NULL)
- break;
- if (!bprm->file) {
- read_unlock(&binfmt_lock);
- return retval;
- }
}
read_unlock(&binfmt_lock);
- if (need_retry && retval == -ENOEXEC && bprm->mm) {
+ if (need_retry && retval == -ENOEXEC) {
if (printable(bprm->buf[0]) && printable(bprm->buf[1]) &&
printable(bprm->buf[2]) && printable(bprm->buf[3]))
return retval;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm()
2013-08-02 19:27 ` [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm() Oleg Nesterov
@ 2013-08-03 19:27 ` Kees Cook
2013-08-04 14:48 ` Oleg Nesterov
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2013-08-03 19:27 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Fri, Aug 2, 2013 at 12:27 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> When search_binary_handler() succeeds it does allow_write_access()
> and fput(), then it clears bprm->file to ensure the caller will not
> do the same.
>
> We can simply move this code to exec_binprm() which is called only
> once. In fact we could move this to free_bprm() and remove the same
> code in do_execve_common's error path.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> fs/exec.c | 9 +++++----
> 1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index ad7d624..ef70320 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1400,10 +1400,6 @@ int search_binary_handler(struct linux_binprm *bprm)
> bprm->recursion_depth--;
> if (retval >= 0) {
> put_binfmt(fmt);
> - allow_write_access(bprm->file);
> - if (bprm->file)
> - fput(bprm->file);
> - bprm->file = NULL;
> return retval;
> }
> read_lock(&binfmt_lock);
> @@ -1455,6 +1451,11 @@ static int exec_binprm(struct linux_binprm *bprm)
> ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
> current->did_exec = 1;
> proc_exec_connector(current);
> +
> + if (bprm->file) {
> + allow_write_access(bprm->file);
> + fput(bprm->file);
> + }
Why not keep the bprm->file = NULL assignment? Seems reasonable to
keep that just to be avoid use-after-free accidents.
-Kees
> }
>
> return ret;
> --
> 1.5.5.1
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5] exec: more cleanups
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
` (4 preceding siblings ...)
2013-08-02 19:27 ` [PATCH 5/5] exec: cleanup the error handling in search_binary_handler() Oleg Nesterov
@ 2013-08-03 19:28 ` Kees Cook
5 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2013-08-03 19:28 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Fri, Aug 2, 2013 at 12:27 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On top of "[PATCH 0/3] exec: minor cleanups + minor fix" I sent
> yesterday.
>
> Perhaps too many patches for the poor search_binary_handler(),
> but I do not know how to document the changes if I join them.
>
> Oleg.
>
> fs/exec.c | 82 ++++++++++++++++++++++++++----------------------------------
> 1 files changed, 36 insertions(+), 46 deletions(-)
>
This all looks really good. Thanks for the cleanups! Besides the one
comment on patch 1, consider the series:
Acked-by: Kees Cook <keescook@chromium.org>
Thanks,
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm()
2013-08-03 19:27 ` Kees Cook
@ 2013-08-04 14:48 ` Oleg Nesterov
0 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2013-08-04 14:48 UTC (permalink / raw)
To: Kees Cook; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On 08/03, Kees Cook wrote:
>
> On Fri, Aug 2, 2013 at 12:27 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > @@ -1455,6 +1451,11 @@ static int exec_binprm(struct linux_binprm *bprm)
> > ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
> > current->did_exec = 1;
> > proc_exec_connector(current);
> > +
> > + if (bprm->file) {
> > + allow_write_access(bprm->file);
> > + fput(bprm->file);
> > + }
>
> Why not keep the bprm->file = NULL assignment?
Because it is no longer needed.
And now that we have the non-recursive exec_binprm() called right
before free_bprm() it is obvious that it won't be used again.
> Seems reasonable to
> keep that just to be avoid use-after-free accidents.
OK. I will add it back. With the comment to explain that this is
only to catch the possible problems.
I guess it would be better if I resend the whole series to avoid
the confusion. I am going to add your acks. It seems that you acked
everything except 1/3 in the previous series, perhaps you can ack
it too?
Thanks for review!
Oleg.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-04 14:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-02 19:27 [PATCH 0/5] exec: more cleanups Oleg Nesterov
2013-08-02 19:27 ` [PATCH 1/5] exec: move allow_write_access/fput to exec_binprm() Oleg Nesterov
2013-08-03 19:27 ` Kees Cook
2013-08-04 14:48 ` Oleg Nesterov
2013-08-02 19:27 ` [PATCH 2/5] exec: kill ->load_binary != NULL check in search_binary_handler() Oleg Nesterov
2013-08-02 19:27 ` [PATCH 3/5] exec: cleanup the CONFIG_MODULES logic Oleg Nesterov
2013-08-02 19:27 ` [PATCH 4/5] exec: don't retry if request_module() fails Oleg Nesterov
2013-08-02 19:27 ` [PATCH 5/5] exec: cleanup the error handling in search_binary_handler() Oleg Nesterov
2013-08-03 19:28 ` [PATCH 0/5] exec: more cleanups Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox