Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
On 11/22, KOSAKI Motohiro wrote: > > >> I have found no problem in this patch. However, I have a very basic > >> question. > >> Why do we need to keep fs->in_exec? > > > > To ensure that a sub-thread can't create a new process with the same > > ->fs while we are doing exec without LSM_UNSAFE_SHARE, I guess. This > > is only for security/ code. > > But in LSM_UNSAFE_SHARE case, we have no check, right? I'm amazing why > we don't need anything. Yes. We rely on security/ code in this case, it can nack this exec if it looks unsafe. IOW. If LSM_UNSAFE_SHARE is not set, we promise that this fs has a single user: the execing thread (it will kill other subthreads which can have the same fs). That is why we need to cancel any attempt to create another CLONE_FS process in between. But let me repeat this is only my speculations, I know nothing about selinux and selinux_bprm_set_creds() in particular. Although it looks obvious that potentially exec with the shared ->fs has the additional security problems. Kosaki, thank you for review! Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
On 11/22, KOSAKI Motohiro wrote: I have found no problem in this patch. However, I have a very basic question. Why do we need to keep fs-in_exec? To ensure that a sub-thread can't create a new process with the same -fs while we are doing exec without LSM_UNSAFE_SHARE, I guess. This is only for security/ code. But in LSM_UNSAFE_SHARE case, we have no check, right? I'm amazing why we don't need anything. Yes. We rely on security/ code in this case, it can nack this exec if it looks unsafe. IOW. If LSM_UNSAFE_SHARE is not set, we promise that this fs has a single user: the execing thread (it will kill other subthreads which can have the same fs). That is why we need to cancel any attempt to create another CLONE_FS process in between. But let me repeat this is only my speculations, I know nothing about selinux and selinux_bprm_set_creds() in particular. Although it looks obvious that potentially exec with the shared -fs has the additional security problems. Kosaki, thank you for review! Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
>> I have found no problem in this patch. However, I have a very basic question. >> Why do we need to keep fs->in_exec? > > To ensure that a sub-thread can't create a new process with the same > ->fs while we are doing exec without LSM_UNSAFE_SHARE, I guess. This > is only for security/ code. But in LSM_UNSAFE_SHARE case, we have no check, right? I'm amazing why we don't need anything. > >> If it is correct, >> can't we move it it to signal->in_exec? > > Yes, perhaps, I am thinking about more cleanups too. But not that this > will add the subtle change. CLONE_THREAD doesn't require CLONE_FS, so > copy_fs() can fail even it the caller doesn't share ->fs with the execing > thread. And we still need fs->lock to set signal->in_exec, this looks > a bit strange. Oops. Yes, this is totally odd. Sorry, we need to stop off topic discussion. Anyway Acked-by: KOSAKI Motohiro > >> I am not expert in this area and I may overlook something. > > Neither me ;) So this patch tries to not change the current logic. > > I feel that perhaps we can do more cleanups, but I am not really sure > and this needs a separate change. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
On 11/22, KOSAKI Motohiro wrote: > > (11/22/2013 12:54 PM), Oleg Nesterov wrote: > > fs_struct->in_exec == T means that this ->fs is used by a single > > process (thread group), and one of the treads does do_execve(). > > > > To avoid the mt-exec races this code has the following complications: > > > > 1. check_unsafe_exec() returns -EBUSY if ->in_exec was > >already set by another thread. > > > > 2. do_execve_common() records "clear_in_exec" to ensure > >that the error path can only clear ->in_exec if it was > >set by current. > > > > However, after 9b1bf12d5d51 "signals: move cred_guard_mutex from > > task_struct to signal_struct" we do not need these complications: > > > > 1. We can't race with our sub-thread, this is called under > >per-process ->cred_guard_mutex. And we can't race with > >another CLONE_FS task, we already checked that this fs > >is not shared. > > > >We can remove the dead -EAGAIN logic. > > > > 2. "out_unmark:" in do_execve_common() is either called > >under ->cred_guard_mutex, or after de_thread() which > >kills other threads, so we can't race with sub-thread > >which could set ->in_exec. And if ->fs is shared with > >another process ->in_exec should be false anyway. > > > >We can clear in_exec unconditionally. > > > > This also means that check_unsafe_exec() can be void. > > > > Signed-off-by: Oleg Nesterov > > I have found no problem in this patch. However, I have a very basic question. > Why do we need to keep fs->in_exec? To ensure that a sub-thread can't create a new process with the same ->fs while we are doing exec without LSM_UNSAFE_SHARE, I guess. This is only for security/ code. > If it is correct, > can't we move it it to signal->in_exec? Yes, perhaps, I am thinking about more cleanups too. But not that this will add the subtle change. CLONE_THREAD doesn't require CLONE_FS, so copy_fs() can fail even it the caller doesn't share ->fs with the execing thread. And we still need fs->lock to set signal->in_exec, this looks a bit strange. > I am not expert in this area and I may overlook something. Neither me ;) So this patch tries to not change the current logic. I feel that perhaps we can do more cleanups, but I am not really sure and this needs a separate change. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
(11/22/2013 12:54 PM), Oleg Nesterov wrote: > fs_struct->in_exec == T means that this ->fs is used by a single > process (thread group), and one of the treads does do_execve(). > > To avoid the mt-exec races this code has the following complications: > > 1. check_unsafe_exec() returns -EBUSY if ->in_exec was > already set by another thread. > > 2. do_execve_common() records "clear_in_exec" to ensure > that the error path can only clear ->in_exec if it was > set by current. > > However, after 9b1bf12d5d51 "signals: move cred_guard_mutex from > task_struct to signal_struct" we do not need these complications: > > 1. We can't race with our sub-thread, this is called under > per-process ->cred_guard_mutex. And we can't race with > another CLONE_FS task, we already checked that this fs > is not shared. > > We can remove the dead -EAGAIN logic. > > 2. "out_unmark:" in do_execve_common() is either called > under ->cred_guard_mutex, or after de_thread() which > kills other threads, so we can't race with sub-thread > which could set ->in_exec. And if ->fs is shared with > another process ->in_exec should be false anyway. > > We can clear in_exec unconditionally. > > This also means that check_unsafe_exec() can be void. > > Signed-off-by: Oleg Nesterov I have found no problem in this patch. However, I have a very basic question. Why do we need to keep fs->in_exec? If I understand correctly, it is needed for retrieving fork() and exec() race in the same process. If it is correct, can't we move it it to signal->in_exec? It seems to match signal->cred_guard_mutex and _I_ can easily understand what the code want. In the other words, currently we have no protection against making new thread when p->fs is shared w/ another process and I have no idea why multi process sharing influence multi thread behavior. I am not expert in this area and I may overlook something. Please correct me if I am silly. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
fs_struct->in_exec == T means that this ->fs is used by a single
process (thread group), and one of the treads does do_execve().
To avoid the mt-exec races this code has the following complications:
1. check_unsafe_exec() returns -EBUSY if ->in_exec was
already set by another thread.
2. do_execve_common() records "clear_in_exec" to ensure
that the error path can only clear ->in_exec if it was
set by current.
However, after 9b1bf12d5d51 "signals: move cred_guard_mutex from
task_struct to signal_struct" we do not need these complications:
1. We can't race with our sub-thread, this is called under
per-process ->cred_guard_mutex. And we can't race with
another CLONE_FS task, we already checked that this fs
is not shared.
We can remove the dead -EAGAIN logic.
2. "out_unmark:" in do_execve_common() is either called
under ->cred_guard_mutex, or after de_thread() which
kills other threads, so we can't race with sub-thread
which could set ->in_exec. And if ->fs is shared with
another process ->in_exec should be false anyway.
We can clear in_exec unconditionally.
This also means that check_unsafe_exec() can be void.
Signed-off-by: Oleg Nesterov
---
fs/exec.c | 29 -
1 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 0cd9c25..60eb5c5 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1225,11 +1225,10 @@ EXPORT_SYMBOL(install_exec_creds);
* - the caller must hold ->cred_guard_mutex to protect against
* PTRACE_ATTACH
*/
-static int check_unsafe_exec(struct linux_binprm *bprm)
+static void check_unsafe_exec(struct linux_binprm *bprm)
{
struct task_struct *p = current, *t;
unsigned n_fs;
- int res = 0;
if (p->ptrace) {
if (p->ptrace & PT_PTRACE_CAP)
@@ -1255,22 +1254,15 @@ static int check_unsafe_exec(struct linux_binprm *bprm)
}
rcu_read_unlock();
- if (p->fs->users > n_fs) {
+ if (p->fs->users > n_fs)
bprm->unsafe |= LSM_UNSAFE_SHARE;
- } else {
- res = -EAGAIN;
- if (!p->fs->in_exec) {
- p->fs->in_exec = 1;
- res = 1;
- }
- }
+ else
+ p->fs->in_exec = 1;
spin_unlock(>fs->lock);
-
- return res;
}
-/*
- * Fill the binprm structure from the inode.
+/*
+ * Fill the binprm structure from the inode.
* Check permissions, then read the first 128 (BINPRM_BUF_SIZE) bytes
*
* This may be called multiple times for binary chains (scripts for example).
@@ -1461,7 +1453,6 @@ static int do_execve_common(const char *filename,
struct linux_binprm *bprm;
struct file *file;
struct files_struct *displaced;
- bool clear_in_exec;
int retval;
/*
@@ -1493,10 +1484,7 @@ static int do_execve_common(const char *filename,
if (retval)
goto out_free;
- retval = check_unsafe_exec(bprm);
- if (retval < 0)
- goto out_free;
- clear_in_exec = retval;
+ check_unsafe_exec(bprm);
current->in_execve = 1;
file = open_exec(filename);
@@ -1565,8 +1553,7 @@ out_file:
}
out_unmark:
- if (clear_in_exec)
- current->fs->in_exec = 0;
+ current->fs->in_exec = 0;
current->in_execve = 0;
out_free:
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
fs_struct-in_exec == T means that this -fs is used by a single
process (thread group), and one of the treads does do_execve().
To avoid the mt-exec races this code has the following complications:
1. check_unsafe_exec() returns -EBUSY if -in_exec was
already set by another thread.
2. do_execve_common() records clear_in_exec to ensure
that the error path can only clear -in_exec if it was
set by current.
However, after 9b1bf12d5d51 signals: move cred_guard_mutex from
task_struct to signal_struct we do not need these complications:
1. We can't race with our sub-thread, this is called under
per-process -cred_guard_mutex. And we can't race with
another CLONE_FS task, we already checked that this fs
is not shared.
We can remove the dead -EAGAIN logic.
2. out_unmark: in do_execve_common() is either called
under -cred_guard_mutex, or after de_thread() which
kills other threads, so we can't race with sub-thread
which could set -in_exec. And if -fs is shared with
another process -in_exec should be false anyway.
We can clear in_exec unconditionally.
This also means that check_unsafe_exec() can be void.
Signed-off-by: Oleg Nesterov o...@redhat.com
---
fs/exec.c | 29 -
1 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 0cd9c25..60eb5c5 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1225,11 +1225,10 @@ EXPORT_SYMBOL(install_exec_creds);
* - the caller must hold -cred_guard_mutex to protect against
* PTRACE_ATTACH
*/
-static int check_unsafe_exec(struct linux_binprm *bprm)
+static void check_unsafe_exec(struct linux_binprm *bprm)
{
struct task_struct *p = current, *t;
unsigned n_fs;
- int res = 0;
if (p-ptrace) {
if (p-ptrace PT_PTRACE_CAP)
@@ -1255,22 +1254,15 @@ static int check_unsafe_exec(struct linux_binprm *bprm)
}
rcu_read_unlock();
- if (p-fs-users n_fs) {
+ if (p-fs-users n_fs)
bprm-unsafe |= LSM_UNSAFE_SHARE;
- } else {
- res = -EAGAIN;
- if (!p-fs-in_exec) {
- p-fs-in_exec = 1;
- res = 1;
- }
- }
+ else
+ p-fs-in_exec = 1;
spin_unlock(p-fs-lock);
-
- return res;
}
-/*
- * Fill the binprm structure from the inode.
+/*
+ * Fill the binprm structure from the inode.
* Check permissions, then read the first 128 (BINPRM_BUF_SIZE) bytes
*
* This may be called multiple times for binary chains (scripts for example).
@@ -1461,7 +1453,6 @@ static int do_execve_common(const char *filename,
struct linux_binprm *bprm;
struct file *file;
struct files_struct *displaced;
- bool clear_in_exec;
int retval;
/*
@@ -1493,10 +1484,7 @@ static int do_execve_common(const char *filename,
if (retval)
goto out_free;
- retval = check_unsafe_exec(bprm);
- if (retval 0)
- goto out_free;
- clear_in_exec = retval;
+ check_unsafe_exec(bprm);
current-in_execve = 1;
file = open_exec(filename);
@@ -1565,8 +1553,7 @@ out_file:
}
out_unmark:
- if (clear_in_exec)
- current-fs-in_exec = 0;
+ current-fs-in_exec = 0;
current-in_execve = 0;
out_free:
--
1.5.5.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
(11/22/2013 12:54 PM), Oleg Nesterov wrote: fs_struct-in_exec == T means that this -fs is used by a single process (thread group), and one of the treads does do_execve(). To avoid the mt-exec races this code has the following complications: 1. check_unsafe_exec() returns -EBUSY if -in_exec was already set by another thread. 2. do_execve_common() records clear_in_exec to ensure that the error path can only clear -in_exec if it was set by current. However, after 9b1bf12d5d51 signals: move cred_guard_mutex from task_struct to signal_struct we do not need these complications: 1. We can't race with our sub-thread, this is called under per-process -cred_guard_mutex. And we can't race with another CLONE_FS task, we already checked that this fs is not shared. We can remove the dead -EAGAIN logic. 2. out_unmark: in do_execve_common() is either called under -cred_guard_mutex, or after de_thread() which kills other threads, so we can't race with sub-thread which could set -in_exec. And if -fs is shared with another process -in_exec should be false anyway. We can clear in_exec unconditionally. This also means that check_unsafe_exec() can be void. Signed-off-by: Oleg Nesterov o...@redhat.com I have found no problem in this patch. However, I have a very basic question. Why do we need to keep fs-in_exec? If I understand correctly, it is needed for retrieving fork() and exec() race in the same process. If it is correct, can't we move it it to signal-in_exec? It seems to match signal-cred_guard_mutex and _I_ can easily understand what the code want. In the other words, currently we have no protection against making new thread when p-fs is shared w/ another process and I have no idea why multi process sharing influence multi thread behavior. I am not expert in this area and I may overlook something. Please correct me if I am silly. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
On 11/22, KOSAKI Motohiro wrote: (11/22/2013 12:54 PM), Oleg Nesterov wrote: fs_struct-in_exec == T means that this -fs is used by a single process (thread group), and one of the treads does do_execve(). To avoid the mt-exec races this code has the following complications: 1. check_unsafe_exec() returns -EBUSY if -in_exec was already set by another thread. 2. do_execve_common() records clear_in_exec to ensure that the error path can only clear -in_exec if it was set by current. However, after 9b1bf12d5d51 signals: move cred_guard_mutex from task_struct to signal_struct we do not need these complications: 1. We can't race with our sub-thread, this is called under per-process -cred_guard_mutex. And we can't race with another CLONE_FS task, we already checked that this fs is not shared. We can remove the dead -EAGAIN logic. 2. out_unmark: in do_execve_common() is either called under -cred_guard_mutex, or after de_thread() which kills other threads, so we can't race with sub-thread which could set -in_exec. And if -fs is shared with another process -in_exec should be false anyway. We can clear in_exec unconditionally. This also means that check_unsafe_exec() can be void. Signed-off-by: Oleg Nesterov o...@redhat.com I have found no problem in this patch. However, I have a very basic question. Why do we need to keep fs-in_exec? To ensure that a sub-thread can't create a new process with the same -fs while we are doing exec without LSM_UNSAFE_SHARE, I guess. This is only for security/ code. If it is correct, can't we move it it to signal-in_exec? Yes, perhaps, I am thinking about more cleanups too. But not that this will add the subtle change. CLONE_THREAD doesn't require CLONE_FS, so copy_fs() can fail even it the caller doesn't share -fs with the execing thread. And we still need fs-lock to set signal-in_exec, this looks a bit strange. I am not expert in this area and I may overlook something. Neither me ;) So this patch tries to not change the current logic. I feel that perhaps we can do more cleanups, but I am not really sure and this needs a separate change. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
I have found no problem in this patch. However, I have a very basic question. Why do we need to keep fs-in_exec? To ensure that a sub-thread can't create a new process with the same -fs while we are doing exec without LSM_UNSAFE_SHARE, I guess. This is only for security/ code. But in LSM_UNSAFE_SHARE case, we have no check, right? I'm amazing why we don't need anything. If it is correct, can't we move it it to signal-in_exec? Yes, perhaps, I am thinking about more cleanups too. But not that this will add the subtle change. CLONE_THREAD doesn't require CLONE_FS, so copy_fs() can fail even it the caller doesn't share -fs with the execing thread. And we still need fs-lock to set signal-in_exec, this looks a bit strange. Oops. Yes, this is totally odd. Sorry, we need to stop off topic discussion. Anyway Acked-by: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com I am not expert in this area and I may overlook something. Neither me ;) So this patch tries to not change the current logic. I feel that perhaps we can do more cleanups, but I am not really sure and this needs a separate change. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
