[RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

Hi guys,

at restore time we would like to have a way for /proc/pid/exe
symlink recovering. So I thought extending prctl might be
a good idea.

Still maybe there some other good and 'right' way to do it,
so I would like to gather opinions.

Please review, thanks!

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

When we do restore we would like to have a way to setup
a former mm_struct::exe_file so that /proc/pid/exe would
point to the original executable file a process had at
checkpoint time.

For this sake PR_SET_MM_EXE_FILE code is introduced.

Note, if mm_struct::exe_file already mapped more than once
we refuse to change anything (which prevents kernel from
potential problems).

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 include/linux/prctl.h |    1 
 kernel/sys.c          |   73 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 1 deletion(-)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================
--- linux-2.6.git.orig/include/linux/prctl.h
+++ linux-2.6.git/include/linux/prctl.h
@@ -118,5 +118,6 @@
 # define PR_SET_MM_ENV_START		10
 # define PR_SET_MM_ENV_END		11
 # define PR_SET_MM_AUXV			12
+# define PR_SET_MM_EXE_FILE		13
 
 #endif /* _LINUX_PRCTL_H */
Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -1701,6 +1701,66 @@ static bool vma_flags_mismatch(struct vm
 		(vma->vm_flags & banned);
 }
 
+/* Expects mm->mmap_sem is read-taken */
+static int prctl_set_mm_exe_file(struct mm_struct *mm,
+				 const void __user *path,
+				 size_t size)
+{
+	struct file *new_exe_file;
+	char *pathbuf;
+	int ret = 0;
+
+	if (size >= PATH_MAX)
+		return -EINVAL;
+
+	/*
+	 * We allow to change only those exe's which
+	 * are not mapped several times. This one
+	 * is early test while mmap_sem is taken.
+	 */
+	if (mm->num_exe_file_vmas > 1)
+		return -EBUSY;
+
+	up_read(&mm->mmap_sem);
+
+	pathbuf = kmalloc(size, GFP_TEMPORARY);
+	if (!pathbuf) {
+		ret = -ENOMEM;
+		goto err_down;
+	}
+
+	if (copy_from_user(pathbuf, path, size)) {
+		kfree(pathbuf);
+		ret = -EFAULT;
+		goto err_down;
+	}
+	pathbuf[size-1] = '\0';
+
+	new_exe_file = open_exec(pathbuf);
+	kfree(pathbuf);
+
+	down_read(&mm->mmap_sem);
+
+	if (IS_ERR(new_exe_file))
+		return PTR_ERR(new_exe_file);
+
+	/*
+	 * We allow to change only those exe's which
+	 * are not mapped several times.
+	 */
+	if (mm->num_exe_file_vmas < 2) {
+		set_mm_exe_file(mm, new_exe_file);
+		ret = 0;
+	} else
+		ret = -EBUSY;
+
+	return ret;
+
+err_down:
+	down_read(&mm->mmap_sem);
+	return ret;
+}
+
 static int prctl_set_mm(int opt, unsigned long addr,
 			unsigned long arg4, unsigned long arg5)
 {
@@ -1709,7 +1769,9 @@ static int prctl_set_mm(int opt, unsigne
 	struct vm_area_struct *vma;
 	int error = 0;
 
-	if (arg5 || (arg4 && opt != PR_SET_MM_AUXV))
+	if (arg5 || (arg4 &&
+		     opt != PR_SET_MM_AUXV &&
+		     opt != PR_SET_MM_EXE_FILE))
 		return -EINVAL;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1837,6 +1899,15 @@ static int prctl_set_mm(int opt, unsigne
 
 		return 0;
 	}
+
+	/*
+	 * This to restore /proc/self/exe link.
+	 */
+	case PR_SET_MM_EXE_FILE:
+		error = prctl_set_mm_exe_file(mm, (const void __user *)addr, arg4);
+		if (error)
+			goto out;
+		break;
 	default:
 		error = -EINVAL;
 		goto out;

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Pavel Emelyanov]

On 02/29/2012 07:16 PM, Cyrill Gorcunov wrote:
> Hi guys,
> 
> at restore time we would like to have a way for /proc/pid/exe
> symlink recovering. So I thought extending prctl might be
> a good idea.
> 
> Still maybe there some other good and 'right' way to do it,
> so I would like to gather opinions.

If using the prctl for this, then taking a given file descriptor
(with fcheck) instead of taking path and opening it in kernel is
better.

> Please review, thanks!
> 
> 	Cyrill

Thanks,
Pavel

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Wed, Feb 29, 2012 at 07:23:07PM +0400, Pavel Emelyanov wrote:
> On 02/29/2012 07:16 PM, Cyrill Gorcunov wrote:
> > Hi guys,
> > 
> > at restore time we would like to have a way for /proc/pid/exe
> > symlink recovering. So I thought extending prctl might be
> > a good idea.
> > 
> > Still maybe there some other good and 'right' way to do it,
> > so I would like to gather opinions.
> 
> If using the prctl for this, then taking a given file descriptor
> (with fcheck) instead of taking path and opening it in kernel is
> better.
> 

Sounds like a plan. Thanks Pavel!

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 02/29, Cyrill Gorcunov wrote:
>
> +static int prctl_set_mm_exe_file(struct mm_struct *mm,
> +				 const void __user *path,
> +				 size_t size)
> +{
> +	struct file *new_exe_file;
> +	char *pathbuf;
> +	int ret = 0;
> +
> +	if (size >= PATH_MAX)
> +		return -EINVAL;
> +
> +	/*
> +	 * We allow to change only those exe's which
> +	 * are not mapped several times. This one
> +	 * is early test while mmap_sem is taken.
> +	 */
> +	if (mm->num_exe_file_vmas > 1)
> +		return -EBUSY;

I don't really understand this check, but it is racy. Another thread
can change ->num_exe_file_vmas right after the check.

> +       up_read(&mm->mmap_sem);

up? I do not see down...

> +	new_exe_file = open_exec(pathbuf);
> +	kfree(pathbuf);
> +
> +	down_read(&mm->mmap_sem);

probably you meant "up" here. OK, I am ignoring ->mmap_sem, I can't
understand what did you really mean ;)

> +	if (IS_ERR(new_exe_file))
> +		return PTR_ERR(new_exe_file);
> +
> +	/*
> +	 * We allow to change only those exe's which
> +	 * are not mapped several times.
> +	 */
> +	if (mm->num_exe_file_vmas < 2) {
> +		set_mm_exe_file(mm, new_exe_file);
> +		ret = 0;
> +	} else
> +		ret = -EBUSY;
> +
> +	return ret;

Both success/EBUSY leak new_exe_file. And I agree with Pavel,
prctl_set_mm_exe_file() should take fd, not filename.

I simply can't understand why set_mm_exe_file() is safe. What
if we race with another thread doing set_mm_exe_file() too?
Or it can race with added_exe_file_vma/removed_exe_file_vma.

And. set_mm_exe_file() sets ->num_exe_file_vmas = 0, this is
simply wrong? It should match the number of VM_EXECUTABLE
vmas.

In short, I do not understand the patch at all. It seems, you
only need to replace mm->exe_file under down_write(mmap_sem)
and nothing else.

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Wed, Feb 29, 2012 at 08:24:00PM +0100, Oleg Nesterov wrote:
> On 02/29, Cyrill Gorcunov wrote:
> >
> > +static int prctl_set_mm_exe_file(struct mm_struct *mm,
> > +				 const void __user *path,
> > +				 size_t size)
> > +{
> > +	struct file *new_exe_file;
> > +	char *pathbuf;
> > +	int ret = 0;
> > +
> > +	if (size >= PATH_MAX)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * We allow to change only those exe's which
> > +	 * are not mapped several times. This one
> > +	 * is early test while mmap_sem is taken.
> > +	 */
> > +	if (mm->num_exe_file_vmas > 1)
> > +		return -EBUSY;
> 
> I don't really understand this check, but it is racy. Another thread
> can change ->num_exe_file_vmas right after the check.
> 
> > +       up_read(&mm->mmap_sem);
> 
> up? I do not see down...

down is taken in calling routine (as pointed in comment on
prctl_set_mm_exe_file), thus I suppose I miss something since
the calling functions which increment/decrement num_exe_file_vmas
(such as mremap) do down_write(mmap_sem) first.

> 
> > +	new_exe_file = open_exec(pathbuf);
> > +	kfree(pathbuf);
> > +
> > +	down_read(&mm->mmap_sem);
> 
> probably you meant "up" here. OK, I am ignoring ->mmap_sem, I can't
> understand what did you really mean ;)
> 

nop, down instead ;)

> > +	if (IS_ERR(new_exe_file))
> > +		return PTR_ERR(new_exe_file);
> > +
> > +	/*
> > +	 * We allow to change only those exe's which
> > +	 * are not mapped several times.
> > +	 */
> > +	if (mm->num_exe_file_vmas < 2) {
> > +		set_mm_exe_file(mm, new_exe_file);
> > +		ret = 0;
> > +	} else
> > +		ret = -EBUSY;
> > +
> > +	return ret;
> 
> Both success/EBUSY leak new_exe_file. And I agree with Pavel,

yeah, will fix, thanks!

> prctl_set_mm_exe_file() should take fd, not filename.
> 

yes, i'll switch to this idea

> I simply can't understand why set_mm_exe_file() is safe. What
> if we race with another thread doing set_mm_exe_file() too?
> Or it can race with added_exe_file_vma/removed_exe_file_vma.

really, Oleg, I don't see race here since this routine is
caller under down_read and I've been releasing mmap_sem for
short time then reacquiring it, and recheck for number of
num_exe_file_vmas. so I presume I miss something obvious
here.

> 
> And. set_mm_exe_file() sets ->num_exe_file_vmas = 0, this is
> simply wrong? It should match the number of VM_EXECUTABLE
> vmas.
> 

yes, it's a nit which sould be fixed. thanks!

> In short, I do not understand the patch at all. It seems, you
> only need to replace mm->exe_file under down_write(mmap_sem)
> and nothing else.

I can't just replace it, I wanted to check it the new symlink
will indeed point to executable (such ceheck btw is done
in open_exec() helper) and I actually wonted to replace
only freshly created executables which didn't have any
remaps on executable VMA (still I might be wrong
here and it's indeed safe just to replace old exe_file).

That's why I posted it as RFC and really appreciate
feedback (so, thanks a lot, Oleg!).

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/01, Cyrill Gorcunov wrote:
>
> On Wed, Feb 29, 2012 at 08:24:00PM +0100, Oleg Nesterov wrote:
> > On 02/29, Cyrill Gorcunov wrote:
> > >
> > > +static int prctl_set_mm_exe_file(struct mm_struct *mm,
> > > +				 const void __user *path,
> > > +				 size_t size)
> > > +{
> > > +	struct file *new_exe_file;
> > > +	char *pathbuf;
> > > +	int ret = 0;
> > > +
> > > +	if (size >= PATH_MAX)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * We allow to change only those exe's which
> > > +	 * are not mapped several times. This one
> > > +	 * is early test while mmap_sem is taken.
> > > +	 */
> > > +	if (mm->num_exe_file_vmas > 1)
> > > +		return -EBUSY;
> >
> > I don't really understand this check, but it is racy. Another thread
> > can change ->num_exe_file_vmas right after the check.
> >
> > > +       up_read(&mm->mmap_sem);
> >
> > up? I do not see down...
>
> down is taken in calling routine (as pointed in comment on
> prctl_set_mm_exe_file),

Ah, indeed, stupid me. Somehow I thought that the caller is sys_prctl().
So it is called by prctl_set_mm() which holds ->mmap_sem for reading.

> thus I suppose I miss something since
> the calling functions which increment/decrement num_exe_file_vmas
> (such as mremap) do down_write(mmap_sem) first.

Yes, so ->num_exe_file_vmas is stable under mmap_sem. But it can
be changed right after up_read(), so I don't underastand this check
anyway.

OK, you recheck this counter later, under mmap_sem.

> > I simply can't understand why set_mm_exe_file() is safe. What
> > if we race with another thread doing set_mm_exe_file() too?
> > Or it can race with added_exe_file_vma/removed_exe_file_vma.
>
> really, Oleg, I don't see race here since this routine is
> caller under down_read and I've been releasing mmap_sem for
> short time then reacquiring it, and recheck for number of
> num_exe_file_vmas. so I presume I miss something obvious
> here.

OK, now that I understand the locking, we can't race with
added_exe_file_vma/removed_exe_file_vma. But I still think we
can race with set_mm_exe_file().

And yes, I think you missed something obvious ;) Suppose that
2 threads call prctl_set_mm(PR_SET_MM_EXE_FILE) at the same
time. Both threads can take ->mmap_sem for reading and do
set_mm_exe_file() at the same time.

> > And. set_mm_exe_file() sets ->num_exe_file_vmas = 0, this is
> > simply wrong? It should match the number of VM_EXECUTABLE
> > vmas.
> >
>
> yes, it's a nit which sould be fixed. thanks!

OK, but then you do not need to check ->num_exe_file_vmas at all?

Except, of course, I think we should fail if this counter is zero.

The changelog says:

	Note, if mm_struct::exe_file already mapped more than once
	we refuse to change anything (which prevents kernel from
	potential problems).

why? which problems?

> > In short, I do not understand the patch at all. It seems, you
> > only need to replace mm->exe_file under down_write(mmap_sem)
> > and nothing else.
>
> I can't just replace it, I wanted to check it the new symlink
> will indeed point to executable

I meant I see no reason to play with num_exe_file_vmas, you only
need to replace ->exe_file.

As for additional checks, I have no opinion. I don't know if it
really makes sense to verify it is executable.

But, hmm. There is another problem with your patch. open_exec()
does deny_write_access(), and I do not see who does the necessary
allow_write_access().

> and I actually wonted to replace
> only freshly created executables which didn't have any
> remaps on executable VMA

I don't really understand what do you mean.

In any case, PR_SET_MM_EXE_FILE is cheating. The new file doesn't
match ->vm_file of VM_EXECUTABLE vmas. And it can be writable.

But why do we require num_exe_file_vmas == 1?

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Thu, Mar 01, 2012 at 07:06:16PM +0100, Oleg Nesterov wrote:
...
> 
> OK, now that I understand the locking, we can't race with
> added_exe_file_vma/removed_exe_file_vma. But I still think we
> can race with set_mm_exe_file().
> 
> And yes, I think you missed something obvious ;) Suppose that
> 2 threads call prctl_set_mm(PR_SET_MM_EXE_FILE) at the same
> time. Both threads can take ->mmap_sem for reading and do
> set_mm_exe_file() at the same time.
> 

Hi Oleg, thanks a lot for comments! Today I started remakeing
the patch and found one obvious problem -- the mmap_sem should
be taken for write not read, since otherwise someone might get
wrong reference when we're chenging this symlink. So sure, this
was incorrect in first place ;)

> > > And. set_mm_exe_file() sets ->num_exe_file_vmas = 0, this is
> > > simply wrong? It should match the number of VM_EXECUTABLE
> > > vmas.
> > >
> >
> > yes, it's a nit which sould be fixed. thanks!
> 
> OK, but then you do not need to check ->num_exe_file_vmas at all?
> 
> Except, of course, I think we should fail if this counter is zero.
> 
> The changelog says:
> 
> 	Note, if mm_struct::exe_file already mapped more than once
> 	we refuse to change anything (which prevents kernel from
> 	potential problems).
> 
> why? which problems?
> 

I've some gut feeling that if I have num_exe_file_vmas more than
one I might broke something if I replace exe_file (actually I'm
checking this, was interrupted with other duties).

> > > In short, I do not understand the patch at all. It seems, you
> > > only need to replace mm->exe_file under down_write(mmap_sem)
> > > and nothing else.
> >
> > I can't just replace it, I wanted to check it the new symlink
> > will indeed point to executable
> 
> I meant I see no reason to play with num_exe_file_vmas, you only
> need to replace ->exe_file.

Yes, I tend to think the same (but as I mentioned, I'm checking
if this really wont break anything).

So, Oleg, basically the new version will use opened fd in form
like (note, I'll recheck for refs and locks more so this is
a draft version to point idea)

static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
{
	struct files_struct *files;
	struct file *file;
	int ret;

	/*
	 * Make sure if someone is trying to obtain
	 * the existing exe_file it will not get
	 * results until we've finished.
	 */
	down_write(&mm->mmap_sem);

	files = get_files_struct(current);
	if (!files)
		return -EINVAL;

	spin_lock(&files->file_lock);

	file = fcheck_files(files, fd);
	if (!file) {
		ret = -EBADFD;
		goto out_unlock;
	}
	get_file(file);
	spin_unlock(&files->file_lock);

	if (mm->num_exe_file_vmas)
		removed_exe_file_vma(mm);

-> This are two lines I'm doubt about
->	set_mm_exe_file(mm, file);
->	added_exe_file_vma(mm);

out_unlock:
	put_files_struct(files);
out:
	up_write(&mm->mmap_sem);
	return ret;
}

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/01, Cyrill Gorcunov wrote:
>
> So, Oleg, basically the new version will use opened fd in form
> like (note, I'll recheck for refs and locks more so this is
> a draft version to point idea)
>
> static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
> {
> 	struct files_struct *files;
> 	struct file *file;
> 	int ret;
>
> 	/*
> 	 * Make sure if someone is trying to obtain
> 	 * the existing exe_file it will not get
> 	 * results until we've finished.
> 	 */
> 	down_write(&mm->mmap_sem);
>
> 	files = get_files_struct(current);
> 	if (!files)
> 		return -EINVAL;
>
> 	spin_lock(&files->file_lock);
>
> 	file = fcheck_files(files, fd);
> 	if (!file) {
> 		ret = -EBADFD;
> 		goto out_unlock;
> 	}
> 	get_file(file);
> 	spin_unlock(&files->file_lock);
>
> 	if (mm->num_exe_file_vmas)
> 		removed_exe_file_vma(mm);

Still can't understand. I think you need:

	file = fget(fd);
	if (!file)
		return -EBADF;

	down_write(&mm->mmap_sem);
	if (mm->num_exe_file_vmas) {
		fput(mm->exe_file);
		mm->exe_file = file;
		file = NULL;
	}
	up_write(&mm->mmap_sem);

	if (!file)
		return 0;

	fput(file);
	return -ESOMETHING;

and that is all.

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Thu, Mar 01, 2012 at 08:41:20PM +0100, Oleg Nesterov wrote:
...
> 
> Still can't understand. I think you need:
> 
> 	file = fget(fd);
> 	if (!file)
> 		return -EBADF;
> 
> 	down_write(&mm->mmap_sem);
> 	if (mm->num_exe_file_vmas) {
> 		fput(mm->exe_file);
> 		mm->exe_file = file;
> 		file = NULL;
> 	}
> 	up_write(&mm->mmap_sem);
> 
> 	if (!file)
> 		return 0;
> 
> 	fput(file);
> 	return -ESOMETHING;
> 
> and that is all.

This breaks overall logic of num_exe_file_vmas.
What the point to have it at all then? I mean,
if there several executable sections in elf file,
once loader finish its work we will have
num_exe_file_vmas more than 1.

Then the process calls for prctl and replaces
own exe_file (I'm talking about possible scenario
since for our own tool we know that there will be
only one .text section and we're more-less safe
in replacing own exe_file, but this interface
will be available for everyone who has c/r config
entry turned on, so I'm trying to find which
negative impact this feature might have, call me
paranoid), so once process has replaced own exe_file
to something else the code which depends on
num_exe_file_vmas become broken.

May not we have a scenario when removed_exe_file_vma
is be called somewhere else later, once this prctl
finished its work? That's what I fear of.

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Thu, Mar 01, 2012 at 08:41:20PM +0100, Oleg Nesterov wrote:
...
> Still can't understand. I think you need:
> 
> 	file = fget(fd);
> 	if (!file)
> 		return -EBADF;
> 
> 	down_write(&mm->mmap_sem);
> 	if (mm->num_exe_file_vmas) {
> 		fput(mm->exe_file);
> 		mm->exe_file = file;
> 		file = NULL;
> 	}
> 	up_write(&mm->mmap_sem);
> 
> 	if (!file)
> 		return 0;
> 
> 	fput(file);
> 	return -ESOMETHING;
> 
> and that is all.

Hi Oleg, sure you were right. I even think testing for
num_exe_file_vmas is not needed, since until real
executable VMA is read and mapped it might remain
zero. So I guess something like below should work.

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: c/r: prctl: Add ability to set new mm_struct::exe_file v2

When we do restore we would like to have a way to setup
a former mm_struct::exe_file so that /proc/pid/exe would
point to the original executable file a process had at
checkpoint time.

For this sake PR_SET_MM_EXE_FILE code is introduced.

This feature is available iif CONFIG_CHECKPOINT_RESTORE is set.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Kees Cook <keescook@chromium.org>
CC: Tejun Heo <tj@kernel.org>
---
 include/linux/prctl.h |    1 +
 kernel/sys.c          |   24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================
--- linux-2.6.git.orig/include/linux/prctl.h
+++ linux-2.6.git/include/linux/prctl.h
@@ -118,5 +118,6 @@
 # define PR_SET_MM_ENV_START		10
 # define PR_SET_MM_ENV_END		11
 # define PR_SET_MM_AUXV			12
+# define PR_SET_MM_EXE_FILE		13
 
 #endif /* _LINUX_PRCTL_H */
Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -36,6 +36,7 @@
 #include <linux/personality.h>
 #include <linux/ptrace.h>
 #include <linux/fs_struct.h>
+#include <linux/file.h>
 #include <linux/gfp.h>
 #include <linux/syscore_ops.h>
 #include <linux/version.h>
@@ -1701,6 +1702,25 @@ static bool vma_flags_mismatch(struct vm
 		(vma->vm_flags & banned);
 }
 
+static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
+{
+	struct file *new_exe_file, *old_exe_file;
+
+	new_exe_file = fget(fd);
+	if (!new_exe_file)
+		return -EBADF;
+
+	down_write(&mm->mmap_sem);
+	old_exe_file = mm->exe_file;
+	mm->exe_file = new_exe_file;
+	up_write(&mm->mmap_sem);
+
+	if (old_exe_file)
+		fput(old_exe_file);
+
+	return 0;
+}
+
 static int prctl_set_mm(int opt, unsigned long addr,
 			unsigned long arg4, unsigned long arg5)
 {
@@ -1715,6 +1735,9 @@ static int prctl_set_mm(int opt, unsigne
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (opt == PR_SET_MM_EXE_FILE)
+		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
+
 	if (addr >= TASK_SIZE)
 		return -EINVAL;
 
@@ -1837,6 +1860,7 @@ static int prctl_set_mm(int opt, unsigne
 
 		return 0;
 	}
+
 	default:
 		error = -EINVAL;
 		goto out;

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/02, Cyrill Gorcunov wrote:
>
> On Thu, Mar 01, 2012 at 08:41:20PM +0100, Oleg Nesterov wrote:
> ...
> >
> > Still can't understand. I think you need:
> >
> > 	file = fget(fd);
> > 	if (!file)
> > 		return -EBADF;
> >
> > 	down_write(&mm->mmap_sem);
> > 	if (mm->num_exe_file_vmas) {
> > 		fput(mm->exe_file);
> > 		mm->exe_file = file;
> > 		file = NULL;
> > 	}
> > 	up_write(&mm->mmap_sem);
> >
> > 	if (!file)
> > 		return 0;
> >
> > 	fput(file);
> > 	return -ESOMETHING;
> >
> > and that is all.
>
> This breaks overall logic of num_exe_file_vmas.

Why? In my opinion, your patch breaks the logic ;)

> What the point to have it at all then?

I think it should die. I already suggested to do

	- struct file *exe_file;
	+ struct path *exe_path;

and kill this counter, but this is off-topic.

> I mean,
> if there several executable sections in elf file,
> once loader finish its work we will have
> num_exe_file_vmas more than 1.

Yes. And?

> Then the process calls for prctl and replaces
> own exe_file (I'm talking about possible scenario
> since for our own tool we know that there will be
> only one .text section and we're more-less safe
> in replacing own exe_file,

confused. I do not see the "num_exe_file_vmas == 1" check in the
last version. (yes, I think it is not needed).

OTOH, you should check num_exe_file_vmas != 0, otherwise you break
the current logic.

> but this interface
> will be available for everyone who has c/r config
> entry turned on,

Yes, and thus it should work in any case.

> so I'm trying to find which
> negative impact this feature might have,

If you find something negative - please explain and correct me ;)

Your message starts with "This breaks overall logic" without any
explanation.

> so once process has replaced own exe_file
> to something else the code which depends on
> num_exe_file_vmas become broken.

Again, why???

> May not we have a scenario when removed_exe_file_vma
> is be called somewhere else later, once this prctl
> finished its work? That's what I fear of.

Of course, removed_exe_file_vma() or added_exe_file_vma() can
be called after prctl(). And we should keep the current logic:
mm->exe_file exists until num_exe_file_vmas != 0.

To simplify, currently we have:

	- num_exe_file_vmas is equal to the number of
	  MAP_EXECUTABLE vmas

	- (num_exe_file_vmas != 0) <=> (exe_file != NULL)

You should keep this. Or you should change the rules and explain
why you are doing this.

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/02, Cyrill Gorcunov wrote:
>
> On Thu, Mar 01, 2012 at 08:41:20PM +0100, Oleg Nesterov wrote:
> ...
> > Still can't understand. I think you need:
> >
> > 	file = fget(fd);
> > 	if (!file)
> > 		return -EBADF;
> >
> > 	down_write(&mm->mmap_sem);
> > 	if (mm->num_exe_file_vmas) {
> > 		fput(mm->exe_file);
> > 		mm->exe_file = file;
> > 		file = NULL;
> > 	}
> > 	up_write(&mm->mmap_sem);
> >
> > 	if (!file)
> > 		return 0;
> >
> > 	fput(file);
> > 	return -ESOMETHING;
> >
> > and that is all.
>
> Hi Oleg, sure you were right. I even think testing for
> num_exe_file_vmas is not needed, since until real
> executable VMA is read and mapped it might remain
> zero. So I guess something like below should work.

No, you need to check num_exe_file_vmas != 0.

> +static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
> +{
> +	struct file *new_exe_file, *old_exe_file;
> +
> +	new_exe_file = fget(fd);
> +	if (!new_exe_file)
> +		return -EBADF;
> +
> +	down_write(&mm->mmap_sem);
> +	old_exe_file = mm->exe_file;
> +	mm->exe_file = new_exe_file;
> +	up_write(&mm->mmap_sem);

This changes the current rule: mm->exe_file goes away once
num_exe_file_vmas becomes zero.

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Fri, Mar 02, 2012 at 04:26:22PM +0100, Oleg Nesterov wrote:
...
> > Hi Oleg, sure you were right. I even think testing for
> > num_exe_file_vmas is not needed, since until real
> > executable VMA is read and mapped it might remain
> > zero. So I guess something like below should work.
> 
> No, you need to check num_exe_file_vmas != 0.
> 
> > +static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
> > +{
> > +	struct file *new_exe_file, *old_exe_file;
> > +
> > +	new_exe_file = fget(fd);
> > +	if (!new_exe_file)
> > +		return -EBADF;
> > +
> > +	down_write(&mm->mmap_sem);
> > +	old_exe_file = mm->exe_file;
> > +	mm->exe_file = new_exe_file;
> > +	up_write(&mm->mmap_sem);
> 
> This changes the current rule: mm->exe_file goes away once
> num_exe_file_vmas becomes zero.

Yes, Oleg, you're right of course! The set_mm_exe_file confused
me a bit (I didn't reply to your previous emails simply because
you were asking me to prove what exactly is broken when num_exe_file_vmas
greater 2 and we reassign new exe_file, and I didn't know what to reply
since I knew I was wrong in first assumption).

As a conclusion -- sure I need to check for num_exe_file_vmas to
not break current logic, thanks!

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Fri, Mar 02, 2012 at 08:12:47PM +0400, Cyrill Gorcunov wrote:
...
> 
> Yes, Oleg, you're right of course! The set_mm_exe_file confused
> me a bit (I didn't reply to your previous emails simply because
> you were asking me to prove what exactly is broken when num_exe_file_vmas
> greater 2 and we reassign new exe_file, and I didn't know what to reply
> since I knew I was wrong in first assumption).
> 
> As a conclusion -- sure I need to check for num_exe_file_vmas to
> not break current logic, thanks!
> 

Hi Oleg,

i suppose the below one should do the trick. I mostly concerned
at moment if I added all permission tests needed.

	Cyrill
---
c/r: prctl: Add ability to set new mm_struct::exe_file v2

From: Cyrill Gorcunov <gorcunov@openvz.org>

When we do restore we would like to have a way to setup
a former mm_struct::exe_file so that /proc/pid/exe would
point to the original executable file a process had at
checkpoint time.

For this the PR_SET_MM_EXE_FILE code is introduced.

This feature is available iif CONFIG_CHECKPOINT_RESTORE is set.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Kees Cook <keescook@chromium.org>
CC: Tejun Heo <tj@kernel.org>
---
 include/linux/prctl.h |    1 
 kernel/sys.c          |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================
--- linux-2.6.git.orig/include/linux/prctl.h
+++ linux-2.6.git/include/linux/prctl.h
@@ -118,5 +118,6 @@
 # define PR_SET_MM_ENV_START		10
 # define PR_SET_MM_ENV_END		11
 # define PR_SET_MM_AUXV			12
+# define PR_SET_MM_EXE_FILE		13
 
 #endif /* _LINUX_PRCTL_H */
Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -36,6 +36,8 @@
 #include <linux/personality.h>
 #include <linux/ptrace.h>
 #include <linux/fs_struct.h>
+#include <linux/file.h>
+#include <linux/mount.h>
 #include <linux/gfp.h>
 #include <linux/syscore_ops.h>
 #include <linux/version.h>
@@ -1701,6 +1703,56 @@ static bool vma_flags_mismatch(struct vm
 		(vma->vm_flags & banned);
 }
 
+static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
+{
+	struct file *exe_file;
+	struct dentry *dentry;
+	int err;
+
+	exe_file = fget(fd);
+	if (!exe_file)
+		return -EBADF;
+
+	dentry = exe_file->f_path.dentry;
+
+	/*
+	 * Permissions should be the same as if the
+	 * file has being opened by the kernel for
+	 * execution.
+	 */
+	err = -EACCES;
+	if (!S_ISREG(dentry->d_inode->i_mode)	||
+	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+		goto exit;
+
+	if ((exe_file->f_flags & O_ACCMODE) != O_RDONLY)
+		goto exit;
+
+	err = inode_permission(dentry->d_inode, MAY_EXEC);
+	if (err)
+		goto exit;
+
+	down_write(&mm->mmap_sem);
+	if (mm->num_exe_file_vmas) {
+
+		err = deny_write_access(exe_file);
+		if (err) {
+			up_write(&mm->mmap_sem);
+			goto exit;
+		}
+
+		fput(mm->exe_file);
+		mm->exe_file = exe_file;
+	} else {
+		fput(exe_file);
+		err = -ENXIO;
+	}
+	up_write(&mm->mmap_sem);
+
+exit:
+	return err;
+}
+
 static int prctl_set_mm(int opt, unsigned long addr,
 			unsigned long arg4, unsigned long arg5)
 {
@@ -1715,6 +1767,9 @@ static int prctl_set_mm(int opt, unsigne
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (opt == PR_SET_MM_EXE_FILE)
+		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
+
 	if (addr >= TASK_SIZE)
 		return -EINVAL;
 
@@ -1837,6 +1892,7 @@ static int prctl_set_mm(int opt, unsigne
 
 		return 0;
 	}
+
 	default:
 		error = -EINVAL;
 		goto out;

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/04, Cyrill Gorcunov wrote:
>
> +static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
> +{
> +	struct file *exe_file;
> +	struct dentry *dentry;
> +	int err;
> +
> +	exe_file = fget(fd);
> +	if (!exe_file)
> +		return -EBADF;
> +
> +	dentry = exe_file->f_path.dentry;
> +
> +	/*
> +	 * Permissions should be the same as if the
> +	 * file has being opened by the kernel for
> +	 * execution.
> +	 */

Why?

> +	err = -EACCES;
> +	if (!S_ISREG(dentry->d_inode->i_mode)	||
> +	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +		goto exit;
> +
> +	if ((exe_file->f_flags & O_ACCMODE) != O_RDONLY)
> +		goto exit;
> +
> +	err = inode_permission(dentry->d_inode, MAY_EXEC);
> +	if (err)
> +		goto exit;

OK, I won't argue, probably this makes sense to make sure that
admin can't get a heart attack looking at /proc/pid/exe.

But the O_RDONLY check looks strange. We are not going to write
to this file, we only set the name (and that is why I think it
should be mm->exe_path). What is the point to check that the file
was opened without FMODE_WRITE? Even if there were any security
risk the apllication can open this file again with the different
flags.

And btw this check is redundant anyway because you do
deny_write_access() below. However, this deny_write_access() looks
wrong:

> +	down_write(&mm->mmap_sem);
> +	if (mm->num_exe_file_vmas) {
> +
> +		err = deny_write_access(exe_file);

And who does allow_write_access() ?

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/05, Oleg Nesterov wrote:
>
> On 03/04, Cyrill Gorcunov wrote:
> >
> > +	err = -EACCES;
> > +	if (!S_ISREG(dentry->d_inode->i_mode)	||
> > +	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> > +		goto exit;
> > +
> > +	if ((exe_file->f_flags & O_ACCMODE) != O_RDONLY)
> > +		goto exit;
> > +
> > +	err = inode_permission(dentry->d_inode, MAY_EXEC);
> > +	if (err)
> > +		goto exit;
>
> OK, I won't argue, probably this makes sense to make sure that
> admin can't get a heart attack looking at /proc/pid/exe.
>
> But the O_RDONLY check looks strange. We are not going to write
> to this file, we only set the name (and that is why I think it
> should be mm->exe_path). What is the point to check that the file
> was opened without FMODE_WRITE? Even if there were any security
> risk the apllication can open this file again with the different
> flags.

Seriously, I think we should cleanup this before c/r adds more
ugliness. I'll try to make the patch today.

And with all these checks I am no longer sure that fd is better
than filename ;)

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Mon, Mar 05, 2012 at 03:26:55PM +0100, Oleg Nesterov wrote:
> > OK, I won't argue, probably this makes sense to make sure that
> > admin can't get a heart attack looking at /proc/pid/exe.
> >
> > But the O_RDONLY check looks strange. We are not going to write
> > to this file, we only set the name (and that is why I think it
> > should be mm->exe_path). What is the point to check that the file
> > was opened without FMODE_WRITE? Even if there were any security
> > risk the apllication can open this file again with the different
> > flags.
>

Hi Oleg!

Replying to both your email -- I wanted to be as close to open_exec
as possible. This prctl does cheat the kernel but with this tests
the cheating should be minimized (it's almost the same as open_exec
does).

> Seriously, I think we should cleanup this before c/r adds more
> ugliness. I'll try to make the patch today.
> 

Cleanup what? If you mean this patch -- just point me what
should I do.

> And with all these checks I am no longer sure that fd is better
> than filename ;)

This security tests was a reason why I've used open_exec in
first version of the patch (and I still would prefer to
have open_exec here instead of fd).

As to allow-write-access -- it should be cleaned once process
finished, no?

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/05, Cyrill Gorcunov wrote:
>
> On Mon, Mar 05, 2012 at 03:26:55PM +0100, Oleg Nesterov wrote:
> > > OK, I won't argue, probably this makes sense to make sure that
> > > admin can't get a heart attack looking at /proc/pid/exe.
> > >
> > > But the O_RDONLY check looks strange. We are not going to write
> > > to this file, we only set the name (and that is why I think it
> > > should be mm->exe_path). What is the point to check that the file
> > > was opened without FMODE_WRITE? Even if there were any security
> > > risk the apllication can open this file again with the different
> > > flags.
> >
>
> Hi Oleg!
>
> Replying to both your email -- I wanted to be as close to open_exec
> as possible.

I see. But open_exec() is different, it returns the file we are going
to read/mmap. PR_SET_MM_EXE_FILE is different, I think O_RDONLY buys
nothing and looks confusing.

Anyway, as I said I won't argue.

> This prctl does cheat the kernel

Yep. Except, well, it cheats the user-space.

> but with this tests
> the cheating should be minimized (it's almost the same as open_exec
> does).

I don't reallt understand "minimized" ;) With this tests
proc/pid/exe can't look "obviously wrong", I agree. But that is all.

> > Seriously, I think we should cleanup this before c/r adds more
> > ugliness. I'll try to make the patch today.
>
> Cleanup what? If you mean this patch -- just point me what
> should I do.

I just sent the patch, "turn mm->exe_file into mm->exe_path"

> > And with all these checks I am no longer sure that fd is better
> > than filename ;)
>
> This security tests was a reason why I've used open_exec in
> first version of the patch

Yes, but me and Pavel forced you to use "int fd" ;)

> (and I still would prefer to
> have open_exec here instead of fd).

With the patch I sent "struct file *" is not needed at all.
I think prctl() can use user_path().

> As to allow-write-access -- it should be cleaned once process
> finished, no?

Exactly! And who will increment ->i_writecount? Nobody, that is
the problem.

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Mon, Mar 05, 2012 at 04:40:29PM +0100, Oleg Nesterov wrote:
> >
> > Hi Oleg!
> >
> > Replying to both your email -- I wanted to be as close to open_exec
> > as possible.
> 
> I see. But open_exec() is different, it returns the file we are going
> to read/mmap. PR_SET_MM_EXE_FILE is different, I think O_RDONLY buys
> nothing and looks confusing.
> 
> Anyway, as I said I won't argue.
> 

OK

> > This prctl does cheat the kernel
> 
> Yep. Except, well, it cheats the user-space.

And kernel as well, since this link is not anymore
the same as it was, and memory read from this file
(at execution time) will keep data irrelevant to what
the new symlink points to (but it's exactly by design).

> 
> > but with this tests
> > the cheating should be minimized (it's almost the same as open_exec
> > does).
> 
> I don't reallt understand "minimized" ;) With this tests
> proc/pid/exe can't look "obviously wrong", I agree. But that is all.

call it "minimized wrong" ;)

> 
> > > Seriously, I think we should cleanup this before c/r adds more
> > > ugliness. I'll try to make the patch today.
> >
> > Cleanup what? If you mean this patch -- just point me what
> > should I do.
> 
> I just sent the patch, "turn mm->exe_file into mm->exe_path"
> 

Cool, just got it, thanks!

> > > And with all these checks I am no longer sure that fd is better
> > > than filename ;)
> >
> > This security tests was a reason why I've used open_exec in
> > first version of the patch
> 
> Yes, but me and Pavel forced you to use "int fd" ;)
> 

With plain fd it doesn't require kernel to allocate temp
memory and copy path from user space, this is a positive
effect.

> > (and I still would prefer to
> > have open_exec here instead of fd).
> 
> With the patch I sent "struct file *" is not needed at all.
> I think prctl() can use user_path().

OK!

> 
> > As to allow-write-access -- it should be cleaned once process
> > finished, no?
> 
> Exactly! And who will increment ->i_writecount? Nobody, that is
> the problem.

I see, thanks!

	Cyrill

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov]

On 03/05, Cyrill Gorcunov wrote:
>
> On Mon, Mar 05, 2012 at 04:40:29PM +0100, Oleg Nesterov wrote:
>
> > > This prctl does cheat the kernel
> >
> > Yep. Except, well, it cheats the user-space.
>
> And kernel as well, since this link is not anymore
> the same as it was, and memory read from this file
> (at execution time) will keep data irrelevant to what
> the new symlink points to (but it's exactly by design).

The kernel will never use this file for reading. It is only
used as a placeholder for ->f_path shown in proc.

User-space can be confused, yes, this is by design.

Oleg.

Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Cyrill Gorcunov]

On Mon, Mar 05, 2012 at 05:31:30PM +0100, Oleg Nesterov wrote:
> 
> User-space can be confused, yes, this is by design.
> 

OK ;)

	Cyrill