[PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare

[PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare

[Mateusz Guzik]

audit_exe_compare directly accesses mm->exe_file without making sure the
object is stable. Fixing it using current primitives results in
partially duplicating what proc_exe_link is doing.

As such, introduce a trivial helper which can be used in both places and
fix the func.

Changes since v1:
* removed an unused 'out' label which crept in

Mateusz Guzik (2):
  mm: introduce get_task_exe_file
  audit: fix exe_file access in audit_exe_compare

 fs/proc/base.c       |  7 +------
 include/linux/mm.h   |  1 +
 kernel/audit_watch.c |  8 +++++---
 kernel/fork.c        | 23 +++++++++++++++++++++++
 4 files changed, 30 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCHv2 1/2] mm: introduce get_task_exe_file

[Mateusz Guzik]

For more convenient access if one has a pointer to the task.

As a minor nit take advantage of the fact that only task lock + rcu are
needed to safely grab ->exe_file. This saves mm refcount dance.

Use the helper in proc_exe_link.

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/base.c     |  7 +------
 include/linux/mm.h |  1 +
 kernel/fork.c      | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2ed41cb..ebccdc1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1556,18 +1556,13 @@ static const struct file_operations proc_pid_set_comm_operations = {
 static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
 {
 	struct task_struct *task;
-	struct mm_struct *mm;
 	struct file *exe_file;
 
 	task = get_proc_task(d_inode(dentry));
 	if (!task)
 		return -ENOENT;
-	mm = get_task_mm(task);
+	exe_file = get_task_exe_file(task);
 	put_task_struct(task);
-	if (!mm)
-		return -ENOENT;
-	exe_file = get_mm_exe_file(mm);
-	mmput(mm);
 	if (exe_file) {
 		*exe_path = exe_file->f_path;
 		path_get(&exe_file->f_path);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9d85402..f4e639e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2014,6 +2014,7 @@ extern void mm_drop_all_locks(struct mm_struct *mm);
 
 extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
 extern struct file *get_mm_exe_file(struct mm_struct *mm);
+extern struct file *get_task_exe_file(struct task_struct *task);
 
 extern bool may_expand_vm(struct mm_struct *, vm_flags_t, unsigned long npages);
 extern void vm_stat_account(struct mm_struct *, vm_flags_t, long npages);
diff --git a/kernel/fork.c b/kernel/fork.c
index 6fe775c..a4b2384 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -800,6 +800,29 @@ struct file *get_mm_exe_file(struct mm_struct *mm)
 EXPORT_SYMBOL(get_mm_exe_file);
 
 /**
+ * get_task_exe_file - acquire a reference to the task's executable file
+ *
+ * Returns %NULL if task's mm (if any) has no associated executable file or
+ * this is a kernel thread with borrowed mm (see the comment above get_task_mm).
+ * User must release file via fput().
+ */
+struct file *get_task_exe_file(struct task_struct *task)
+{
+	struct file *exe_file = NULL;
+	struct mm_struct *mm;
+
+	task_lock(task);
+	mm = task->mm;
+	if (mm) {
+		if (!(task->flags & PF_KTHREAD))
+			exe_file = get_mm_exe_file(mm);
+	}
+	task_unlock(task);
+	return exe_file;
+}
+EXPORT_SYMBOL(get_task_exe_file);
+
+/**
  * get_task_mm - acquire a reference to the task's mm
  *
  * Returns %NULL if the task has no mm.  Checks PF_KTHREAD (meaning
-- 
1.8.3.1

[PATCHv2 2/2] audit: fix exe_file access in audit_exe_compare

[Mateusz Guzik]

Prior to the change the function would blindly deference mm, exe_file
and exe_file->f_inode, each of which could have been NULL or freed.

Use get_task_exe_file to safely obtain stable exe_file.

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 kernel/audit_watch.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index d6709eb..0d302a8 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -19,6 +19,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/file.h>
 #include <linux/kernel.h>
 #include <linux/audit.h>
 #include <linux/kthread.h>
@@ -544,10 +545,11 @@ int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
 	unsigned long ino;
 	dev_t dev;
 
-	rcu_read_lock();
-	exe_file = rcu_dereference(tsk->mm->exe_file);
+	exe_file = get_task_exe_file(tsk);
+	if (!exe_file)
+		return 0;
 	ino = exe_file->f_inode->i_ino;
 	dev = exe_file->f_inode->i_sb->s_dev;
-	rcu_read_unlock();
+	fput(exe_file);
 	return audit_mark_compare(mark, ino, dev);
 }
-- 
1.8.3.1

Re: [PATCHv2 1/2] mm: introduce get_task_exe_file

[Oleg Nesterov]

On 08/23, Mateusz Guzik wrote:
>
> +struct file *get_task_exe_file(struct task_struct *task)
> +{
> +	struct file *exe_file = NULL;
> +	struct mm_struct *mm;
> +
> +	task_lock(task);
> +	mm = task->mm;
> +	if (mm) {
> +		if (!(task->flags & PF_KTHREAD))
> +			exe_file = get_mm_exe_file(mm);
> +	}
> +	task_unlock(task);
> +	return exe_file;
> +}

I can't believe I am going to comment the coding style but I can't resist ;)

	if (mm && !(task->flags & PF_KTHREAD)))
		exe_file = get_mm_exe_file(mm);

looks a bit simpler to me. But this is purely cosmetic and subjective,
both patches look good to me.

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCHv2 1/2] mm: introduce get_task_exe_file

[Mateusz Guzik]

On Tue, Aug 23, 2016 at 04:48:13PM +0200, Oleg Nesterov wrote:
> On 08/23, Mateusz Guzik wrote:
> >
> > +struct file *get_task_exe_file(struct task_struct *task)
> > +{
> > +	struct file *exe_file = NULL;
> > +	struct mm_struct *mm;
> > +
> > +	task_lock(task);
> > +	mm = task->mm;
> > +	if (mm) {
> > +		if (!(task->flags & PF_KTHREAD))
> > +			exe_file = get_mm_exe_file(mm);
> > +	}
> > +	task_unlock(task);
> > +	return exe_file;
> > +}
> 
> I can't believe I am going to comment the coding style but I can't resist ;)
> 
> 	if (mm && !(task->flags & PF_KTHREAD)))
> 		exe_file = get_mm_exe_file(mm);
> 
> looks a bit simpler to me. But this is purely cosmetic and subjective,
> both patches look good to me.
> 

Actually I did it for some consistency with get_task_mm.

The check can likely be done prior to taking the lock in both functions
and that would clean them up a little bit, but I wanted to avoid nit
picking... :>

> Acked-by: Oleg Nesterov <oleg@redhat.com>
> 

Thanks

-- 
Mateusz Guzik

Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare

[Paul Moore]

On Tue, Aug 23, 2016 at 10:20 AM, Mateusz Guzik <mguzik@redhat.com> wrote:
> audit_exe_compare directly accesses mm->exe_file without making sure the
> object is stable. Fixing it using current primitives results in
> partially duplicating what proc_exe_link is doing.
>
> As such, introduce a trivial helper which can be used in both places and
> fix the func.
>
> Changes since v1:
> * removed an unused 'out' label which crept in
>
> Mateusz Guzik (2):
>   mm: introduce get_task_exe_file
>   audit: fix exe_file access in audit_exe_compare
>
>  fs/proc/base.c       |  7 +------
>  include/linux/mm.h   |  1 +
>  kernel/audit_watch.c |  8 +++++---
>  kernel/fork.c        | 23 +++++++++++++++++++++++
>  4 files changed, 30 insertions(+), 9 deletions(-)

Thanks for doing this.

Both patches look fine to me, does anyone in the mm area have any
objections?  If not, I'll merge these into the audit tree and mark
them for stable.

-- 
paul moore
www.paul-moore.com

Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare

[Richard Guy Briggs]

On 2016-08-23 16:20, Mateusz Guzik wrote:
> audit_exe_compare directly accesses mm->exe_file without making sure the
> object is stable. Fixing it using current primitives results in
> partially duplicating what proc_exe_link is doing.
> 
> As such, introduce a trivial helper which can be used in both places and
> fix the func.
> 
> Changes since v1:
> * removed an unused 'out' label which crept in
> 
> Mateusz Guzik (2):
>   mm: introduce get_task_exe_file
>   audit: fix exe_file access in audit_exe_compare

The task_lock affects a much bigger struct than the mm ref count.  Is
this really necessary?  Is a spin-lock significantly lower cost than a
refcount?  Other than that, this refactorization looks sensible.

Acked-by: Richard Guy Briggs <rgb@redhat.com>

>  fs/proc/base.c       |  7 +------
>  include/linux/mm.h   |  1 +
>  kernel/audit_watch.c |  8 +++++---
>  kernel/fork.c        | 23 +++++++++++++++++++++++
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 
> -- 
> 1.8.3.1
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare

[Mateusz Guzik]

On Tue, Aug 30, 2016 at 02:50:21PM -0400, Richard Guy Briggs wrote:
> On 2016-08-23 16:20, Mateusz Guzik wrote:
> > audit_exe_compare directly accesses mm->exe_file without making sure the
> > object is stable. Fixing it using current primitives results in
> > partially duplicating what proc_exe_link is doing.
> > 
> > As such, introduce a trivial helper which can be used in both places and
> > fix the func.
> > 
> > Changes since v1:
> > * removed an unused 'out' label which crept in
> > 
> > Mateusz Guzik (2):
> >   mm: introduce get_task_exe_file
> >   audit: fix exe_file access in audit_exe_compare
> 
> The task_lock affects a much bigger struct than the mm ref count.  Is
> this really necessary?  Is a spin-lock significantly lower cost than a
> refcount?  Other than that, this refactorization looks sensible.
> 

proc_exe_link was taking the lock anyway to guarantee a stable mm.
I think the helper cleans the code up a little bit and there is
microoptimisation to not play with the refcount.

If audit_exe_compare has guarantees the task wont reach exit_mm, it can
use get_mm_exe_file which means the atomic op would be only on the file
object.

I was under the impression this is the expected behaviour, but your
patch used the task lock to grab mm, so I mimicked it here.

-- 
Mateusz Guzik

Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare

[Paul Moore]

On Mon, Aug 29, 2016 at 6:50 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Aug 23, 2016 at 10:20 AM, Mateusz Guzik <mguzik@redhat.com> wrote:
>> audit_exe_compare directly accesses mm->exe_file without making sure the
>> object is stable. Fixing it using current primitives results in
>> partially duplicating what proc_exe_link is doing.
>>
>> As such, introduce a trivial helper which can be used in both places and
>> fix the func.
>>
>> Changes since v1:
>> * removed an unused 'out' label which crept in
>>
>> Mateusz Guzik (2):
>>   mm: introduce get_task_exe_file
>>   audit: fix exe_file access in audit_exe_compare
>>
>>  fs/proc/base.c       |  7 +------
>>  include/linux/mm.h   |  1 +
>>  kernel/audit_watch.c |  8 +++++---
>>  kernel/fork.c        | 23 +++++++++++++++++++++++
>>  4 files changed, 30 insertions(+), 9 deletions(-)
>
> Thanks for doing this.
>
> Both patches look fine to me, does anyone in the mm area have any
> objections?  If not, I'll merge these into the audit tree and mark
> them for stable.

I just merged these patches into audit#stable-4.8 and have a kernel
building now, as soon as it finishes I'll do some quick sanity tests
and send them off to Linus.

-- 
paul moore
www.paul-moore.com