Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 12/19, Richard Guy Briggs wrote: > > On 13/12/18, Oleg Nesterov wrote: > > > Otherwise I can't understand your email, at least right now... I do not > > know how/where audit uses parent/real_parent. > > It uses real_parent to include the ppid number of a process in a couple > of log records. I did a quick grep, it seems that audit uses sys_getppid() which should be fine. Nevermind, if you meant that (say) audit_alloc() paths use tsk->parent somehow, I agree this doesn't look right/safe. new_child->*parent can point to nowhere right after dup_task_struct() and there is no way to detect this. Unless, of course new_child->*parent == current, but copy_process() changes child->parent only after it takes tasklist_lock. > > But yes, unless tsk == current, the usage of tsk->*parent is not safe even > > under rcu_read_lock() unless you verify that this task was not unhashed. > > As I said, the only case I can see is in copy_process() when a signal is > pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it. > Still, that is enough to need to check it. Hmm, so I guess you are worried about audit_free? But this error path can be called even before it checks signal_pending(), suppose that copy_semundo() fails? So it seems that CLONE_PARENT/THREAD doesn't really matter because it audit_free() can be called before copy_process() sets ->parent = current? Most probably I misunderstood you, so please ignore. I am sure you fully understand the problems and do not need my comments ;) > So what are you saying? I should use pid_alive() inside the > rcu_read_lock() to verify it is not unhashed since I don't have anything > to do with ->ptrace or any other task lock? In fact, the answer is > under my nose in __task_pid_nr_ns(), which already uses this approach. Yes. task->parent (or real_parent) can only make sense if this task can be re-parented (if parent exits, or debugger detaches). If the task is dead (removed from the parent->children list), obviously nobody can update this pointer. And this reminds me we should simply clear ->parent/real_parent on exit. And ->group_leader. I'll try to make the patch(s), after I finish thread_group changes. Unfortunately this needs a lot of grepping. 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] apparmor: remove the task arg from may_change_ptraced_domain()
On 12/19, Richard Guy Briggs wrote: On 13/12/18, Oleg Nesterov wrote: Otherwise I can't understand your email, at least right now... I do not know how/where audit uses parent/real_parent. It uses real_parent to include the ppid number of a process in a couple of log records. I did a quick grep, it seems that audit uses sys_getppid() which should be fine. Nevermind, if you meant that (say) audit_alloc() paths use tsk-parent somehow, I agree this doesn't look right/safe. new_child-*parent can point to nowhere right after dup_task_struct() and there is no way to detect this. Unless, of course new_child-*parent == current, but copy_process() changes child-parent only after it takes tasklist_lock. But yes, unless tsk == current, the usage of tsk-*parent is not safe even under rcu_read_lock() unless you verify that this task was not unhashed. As I said, the only case I can see is in copy_process() when a signal is pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it. Still, that is enough to need to check it. Hmm, so I guess you are worried about audit_free? But this error path can be called even before it checks signal_pending(), suppose that copy_semundo() fails? So it seems that CLONE_PARENT/THREAD doesn't really matter because it audit_free() can be called before copy_process() sets -parent = current? Most probably I misunderstood you, so please ignore. I am sure you fully understand the problems and do not need my comments ;) So what are you saying? I should use pid_alive() inside the rcu_read_lock() to verify it is not unhashed since I don't have anything to do with -ptrace or any other task lock? In fact, the answer is under my nose in __task_pid_nr_ns(), which already uses this approach. Yes. task-parent (or real_parent) can only make sense if this task can be re-parented (if parent exits, or debugger detaches). If the task is dead (removed from the parent-children list), obviously nobody can update this pointer. And this reminds me we should simply clear -parent/real_parent on exit. And -group_leader. I'll try to make the patch(s), after I finish thread_group changes. Unfortunately this needs a lot of grepping. 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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 12/19/2013 08:36 PM, Richard Guy Briggs wrote: > On 13/12/18, Oleg Nesterov wrote: >> On 12/18, Richard Guy Briggs wrote: >>> >>> Bcc: r...@redhat.com >>> Subject: Re: [PATCH] apparmor: remove the "task" arg from >>> may_change_ptraced_domain() >>> Reply-To: >>> In-Reply-To: <20130926132519.gy13...@madcap2.tricolour.ca> >> >> The subject is empty ;) I changed it to match the above. > > HTH?!? Thanks for adding it. (more below...) > >>> On 13/09/26, Richard Guy Briggs wrote: >>>> On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: >>>>> On 09/23, Richard Guy Briggs wrote: >>>>>> >>>>>> On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: >>>>>>> Unless task == current ptrace_parent(task) is not safe even under >>>>>>> rcu_read_lock() and most of the current users are not right. >>>>>> >>>>>> Could you point to an explanation of this? >>>>> >>>>> If this task exits before rcu_read_lock() ->parent can point to the >>>>> already freed/reused memory. >>>> >>>> Ok, understood. So even though the task may have exited, the task >>>> struct pointer is still valid, but not the contents of the task struct >>>> to which it points. >>> >>> [The thread also relates to the patch >>> "pid: get ppid pid_t of task in init_pid_ns safely" >>> in which sys_getppid() (which appears safe) is replaced with something that >>> references the init_pid_ns rather than current's pid_ns.] >>> >>> So, in the general case, that call is not safe, and we should at least >>> remove the task_struct argument. >> >> I changed my mind, please see the recent discussion with Paul: >> >> http://marc.info/?t=13862628191 >> >> instead we should document why ptrace_parent() is safe without pid_alive(). > > Interesting. I wasn't aware of pid_alive(), but that would certainly > help. > >> I hope that the change in apparmor was fine anyway. > > Yes, I'm fine with apparmor change, if it was deemed that the ppid > wasn't needed. If it is, then it should use this new task_ppid_nr(). it wasn't needed, changes where made years ago to allow us to get rid of using the parent pid. Its was left in for a transition period and just had never been removed. > Better yet I think to generalize it to anticipate auditd in containers. > yep, that is the way to go -- 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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 13/12/18, Oleg Nesterov wrote: > On 12/18, Richard Guy Briggs wrote: > > > > Bcc: r...@redhat.com > > Subject: Re: [PATCH] apparmor: remove the "task" arg from > > may_change_ptraced_domain() > > Reply-To: > > In-Reply-To: <20130926132519.gy13...@madcap2.tricolour.ca> > > The subject is empty ;) I changed it to match the above. HTH?!? Thanks for adding it. (more below...) > > On 13/09/26, Richard Guy Briggs wrote: > > > On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: > > > > On 09/23, Richard Guy Briggs wrote: > > > > > > > > > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: > > > > > > Unless task == current ptrace_parent(task) is not safe even under > > > > > > rcu_read_lock() and most of the current users are not right. > > > > > > > > > > Could you point to an explanation of this? > > > > > > > > If this task exits before rcu_read_lock() ->parent can point to the > > > > already freed/reused memory. > > > > > > Ok, understood. So even though the task may have exited, the task > > > struct pointer is still valid, but not the contents of the task struct > > > to which it points. > > > > [The thread also relates to the patch > > "pid: get ppid pid_t of task in init_pid_ns safely" > > in which sys_getppid() (which appears safe) is replaced with something that > > references the init_pid_ns rather than current's pid_ns.] > > > > So, in the general case, that call is not safe, and we should at least > > remove the task_struct argument. > > I changed my mind, please see the recent discussion with Paul: > > http://marc.info/?t=13862628191 > > instead we should document why ptrace_parent() is safe without pid_alive(). Interesting. I wasn't aware of pid_alive(), but that would certainly help. > I hope that the change in apparmor was fine anyway. Yes, I'm fine with apparmor change, if it was deemed that the ppid wasn't needed. If it is, then it should use this new task_ppid_nr(). Better yet I think to generalize it to anticipate auditd in containers. > Otherwise I can't understand your email, at least right now... I do not > know how/where audit uses parent/real_parent. It uses real_parent to include the ppid number of a process in a couple of log records. > But yes, unless tsk == current, the usage of tsk->*parent is not safe even > under rcu_read_lock() unless you verify that this task was not unhashed. As I said, the only case I can see is in copy_process() when a signal is pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it. Still, that is enough to need to check it. > ptrace_parent() is safe because it checks ->ptrace. Previously I thought > we should not rely on this, but the additional pid_alive() looks ugly so > it would be better to simply document this. I'll send the patch. So what are you saying? I should use pid_alive() inside the rcu_read_lock() to verify it is not unhashed since I don't have anything to do with ->ptrace or any other task lock? In fact, the answer is under my nose in __task_pid_nr_ns(), which already uses this approach. And rcu_read_lock() can be nested. > Oleg. - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- 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] apparmor: remove the task arg from may_change_ptraced_domain()
On 13/12/18, Oleg Nesterov wrote: On 12/18, Richard Guy Briggs wrote: Bcc: r...@redhat.com Subject: Re: [PATCH] apparmor: remove the task arg from may_change_ptraced_domain() Reply-To: In-Reply-To: 20130926132519.gy13...@madcap2.tricolour.ca The subject is empty ;) I changed it to match the above. HTH?!? Thanks for adding it. (more below...) On 13/09/26, Richard Guy Briggs wrote: On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: On 09/23, Richard Guy Briggs wrote: On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: Unless task == current ptrace_parent(task) is not safe even under rcu_read_lock() and most of the current users are not right. Could you point to an explanation of this? If this task exits before rcu_read_lock() -parent can point to the already freed/reused memory. Ok, understood. So even though the task may have exited, the task struct pointer is still valid, but not the contents of the task struct to which it points. [The thread also relates to the patch pid: get ppid pid_t of task in init_pid_ns safely in which sys_getppid() (which appears safe) is replaced with something that references the init_pid_ns rather than current's pid_ns.] So, in the general case, that call is not safe, and we should at least remove the task_struct argument. I changed my mind, please see the recent discussion with Paul: http://marc.info/?t=13862628191 instead we should document why ptrace_parent() is safe without pid_alive(). Interesting. I wasn't aware of pid_alive(), but that would certainly help. I hope that the change in apparmor was fine anyway. Yes, I'm fine with apparmor change, if it was deemed that the ppid wasn't needed. If it is, then it should use this new task_ppid_nr(). Better yet I think to generalize it to anticipate auditd in containers. Otherwise I can't understand your email, at least right now... I do not know how/where audit uses parent/real_parent. It uses real_parent to include the ppid number of a process in a couple of log records. But yes, unless tsk == current, the usage of tsk-*parent is not safe even under rcu_read_lock() unless you verify that this task was not unhashed. As I said, the only case I can see is in copy_process() when a signal is pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it. Still, that is enough to need to check it. ptrace_parent() is safe because it checks -ptrace. Previously I thought we should not rely on this, but the additional pid_alive() looks ugly so it would be better to simply document this. I'll send the patch. So what are you saying? I should use pid_alive() inside the rcu_read_lock() to verify it is not unhashed since I don't have anything to do with -ptrace or any other task lock? In fact, the answer is under my nose in __task_pid_nr_ns(), which already uses this approach. And rcu_read_lock() can be nested. Oleg. - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- 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] apparmor: remove the task arg from may_change_ptraced_domain()
On 12/19/2013 08:36 PM, Richard Guy Briggs wrote: On 13/12/18, Oleg Nesterov wrote: On 12/18, Richard Guy Briggs wrote: Bcc: r...@redhat.com Subject: Re: [PATCH] apparmor: remove the task arg from may_change_ptraced_domain() Reply-To: In-Reply-To: 20130926132519.gy13...@madcap2.tricolour.ca The subject is empty ;) I changed it to match the above. HTH?!? Thanks for adding it. (more below...) On 13/09/26, Richard Guy Briggs wrote: On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: On 09/23, Richard Guy Briggs wrote: On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: Unless task == current ptrace_parent(task) is not safe even under rcu_read_lock() and most of the current users are not right. Could you point to an explanation of this? If this task exits before rcu_read_lock() -parent can point to the already freed/reused memory. Ok, understood. So even though the task may have exited, the task struct pointer is still valid, but not the contents of the task struct to which it points. [The thread also relates to the patch pid: get ppid pid_t of task in init_pid_ns safely in which sys_getppid() (which appears safe) is replaced with something that references the init_pid_ns rather than current's pid_ns.] So, in the general case, that call is not safe, and we should at least remove the task_struct argument. I changed my mind, please see the recent discussion with Paul: http://marc.info/?t=13862628191 instead we should document why ptrace_parent() is safe without pid_alive(). Interesting. I wasn't aware of pid_alive(), but that would certainly help. I hope that the change in apparmor was fine anyway. Yes, I'm fine with apparmor change, if it was deemed that the ppid wasn't needed. If it is, then it should use this new task_ppid_nr(). it wasn't needed, changes where made years ago to allow us to get rid of using the parent pid. Its was left in for a transition period and just had never been removed. Better yet I think to generalize it to anticipate auditd in containers. yep, that is the way to go -- 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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 12/18, Richard Guy Briggs wrote: > > Bcc: r...@redhat.com > Subject: Re: [PATCH] apparmor: remove the "task" arg from > may_change_ptraced_domain() > Reply-To: > In-Reply-To: <20130926132519.gy13...@madcap2.tricolour.ca> The subject is empty ;) I changed it to match the above. > On 13/09/26, Richard Guy Briggs wrote: > > On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: > > > On 09/23, Richard Guy Briggs wrote: > > > > > > > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: > > > > > Unless task == current ptrace_parent(task) is not safe even under > > > > > rcu_read_lock() and most of the current users are not right. > > > > > > > > Could you point to an explanation of this? > > > > > > If this task exits before rcu_read_lock() ->parent can point to the > > > already freed/reused memory. > > > > Ok, understood. So even though the task may have exited, the task > > struct pointer is still valid, but not the contents of the task struct > > to which it points. > > [The thread also relates to the patch > "pid: get ppid pid_t of task in init_pid_ns safely" > in which sys_getppid() (which appears safe) is replaced with something that > references the init_pid_ns rather than current's pid_ns.] > > So, in the general case, that call is not safe, and we should at least > remove the task_struct argument. I changed my mind, please see the recent discussion with Paul: http://marc.info/?t=13862628191 instead we should document why ptrace_parent() is safe without pid_alive(). I hope that the change in apparmor was fine anyway. Otherwise I can't understand your email, at least right now... I do not know how/where audit uses parent/real_parent. But yes, unless tsk == current, the usage of tsk->*parent is not safe even under rcu_read_lock() unless you verify that this task was not unhashed. ptrace_parent() is safe because it checks ->ptrace. Previously I thought we should not rely on this, but the additional pid_alive() looks ugly so it would be better to simply document this. I'll send the patch. 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] apparmor: remove the task arg from may_change_ptraced_domain()
On 12/18, Richard Guy Briggs wrote: Bcc: r...@redhat.com Subject: Re: [PATCH] apparmor: remove the task arg from may_change_ptraced_domain() Reply-To: In-Reply-To: 20130926132519.gy13...@madcap2.tricolour.ca The subject is empty ;) I changed it to match the above. On 13/09/26, Richard Guy Briggs wrote: On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: On 09/23, Richard Guy Briggs wrote: On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: Unless task == current ptrace_parent(task) is not safe even under rcu_read_lock() and most of the current users are not right. Could you point to an explanation of this? If this task exits before rcu_read_lock() -parent can point to the already freed/reused memory. Ok, understood. So even though the task may have exited, the task struct pointer is still valid, but not the contents of the task struct to which it points. [The thread also relates to the patch pid: get ppid pid_t of task in init_pid_ns safely in which sys_getppid() (which appears safe) is replaced with something that references the init_pid_ns rather than current's pid_ns.] So, in the general case, that call is not safe, and we should at least remove the task_struct argument. I changed my mind, please see the recent discussion with Paul: http://marc.info/?t=13862628191 instead we should document why ptrace_parent() is safe without pid_alive(). I hope that the change in apparmor was fine anyway. Otherwise I can't understand your email, at least right now... I do not know how/where audit uses parent/real_parent. But yes, unless tsk == current, the usage of tsk-*parent is not safe even under rcu_read_lock() unless you verify that this task was not unhashed. ptrace_parent() is safe because it checks -ptrace. Previously I thought we should not rely on this, but the additional pid_alive() looks ugly so it would be better to simply document this. I'll send the patch. 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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: > On 09/23, Richard Guy Briggs wrote: > > > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: > > > Unless task == current ptrace_parent(task) is not safe even under > > > rcu_read_lock() and most of the current users are not right. > > > > Could you point to an explanation of this? > > If this task exits before rcu_read_lock() ->parent can point to the > already freed/reused memory. Ok, understood. So even though the task may have exited, the task struct pointer is still valid, but not the contents of the task struct to which it points. > (in the long term we should probably clear > ->parent/real_parent/group_leader/more in __unhash_process(), but > lets not discuss this right now ;) ...so that the contents are valid in a task struct of a task that has exited. Thanks for the (more obvious to me now) explanation. > Oleg. - RGB -- Richard Guy Briggs Senior Software Engineer Kernel Security AMER ENG Base Operating Systems Remote, Ottawa, Canada Voice: +1.647.777.2635 Internal: (81) 32635 Alt: +1.613.693.0684x3545 -- 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] apparmor: remove the task arg from may_change_ptraced_domain()
On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote: On 09/23, Richard Guy Briggs wrote: On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: Unless task == current ptrace_parent(task) is not safe even under rcu_read_lock() and most of the current users are not right. Could you point to an explanation of this? If this task exits before rcu_read_lock() -parent can point to the already freed/reused memory. Ok, understood. So even though the task may have exited, the task struct pointer is still valid, but not the contents of the task struct to which it points. (in the long term we should probably clear -parent/real_parent/group_leader/more in __unhash_process(), but lets not discuss this right now ;) ...so that the contents are valid in a task struct of a task that has exited. Thanks for the (more obvious to me now) explanation. Oleg. - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer Kernel Security AMER ENG Base Operating Systems Remote, Ottawa, Canada Voice: +1.647.777.2635 Internal: (81) 32635 Alt: +1.613.693.0684x3545 -- 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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 09/23, Richard Guy Briggs wrote: > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: > > Unless task == current ptrace_parent(task) is not safe even under > > rcu_read_lock() and most of the current users are not right. > > Could you point to an explanation of this? If this task exits before rcu_read_lock() ->parent can point to the already freed/reused memory. (in the long term we should probably clear ->parent/real_parent/group_leader/more in __unhash_process(), but lets not discuss this right now ;) > (Did you send a patch to fix the selinux hook?) No, sorry, I was sick. Will do. > Acked-by: Richard Guy Briggs Thanks! 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] apparmor: remove the task arg from may_change_ptraced_domain()
On 09/23, Richard Guy Briggs wrote: On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote: Unless task == current ptrace_parent(task) is not safe even under rcu_read_lock() and most of the current users are not right. Could you point to an explanation of this? If this task exits before rcu_read_lock() -parent can point to the already freed/reused memory. (in the long term we should probably clear -parent/real_parent/group_leader/more in __unhash_process(), but lets not discuss this right now ;) (Did you send a patch to fix the selinux hook?) No, sorry, I was sick. Will do. Acked-by: Richard Guy Briggs r...@redhat.com Thanks! 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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> Unless task == current ptrace_parent(task) is not safe even under
> rcu_read_lock() and most of the current users are not right.
Could you point to an explanation of this?
> So may_change_ptraced_domain(task) looks wrong as well. However it
> is always called with task == current so the code is actually fine.
> Remove this argument to make this fact clear.
>
> Note: perhaps we should simply kill ptrace_parent(), it buys almost
> nothing. And it is obviously racy, perhaps this should be fixed.
(Did you send a patch to fix the selinux hook?)
> Signed-off-by: Oleg Nesterov
Acked-by: Richard Guy Briggs
> ---
> security/apparmor/domain.c | 14 ++
> 1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 26c607c..8423558 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
>
> /**
> * may_change_ptraced_domain - check if can change profile on ptraced task
> - * @task: task we want to change profile of (NOT NULL)
> * @to_profile: profile to change to (NOT NULL)
> *
> - * Check if the task is ptraced and if so if the tracing task is allowed
> + * Check if current is ptraced and if so if the tracing task is allowed
> * to trace the new domain
> *
> * Returns: %0 or error if change not allowed
> */
> -static int may_change_ptraced_domain(struct task_struct *task,
> - struct aa_profile *to_profile)
> +static int may_change_ptraced_domain(struct aa_profile *to_profile)
> {
> struct task_struct *tracer;
> struct aa_profile *tracerp = NULL;
> int error = 0;
>
> rcu_read_lock();
> - tracer = ptrace_parent(task);
> + tracer = ptrace_parent(current);
> if (tracer)
> /* released below */
> tracerp = aa_get_task_profile(tracer);
> @@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> }
>
> if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
> - error = may_change_ptraced_domain(current, new_profile);
> + error = may_change_ptraced_domain(new_profile);
> if (error) {
> aa_put_profile(new_profile);
> goto audit;
> @@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64
> token, bool permtest)
> }
> }
>
> - error = may_change_ptraced_domain(current, hat);
> + error = may_change_ptraced_domain(hat);
> if (error) {
> info = "ptraced";
> error = -EPERM;
> @@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char
> *hname, bool onexec,
> }
>
> /* check if tracing task is allowed to trace target domain */
> - error = may_change_ptraced_domain(current, target);
> + error = may_change_ptraced_domain(target);
> if (error) {
> info = "ptrace prevents transition";
> goto audit;
> --
> 1.5.5.1
>
>
- RGB
--
Richard Guy Briggs
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
--
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] apparmor: remove the task arg from may_change_ptraced_domain()
On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
Unless task == current ptrace_parent(task) is not safe even under
rcu_read_lock() and most of the current users are not right.
Could you point to an explanation of this?
So may_change_ptraced_domain(task) looks wrong as well. However it
is always called with task == current so the code is actually fine.
Remove this argument to make this fact clear.
Note: perhaps we should simply kill ptrace_parent(), it buys almost
nothing. And it is obviously racy, perhaps this should be fixed.
(Did you send a patch to fix the selinux hook?)
Signed-off-by: Oleg Nesterov o...@redhat.com
Acked-by: Richard Guy Briggs r...@redhat.com
---
security/apparmor/domain.c | 14 ++
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 26c607c..8423558 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
/**
* may_change_ptraced_domain - check if can change profile on ptraced task
- * @task: task we want to change profile of (NOT NULL)
* @to_profile: profile to change to (NOT NULL)
*
- * Check if the task is ptraced and if so if the tracing task is allowed
+ * Check if current is ptraced and if so if the tracing task is allowed
* to trace the new domain
*
* Returns: %0 or error if change not allowed
*/
-static int may_change_ptraced_domain(struct task_struct *task,
- struct aa_profile *to_profile)
+static int may_change_ptraced_domain(struct aa_profile *to_profile)
{
struct task_struct *tracer;
struct aa_profile *tracerp = NULL;
int error = 0;
rcu_read_lock();
- tracer = ptrace_parent(task);
+ tracer = ptrace_parent(current);
if (tracer)
/* released below */
tracerp = aa_get_task_profile(tracer);
@@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
}
if (bprm-unsafe (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
- error = may_change_ptraced_domain(current, new_profile);
+ error = may_change_ptraced_domain(new_profile);
if (error) {
aa_put_profile(new_profile);
goto audit;
@@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64
token, bool permtest)
}
}
- error = may_change_ptraced_domain(current, hat);
+ error = may_change_ptraced_domain(hat);
if (error) {
info = ptraced;
error = -EPERM;
@@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char
*hname, bool onexec,
}
/* check if tracing task is allowed to trace target domain */
- error = may_change_ptraced_domain(current, target);
+ error = may_change_ptraced_domain(target);
if (error) {
info = ptrace prevents transition;
goto audit;
--
1.5.5.1
- RGB
--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
--
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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 09/16/2013 07:20 AM, Oleg Nesterov wrote:
> Unless task == current ptrace_parent(task) is not safe even under
> rcu_read_lock() and most of the current users are not right.
>
> So may_change_ptraced_domain(task) looks wrong as well. However it
> is always called with task == current so the code is actually fine.
> Remove this argument to make this fact clear.
>
> Note: perhaps we should simply kill ptrace_parent(), it buys almost
> nothing. And it is obviously racy, perhaps this should be fixed.
>
> Signed-off-by: Oleg Nesterov
Acked-by: John Johansen
> ---
> security/apparmor/domain.c | 14 ++
> 1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 26c607c..8423558 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
>
> /**
> * may_change_ptraced_domain - check if can change profile on ptraced task
> - * @task: task we want to change profile of (NOT NULL)
> * @to_profile: profile to change to (NOT NULL)
> *
> - * Check if the task is ptraced and if so if the tracing task is allowed
> + * Check if current is ptraced and if so if the tracing task is allowed
> * to trace the new domain
> *
> * Returns: %0 or error if change not allowed
> */
> -static int may_change_ptraced_domain(struct task_struct *task,
> - struct aa_profile *to_profile)
> +static int may_change_ptraced_domain(struct aa_profile *to_profile)
> {
> struct task_struct *tracer;
> struct aa_profile *tracerp = NULL;
> int error = 0;
>
> rcu_read_lock();
> - tracer = ptrace_parent(task);
> + tracer = ptrace_parent(current);
> if (tracer)
> /* released below */
> tracerp = aa_get_task_profile(tracer);
> @@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> }
>
> if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
> - error = may_change_ptraced_domain(current, new_profile);
> + error = may_change_ptraced_domain(new_profile);
> if (error) {
> aa_put_profile(new_profile);
> goto audit;
> @@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64
> token, bool permtest)
> }
> }
>
> - error = may_change_ptraced_domain(current, hat);
> + error = may_change_ptraced_domain(hat);
> if (error) {
> info = "ptraced";
> error = -EPERM;
> @@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char
> *hname, bool onexec,
> }
>
> /* check if tracing task is allowed to trace target domain */
> - error = may_change_ptraced_domain(current, target);
> + error = may_change_ptraced_domain(target);
> if (error) {
> info = "ptrace prevents transition";
> goto audit;
>
--
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] apparmor: remove the "task" arg from may_change_ptraced_domain()
On 09/16, Oleg Nesterov wrote:
>
> Unless task == current ptrace_parent(task) is not safe even under
> rcu_read_lock() and most of the current users are not right.
In particular selinux is buggy. But this needs another simple patch,
will do tomorrow.
> So may_change_ptraced_domain(task) looks wrong as well. However it
> is always called with task == current so the code is actually fine.
> Remove this argument to make this fact clear.
>
> Note: perhaps we should simply kill ptrace_parent(), it buys almost
> nothing. And it is obviously racy, perhaps this should be fixed.
>
> Signed-off-by: Oleg Nesterov
> ---
> security/apparmor/domain.c | 14 ++
> 1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 26c607c..8423558 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
>
> /**
> * may_change_ptraced_domain - check if can change profile on ptraced task
> - * @task: task we want to change profile of (NOT NULL)
> * @to_profile: profile to change to (NOT NULL)
> *
> - * Check if the task is ptraced and if so if the tracing task is allowed
> + * Check if current is ptraced and if so if the tracing task is allowed
> * to trace the new domain
> *
> * Returns: %0 or error if change not allowed
> */
> -static int may_change_ptraced_domain(struct task_struct *task,
> - struct aa_profile *to_profile)
> +static int may_change_ptraced_domain(struct aa_profile *to_profile)
> {
> struct task_struct *tracer;
> struct aa_profile *tracerp = NULL;
> int error = 0;
>
> rcu_read_lock();
> - tracer = ptrace_parent(task);
> + tracer = ptrace_parent(current);
> if (tracer)
> /* released below */
> tracerp = aa_get_task_profile(tracer);
> @@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> }
>
> if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
> - error = may_change_ptraced_domain(current, new_profile);
> + error = may_change_ptraced_domain(new_profile);
> if (error) {
> aa_put_profile(new_profile);
> goto audit;
> @@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64
> token, bool permtest)
> }
> }
>
> - error = may_change_ptraced_domain(current, hat);
> + error = may_change_ptraced_domain(hat);
> if (error) {
> info = "ptraced";
> error = -EPERM;
> @@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char
> *hname, bool onexec,
> }
>
> /* check if tracing task is allowed to trace target domain */
> - error = may_change_ptraced_domain(current, target);
> + error = may_change_ptraced_domain(target);
> if (error) {
> info = "ptrace prevents transition";
> goto audit;
> --
> 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] apparmor: remove the task arg from may_change_ptraced_domain()
On 09/16, Oleg Nesterov wrote:
Unless task == current ptrace_parent(task) is not safe even under
rcu_read_lock() and most of the current users are not right.
In particular selinux is buggy. But this needs another simple patch,
will do tomorrow.
So may_change_ptraced_domain(task) looks wrong as well. However it
is always called with task == current so the code is actually fine.
Remove this argument to make this fact clear.
Note: perhaps we should simply kill ptrace_parent(), it buys almost
nothing. And it is obviously racy, perhaps this should be fixed.
Signed-off-by: Oleg Nesterov o...@redhat.com
---
security/apparmor/domain.c | 14 ++
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 26c607c..8423558 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
/**
* may_change_ptraced_domain - check if can change profile on ptraced task
- * @task: task we want to change profile of (NOT NULL)
* @to_profile: profile to change to (NOT NULL)
*
- * Check if the task is ptraced and if so if the tracing task is allowed
+ * Check if current is ptraced and if so if the tracing task is allowed
* to trace the new domain
*
* Returns: %0 or error if change not allowed
*/
-static int may_change_ptraced_domain(struct task_struct *task,
- struct aa_profile *to_profile)
+static int may_change_ptraced_domain(struct aa_profile *to_profile)
{
struct task_struct *tracer;
struct aa_profile *tracerp = NULL;
int error = 0;
rcu_read_lock();
- tracer = ptrace_parent(task);
+ tracer = ptrace_parent(current);
if (tracer)
/* released below */
tracerp = aa_get_task_profile(tracer);
@@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
}
if (bprm-unsafe (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
- error = may_change_ptraced_domain(current, new_profile);
+ error = may_change_ptraced_domain(new_profile);
if (error) {
aa_put_profile(new_profile);
goto audit;
@@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64
token, bool permtest)
}
}
- error = may_change_ptraced_domain(current, hat);
+ error = may_change_ptraced_domain(hat);
if (error) {
info = ptraced;
error = -EPERM;
@@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char
*hname, bool onexec,
}
/* check if tracing task is allowed to trace target domain */
- error = may_change_ptraced_domain(current, target);
+ error = may_change_ptraced_domain(target);
if (error) {
info = ptrace prevents transition;
goto audit;
--
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] apparmor: remove the task arg from may_change_ptraced_domain()
On 09/16/2013 07:20 AM, Oleg Nesterov wrote:
Unless task == current ptrace_parent(task) is not safe even under
rcu_read_lock() and most of the current users are not right.
So may_change_ptraced_domain(task) looks wrong as well. However it
is always called with task == current so the code is actually fine.
Remove this argument to make this fact clear.
Note: perhaps we should simply kill ptrace_parent(), it buys almost
nothing. And it is obviously racy, perhaps this should be fixed.
Signed-off-by: Oleg Nesterov o...@redhat.com
Acked-by: John Johansen john.johan...@canonical.com
---
security/apparmor/domain.c | 14 ++
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 26c607c..8423558 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
/**
* may_change_ptraced_domain - check if can change profile on ptraced task
- * @task: task we want to change profile of (NOT NULL)
* @to_profile: profile to change to (NOT NULL)
*
- * Check if the task is ptraced and if so if the tracing task is allowed
+ * Check if current is ptraced and if so if the tracing task is allowed
* to trace the new domain
*
* Returns: %0 or error if change not allowed
*/
-static int may_change_ptraced_domain(struct task_struct *task,
- struct aa_profile *to_profile)
+static int may_change_ptraced_domain(struct aa_profile *to_profile)
{
struct task_struct *tracer;
struct aa_profile *tracerp = NULL;
int error = 0;
rcu_read_lock();
- tracer = ptrace_parent(task);
+ tracer = ptrace_parent(current);
if (tracer)
/* released below */
tracerp = aa_get_task_profile(tracer);
@@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
}
if (bprm-unsafe (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
- error = may_change_ptraced_domain(current, new_profile);
+ error = may_change_ptraced_domain(new_profile);
if (error) {
aa_put_profile(new_profile);
goto audit;
@@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64
token, bool permtest)
}
}
- error = may_change_ptraced_domain(current, hat);
+ error = may_change_ptraced_domain(hat);
if (error) {
info = ptraced;
error = -EPERM;
@@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char
*hname, bool onexec,
}
/* check if tracing task is allowed to trace target domain */
- error = may_change_ptraced_domain(current, target);
+ error = may_change_ptraced_domain(target);
if (error) {
info = ptrace prevents transition;
goto audit;
--
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/
