* [PATCH 0/3] exec: minor cleanups + minor fix
@ 2013-08-01 19:04 Oleg Nesterov
2013-08-01 19:05 ` [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code Oleg Nesterov
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-01 19:04 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
Andrew,
By discussion with Zach, please drop
fs-binfmts-add-a-name-field-to-the-binfmt-struct.patch
fs-binfmts-better-handling-of-binfmt-loops.patch
fs-binfmts-whitespace-fixes-with-scripts-cleanfile.patch
Zach is working on v2.
Meanwhile, can't we cleanup search_binary_handler() a bit? It
doesn't look nice imho. And we can certainly cleanup it more
(perhaps I'll try tomorrow). The error handling, request_module()
logic and even "for (try=0; try<2; try++)" looks horrible imho.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code
2013-08-01 19:04 [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
@ 2013-08-01 19:05 ` Oleg Nesterov
2013-08-03 19:05 ` Kees Cook
2013-08-01 19:05 ` [PATCH 2/3] exec: kill "int depth" in search_binary_handler() Oleg Nesterov
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-01 19:05 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
task_pid_nr_ns() and trace/ptrace code in the middle of the
recursive search_binary_handler() looks confusing and imho
annoying. We only need this code if "depth == 0", lets add
a simple helper which calls search_binary_handler() and does
trace_sched_process_exec() + ptrace_event().
The patch also moves the setting of task->did_exec, we need
to do this only once.
Note: we can kill either task->did_exec or PF_FORKNOEXEC.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 36 ++++++++++++++++++++++--------------
1 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 9c73def..a9ae4f2 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1373,7 +1373,6 @@ int search_binary_handler(struct linux_binprm *bprm)
unsigned int depth = bprm->recursion_depth;
int try,retval;
struct linux_binfmt *fmt;
- pid_t old_pid, old_vpid;
/* This allows 4 levels of binfmt rewrites before failing hard. */
if (depth > 5)
@@ -1387,12 +1386,6 @@ int search_binary_handler(struct linux_binprm *bprm)
if (retval)
return retval;
- /* Need to fetch pid before load_binary changes it */
- old_pid = current->pid;
- rcu_read_lock();
- old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
- rcu_read_unlock();
-
retval = -ENOENT;
for (try=0; try<2; try++) {
read_lock(&binfmt_lock);
@@ -1407,16 +1400,11 @@ int search_binary_handler(struct linux_binprm *bprm)
retval = fn(bprm);
bprm->recursion_depth = depth;
if (retval >= 0) {
- if (depth == 0) {
- trace_sched_process_exec(current, old_pid, bprm);
- ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
- }
put_binfmt(fmt);
allow_write_access(bprm->file);
if (bprm->file)
fput(bprm->file);
bprm->file = NULL;
- current->did_exec = 1;
proc_exec_connector(current);
return retval;
}
@@ -1450,9 +1438,29 @@ int search_binary_handler(struct linux_binprm *bprm)
}
return retval;
}
-
EXPORT_SYMBOL(search_binary_handler);
+static int exec_binprm(struct linux_binprm *bprm)
+{
+ pid_t old_pid, old_vpid;
+ int ret;
+
+ /* Need to fetch pid before load_binary changes it */
+ old_pid = current->pid;
+ rcu_read_lock();
+ old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
+ rcu_read_unlock();
+
+ ret = search_binary_handler(bprm);
+ if (ret >= 0) {
+ trace_sched_process_exec(current, old_pid, bprm);
+ ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
+ current->did_exec = 1;
+ }
+
+ return ret;
+}
+
/*
* sys_execve() executes a new program.
*/
@@ -1541,7 +1549,7 @@ static int do_execve_common(const char *filename,
if (retval < 0)
goto out;
- retval = search_binary_handler(bprm);
+ retval = exec_binprm(bprm);
if (retval < 0)
goto out;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] exec: kill "int depth" in search_binary_handler()
2013-08-01 19:04 [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
2013-08-01 19:05 ` [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code Oleg Nesterov
@ 2013-08-01 19:05 ` Oleg Nesterov
2013-08-03 18:28 ` Kees Cook
2013-08-01 19:05 ` [PATCH 3/3] exec: proc_exec_connector() should be called only once Oleg Nesterov
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-01 19:05 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
Nobody except search_binary_handler() should touch ->recursion_depth,
"int depth" buys nothing but complicates the code, kill it.
Probably we should also kill "fn" and the !NULL check, ->load_binary
should be always defined. And it can not go away after read_unlock()
or this code is buggy anyway.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index a9ae4f2..f32079c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1370,12 +1370,11 @@ EXPORT_SYMBOL(remove_arg_zero);
*/
int search_binary_handler(struct linux_binprm *bprm)
{
- unsigned int depth = bprm->recursion_depth;
- int try,retval;
+ int try, retval;
struct linux_binfmt *fmt;
/* This allows 4 levels of binfmt rewrites before failing hard. */
- if (depth > 5)
+ if (bprm->recursion_depth > 5)
return -ELOOP;
retval = security_bprm_check(bprm);
@@ -1396,9 +1395,9 @@ int search_binary_handler(struct linux_binprm *bprm)
if (!try_module_get(fmt->module))
continue;
read_unlock(&binfmt_lock);
- bprm->recursion_depth = depth + 1;
+ bprm->recursion_depth++;
retval = fn(bprm);
- bprm->recursion_depth = depth;
+ bprm->recursion_depth--;
if (retval >= 0) {
put_binfmt(fmt);
allow_write_access(bprm->file);
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] exec: proc_exec_connector() should be called only once
2013-08-01 19:04 [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
2013-08-01 19:05 ` [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code Oleg Nesterov
2013-08-01 19:05 ` [PATCH 2/3] exec: kill "int depth" in search_binary_handler() Oleg Nesterov
@ 2013-08-01 19:05 ` Oleg Nesterov
2013-08-03 19:10 ` Kees Cook
2013-08-02 14:09 ` [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
2013-08-04 16:30 ` Oleg Nesterov
4 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-01 19:05 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
A separate one-liner with the minor fix.
PROC_EVENT_EXEC reports the "exec" event, but this message
is sent at least twice if search_binary_handler() is called
by ->load_binary() recursively, say, load_script().
Move it to exec_binprm(), this is "depth == 0" code too.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index f32079c..ad7d624 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1404,7 +1404,6 @@ int search_binary_handler(struct linux_binprm *bprm)
if (bprm->file)
fput(bprm->file);
bprm->file = NULL;
- proc_exec_connector(current);
return retval;
}
read_lock(&binfmt_lock);
@@ -1455,6 +1454,7 @@ static int exec_binprm(struct linux_binprm *bprm)
trace_sched_process_exec(current, old_pid, bprm);
ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
current->did_exec = 1;
+ proc_exec_connector(current);
}
return ret;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] exec: minor cleanups + minor fix
2013-08-01 19:04 [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
` (2 preceding siblings ...)
2013-08-01 19:05 ` [PATCH 3/3] exec: proc_exec_connector() should be called only once Oleg Nesterov
@ 2013-08-02 14:09 ` Oleg Nesterov
2013-08-02 14:38 ` Zach Levis
2013-08-04 16:30 ` Oleg Nesterov
4 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-02 14:09 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
On 08/01, Oleg Nesterov wrote:
>
> Andrew,
>
> By discussion with Zach, please drop
>
> fs-binfmts-add-a-name-field-to-the-binfmt-struct.patch
> fs-binfmts-better-handling-of-binfmt-loops.patch
> fs-binfmts-whitespace-fixes-with-scripts-cleanfile.patch
>
> Zach is working on v2.
>
> Meanwhile, can't we cleanup search_binary_handler() a bit? It
> doesn't look nice imho. And we can certainly cleanup it more
> (perhaps I'll try tomorrow). The error handling, request_module()
> logic and even "for (try=0; try<2; try++)" looks horrible imho.
Seriously, I think this should be cleanuped too.
Zach, I am sorry if you already rebased your changes, I'll send
a couple more patches today.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] exec: minor cleanups + minor fix
2013-08-02 14:09 ` [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
@ 2013-08-02 14:38 ` Zach Levis
0 siblings, 0 replies; 14+ messages in thread
From: Zach Levis @ 2013-08-02 14:38 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
On 08/02/2013 07:09 AM, Oleg Nesterov wrote:
> On 08/01, Oleg Nesterov wrote:
>>
>> Andrew,
>>
>> By discussion with Zach, please drop
>>
>> fs-binfmts-add-a-name-field-to-the-binfmt-struct.patch
>> fs-binfmts-better-handling-of-binfmt-loops.patch
>> fs-binfmts-whitespace-fixes-with-scripts-cleanfile.patch
>>
>> Zach is working on v2.
>>
>> Meanwhile, can't we cleanup search_binary_handler() a bit? It
>> doesn't look nice imho. And we can certainly cleanup it more
>> (perhaps I'll try tomorrow). The error handling, request_module()
>> logic and even "for (try=0; try<2; try++)" looks horrible imho.
>
> Seriously, I think this should be cleanuped too.
>
> Zach, I am sorry if you already rebased your changes, I'll send
> a couple more patches today.
>
Don't worry -- I've been holding off on sending v2 for your changes
today. It's not much git wrangling to apply your additional patches anyway.
> Oleg.
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] exec: kill "int depth" in search_binary_handler()
2013-08-01 19:05 ` [PATCH 2/3] exec: kill "int depth" in search_binary_handler() Oleg Nesterov
@ 2013-08-03 18:28 ` Kees Cook
2013-08-03 18:55 ` [PATCH v2 " Oleg Nesterov
0 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2013-08-03 18:28 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> Nobody except search_binary_handler() should touch ->recursion_depth,
> "int depth" buys nothing but complicates the code, kill it.
I'd like to see a comment added to binfmts.h's recursion_depth field
that reminds people that recursion_depth is for
search_binary_handler()'s use only, and a binfmt loader shouldn't
touch it. Besides that, yeah, sensible clean up.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] exec: kill "int depth" in search_binary_handler()
2013-08-03 18:28 ` Kees Cook
@ 2013-08-03 18:55 ` Oleg Nesterov
2013-08-03 19:31 ` Kees Cook
0 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-03 18:55 UTC (permalink / raw)
To: Kees Cook; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On 08/03, Kees Cook wrote:
>
> On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Nobody except search_binary_handler() should touch ->recursion_depth,
> > "int depth" buys nothing but complicates the code, kill it.
>
> I'd like to see a comment added to binfmts.h's recursion_depth field
> that reminds people that recursion_depth is for
> search_binary_handler()'s use only, and a binfmt loader shouldn't
> touch it.
And this comment probably makes sense even without this change
> Besides that, yeah, sensible clean up.
OK, thanks, please see v2. The only change is the comment in .h
------------------------------------------------------------------------------
Subject: [PATCH 1/1] exec: kill "int depth" in search_binary_handler()
Nobody except search_binary_handler() should touch ->recursion_depth,
"int depth" buys nothing but complicates the code, kill it.
Probably we should also kill "fn" and the !NULL check, ->load_binary
should be always defined. And it can not go away after read_unlock()
or this code is buggy anyway.
v2: add the comment about linux_binprm->recursion_depth
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/exec.c | 9 ++++-----
include/linux/binfmts.h | 2 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index a9ae4f2..f32079c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1370,12 +1370,11 @@ EXPORT_SYMBOL(remove_arg_zero);
*/
int search_binary_handler(struct linux_binprm *bprm)
{
- unsigned int depth = bprm->recursion_depth;
- int try,retval;
+ int try, retval;
struct linux_binfmt *fmt;
/* This allows 4 levels of binfmt rewrites before failing hard. */
- if (depth > 5)
+ if (bprm->recursion_depth > 5)
return -ELOOP;
retval = security_bprm_check(bprm);
@@ -1396,9 +1395,9 @@ int search_binary_handler(struct linux_binprm *bprm)
if (!try_module_get(fmt->module))
continue;
read_unlock(&binfmt_lock);
- bprm->recursion_depth = depth + 1;
+ bprm->recursion_depth++;
retval = fn(bprm);
- bprm->recursion_depth = depth;
+ bprm->recursion_depth--;
if (retval >= 0) {
put_binfmt(fmt);
allow_write_access(bprm->file);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 70cf138..e8112ae 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -31,7 +31,7 @@ struct linux_binprm {
#ifdef __alpha__
unsigned int taso:1;
#endif
- unsigned int recursion_depth;
+ unsigned int recursion_depth; /* only for search_binary_handler() */
struct file * file;
struct cred *cred; /* new credentials */
int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code
2013-08-01 19:05 ` [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code Oleg Nesterov
@ 2013-08-03 19:05 ` Kees Cook
2013-08-04 14:35 ` Oleg Nesterov
0 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2013-08-03 19:05 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> task_pid_nr_ns() and trace/ptrace code in the middle of the
> recursive search_binary_handler() looks confusing and imho
> annoying. We only need this code if "depth == 0", lets add
> a simple helper which calls search_binary_handler() and does
> trace_sched_process_exec() + ptrace_event().
>
> The patch also moves the setting of task->did_exec, we need
> to do this only once.
>
> Note: we can kill either task->did_exec or PF_FORKNOEXEC.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> fs/exec.c | 36 ++++++++++++++++++++++--------------
> 1 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 9c73def..a9ae4f2 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1373,7 +1373,6 @@ int search_binary_handler(struct linux_binprm *bprm)
> unsigned int depth = bprm->recursion_depth;
> int try,retval;
> struct linux_binfmt *fmt;
> - pid_t old_pid, old_vpid;
>
> /* This allows 4 levels of binfmt rewrites before failing hard. */
> if (depth > 5)
> @@ -1387,12 +1386,6 @@ int search_binary_handler(struct linux_binprm *bprm)
> if (retval)
> return retval;
>
> - /* Need to fetch pid before load_binary changes it */
> - old_pid = current->pid;
> - rcu_read_lock();
> - old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
> - rcu_read_unlock();
> -
> retval = -ENOENT;
> for (try=0; try<2; try++) {
> read_lock(&binfmt_lock);
> @@ -1407,16 +1400,11 @@ int search_binary_handler(struct linux_binprm *bprm)
> retval = fn(bprm);
> bprm->recursion_depth = depth;
> if (retval >= 0) {
> - if (depth == 0) {
> - trace_sched_process_exec(current, old_pid, bprm);
> - ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
> - }
> put_binfmt(fmt);
> allow_write_access(bprm->file);
> if (bprm->file)
> fput(bprm->file);
> bprm->file = NULL;
> - current->did_exec = 1;
> proc_exec_connector(current);
> return retval;
> }
> @@ -1450,9 +1438,29 @@ int search_binary_handler(struct linux_binprm *bprm)
> }
> return retval;
> }
> -
> EXPORT_SYMBOL(search_binary_handler);
>
> +static int exec_binprm(struct linux_binprm *bprm)
> +{
> + pid_t old_pid, old_vpid;
> + int ret;
> +
> + /* Need to fetch pid before load_binary changes it */
> + old_pid = current->pid;
> + rcu_read_lock();
> + old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
> + rcu_read_unlock();
> +
> + ret = search_binary_handler(bprm);
> + if (ret >= 0) {
> + trace_sched_process_exec(current, old_pid, bprm);
> + ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
> + current->did_exec = 1;
> + }
Cleanup looks good. One idea here, though: this could be made more
pretty by doing:
if (ret < 0)
return ret;
to avoid the indentation for the "expected" code path.
-Kees
> +
> + return ret;
> +}
> +
> /*
> * sys_execve() executes a new program.
> */
> @@ -1541,7 +1549,7 @@ static int do_execve_common(const char *filename,
> if (retval < 0)
> goto out;
>
> - retval = search_binary_handler(bprm);
> + retval = exec_binprm(bprm);
> if (retval < 0)
> goto out;
>
> --
> 1.5.5.1
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] exec: proc_exec_connector() should be called only once
2013-08-01 19:05 ` [PATCH 3/3] exec: proc_exec_connector() should be called only once Oleg Nesterov
@ 2013-08-03 19:10 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2013-08-03 19:10 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> A separate one-liner with the minor fix.
>
> PROC_EVENT_EXEC reports the "exec" event, but this message
> is sent at least twice if search_binary_handler() is called
> by ->load_binary() recursively, say, load_script().
>
> Move it to exec_binprm(), this is "depth == 0" code too.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Yeah, looks right. I almost mentioned this while reading the other
cleanup patch, but then you fixed it too. :)
Acked-by: Kees Cook <keescook@chromium.org>
Thanks,
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] exec: kill "int depth" in search_binary_handler()
2013-08-03 18:55 ` [PATCH v2 " Oleg Nesterov
@ 2013-08-03 19:31 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2013-08-03 19:31 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Sat, Aug 3, 2013 at 11:55 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/03, Kees Cook wrote:
>>
>> On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > Nobody except search_binary_handler() should touch ->recursion_depth,
>> > "int depth" buys nothing but complicates the code, kill it.
>>
>> I'd like to see a comment added to binfmts.h's recursion_depth field
>> that reminds people that recursion_depth is for
>> search_binary_handler()'s use only, and a binfmt loader shouldn't
>> touch it.
>
> And this comment probably makes sense even without this change
Yeah totally agreed -- I should have added this when I reorganized the
depth handling earlier. :)
>> Besides that, yeah, sensible clean up.
>
> OK, thanks, please see v2. The only change is the comment in .h
>
> ------------------------------------------------------------------------------
> Subject: [PATCH 1/1] exec: kill "int depth" in search_binary_handler()
>
> Nobody except search_binary_handler() should touch ->recursion_depth,
> "int depth" buys nothing but complicates the code, kill it.
>
> Probably we should also kill "fn" and the !NULL check, ->load_binary
> should be always defined. And it can not go away after read_unlock()
> or this code is buggy anyway.
>
> v2: add the comment about linux_binprm->recursion_depth
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Kees Cook <keescook@chromium.org>
Thanks,
-Kees
> ---
> fs/exec.c | 9 ++++-----
> include/linux/binfmts.h | 2 +-
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index a9ae4f2..f32079c 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1370,12 +1370,11 @@ EXPORT_SYMBOL(remove_arg_zero);
> */
> int search_binary_handler(struct linux_binprm *bprm)
> {
> - unsigned int depth = bprm->recursion_depth;
> - int try,retval;
> + int try, retval;
> struct linux_binfmt *fmt;
>
> /* This allows 4 levels of binfmt rewrites before failing hard. */
> - if (depth > 5)
> + if (bprm->recursion_depth > 5)
> return -ELOOP;
>
> retval = security_bprm_check(bprm);
> @@ -1396,9 +1395,9 @@ int search_binary_handler(struct linux_binprm *bprm)
> if (!try_module_get(fmt->module))
> continue;
> read_unlock(&binfmt_lock);
> - bprm->recursion_depth = depth + 1;
> + bprm->recursion_depth++;
> retval = fn(bprm);
> - bprm->recursion_depth = depth;
> + bprm->recursion_depth--;
> if (retval >= 0) {
> put_binfmt(fmt);
> allow_write_access(bprm->file);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 70cf138..e8112ae 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -31,7 +31,7 @@ struct linux_binprm {
> #ifdef __alpha__
> unsigned int taso:1;
> #endif
> - unsigned int recursion_depth;
> + unsigned int recursion_depth; /* only for search_binary_handler() */
> struct file * file;
> struct cred *cred; /* new credentials */
> int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
> --
> 1.5.5.1
>
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code
2013-08-03 19:05 ` Kees Cook
@ 2013-08-04 14:35 ` Oleg Nesterov
2013-08-04 17:13 ` Kees Cook
0 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-04 14:35 UTC (permalink / raw)
To: Kees Cook; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On 08/03, Kees Cook wrote:
>
> On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > +static int exec_binprm(struct linux_binprm *bprm)
> > +{
> > + pid_t old_pid, old_vpid;
> > + int ret;
> > +
> > + /* Need to fetch pid before load_binary changes it */
> > + old_pid = current->pid;
> > + rcu_read_lock();
> > + old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
> > + rcu_read_unlock();
> > +
> > + ret = search_binary_handler(bprm);
> > + if (ret >= 0) {
> > + trace_sched_process_exec(current, old_pid, bprm);
> > + ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
> > + current->did_exec = 1;
> > + }
>
> Cleanup looks good. One idea here, though: this could be made more
> pretty by doing:
>
> if (ret < 0)
> return ret;
>
> to avoid the indentation for the "expected" code path.
Well, I do not reallt mind. But this "if" block is simple and small,
we do we need another "return" ?
To me the code looks more readable this way, but I can redo/resend.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] exec: minor cleanups + minor fix
2013-08-01 19:04 [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
` (3 preceding siblings ...)
2013-08-02 14:09 ` [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
@ 2013-08-04 16:30 ` Oleg Nesterov
4 siblings, 0 replies; 14+ messages in thread
From: Oleg Nesterov @ 2013-08-04 16:30 UTC (permalink / raw)
To: Andrew Morton, Zach Levis
Cc: Al Viro, Evgeniy Polyakov, Kees Cook, linux-kernel
Andrew,
please ignore this and the next series, I'll resend everything
with a couple of cosmetic changes.
Plus I believe audit_bprm() is not right too, this needs another
patch.
On 08/01, Oleg Nesterov wrote:
>
> Andrew,
>
> By discussion with Zach, please drop
>
> fs-binfmts-add-a-name-field-to-the-binfmt-struct.patch
> fs-binfmts-better-handling-of-binfmt-loops.patch
> fs-binfmts-whitespace-fixes-with-scripts-cleanfile.patch
>
> Zach is working on v2.
>
> Meanwhile, can't we cleanup search_binary_handler() a bit? It
> doesn't look nice imho. And we can certainly cleanup it more
> (perhaps I'll try tomorrow). The error handling, request_module()
> logic and even "for (try=0; try<2; try++)" looks horrible imho.
>
> Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code
2013-08-04 14:35 ` Oleg Nesterov
@ 2013-08-04 17:13 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2013-08-04 17:13 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Zach Levis, Al Viro, Evgeniy Polyakov, LKML
On Sun, Aug 4, 2013 at 7:35 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/03, Kees Cook wrote:
>>
>> On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > +static int exec_binprm(struct linux_binprm *bprm)
>> > +{
>> > + pid_t old_pid, old_vpid;
>> > + int ret;
>> > +
>> > + /* Need to fetch pid before load_binary changes it */
>> > + old_pid = current->pid;
>> > + rcu_read_lock();
>> > + old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
>> > + rcu_read_unlock();
>> > +
>> > + ret = search_binary_handler(bprm);
>> > + if (ret >= 0) {
>> > + trace_sched_process_exec(current, old_pid, bprm);
>> > + ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
>> > + current->did_exec = 1;
>> > + }
>>
>> Cleanup looks good. One idea here, though: this could be made more
>> pretty by doing:
>>
>> if (ret < 0)
>> return ret;
>>
>> to avoid the indentation for the "expected" code path.
>
> Well, I do not reallt mind. But this "if" block is simple and small,
> we do we need another "return" ?
>
> To me the code looks more readable this way, but I can redo/resend.
Cool, that's fine how it is. It was just a style suggestion. :)
Acked-by: Kees Cook <keescook@chromium.org>
Thanks!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-08-04 17:13 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-01 19:04 [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
2013-08-01 19:05 ` [PATCH 1/3] exec: introduce exec_binprm() for "depth == 0" code Oleg Nesterov
2013-08-03 19:05 ` Kees Cook
2013-08-04 14:35 ` Oleg Nesterov
2013-08-04 17:13 ` Kees Cook
2013-08-01 19:05 ` [PATCH 2/3] exec: kill "int depth" in search_binary_handler() Oleg Nesterov
2013-08-03 18:28 ` Kees Cook
2013-08-03 18:55 ` [PATCH v2 " Oleg Nesterov
2013-08-03 19:31 ` Kees Cook
2013-08-01 19:05 ` [PATCH 3/3] exec: proc_exec_connector() should be called only once Oleg Nesterov
2013-08-03 19:10 ` Kees Cook
2013-08-02 14:09 ` [PATCH 0/3] exec: minor cleanups + minor fix Oleg Nesterov
2013-08-02 14:38 ` Zach Levis
2013-08-04 16:30 ` Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox