c/r: prctl: Add ability to set new mm_struct::exe_file v3

Hi Oleg, could you please take a look once you get a minute (no urgency).

From: Cyrill Gorcunov <[email protected]>
Subject: c/r: prctl: Add ability to set new mm_struct::exe_file v3

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 option takes a file descriptor which will be set as new /proc/$pid/exe symlink.

This feature is available iif CONFIG_CHECKPOINT_RESTORE is set.

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Oleg Nesterov <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Pavel Emelyanov <[email protected]>

CC Kees Cook <[email protected]>

CC Tejun Heo <[email protected]>

---
 include/linux/prctl.h |    1 +
 kernel/sys.c          |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================

include/linux/prctl.h +1 -0

@@ -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
===================================================================

kernel/sys.c +50 -0

@@ -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,50 @@ 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;
+
+	/*
+	 * Because the original mm->exe_file
+	 * points to executable file, make sure
+	 * this one is executable as well to not
+	 * break "big" picture and proc/pid/exe
+	 * symlink will be still pointing to
+	 * executable one.
+	 */
+	err = -EACCES;
+	if (!S_ISREG(dentry->d_inode->i_mode)	||
+	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+		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) {
+		fput(mm->exe_file);
+		mm->exe_file = exe_file;
+		exe_file = NULL;
+	} else
+		set_mm_exe_file(mm, exe_file);
+	up_write(&mm->mmap_sem);
+
+exit:
+	if (exe_file)
+		fput(exe_file);
+	return err;
+}
+
 static int prctl_set_mm(int opt, unsigned long addr,
 			unsigned long arg4, unsigned long arg5)
 {

@@ -1715,6 +1761,9 @@ static int prctl_set_mm(int opt, unsigne
 	if (!capable(CAP_SYS_RESOURCE))
 		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 +1886,7 @@ static int prctl_set_mm(int opt, unsigne
 
 		return 0;
 	}
+
 	default:
 		error = -EINVAL;
 		goto out;

Quoted 2 lines · Cyrill Gorcunov

Hi Oleg, could you please take a look once you get a minute (no urgency).

Add Matt. I won't touch the text below to keep the patch intact.

With this change

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

I simply do not understand what mm->num_exe_file_vmas means after PR_SET_MM_EXE_FILE.

I think that you should do

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

to keep the current "mm->exe_file goes away after the final unmap(MAP_EXECUTABLE)" logic.

OK, may be this doesn't work in c/r case because you are actually going to remove the old mappings? But in this case the new exe_file will go away anyway, afaics PR_SET_MM_EXE_FILE is called when you still have the old mappings.

And I don't think the unconditional

	down_write(&mm->mmap_sem);
	set_mm_exe_file(mm, exe_file);
	up_write(&mm->mmap_sem);

is 100% right, this clears ->num_exe_file_vmas. This means that (if you still have the old mapping) the new exe_file can go away after added_exe_file_vma() + removed_exe_file_vma(). Normally this should happen, but afaics this is possible. Note that even, say, mprotect() can trigger added_exe_file_vma().

May be we can do something like

	down_write(&mm->mmap_sem);
	set_mm_exe_file(mm, exe_file);
	// we are cheating anyway, make sure it can never == 0
	// if we have the "old" VM_EXECUTABLE vmas.
	mm->num_exe_file_vmas = LONG_MAX;
	up_write(&mm->mmap_sem);

I dunno. Matt, could you help?

Quoted 10 lines · Oleg Nesterov

I think that you should do

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

Quoted 7 lines

to keep the current "mm->exe_file goes away after the final unmap(MAP_EXECUTABLE)" logic.

OK, may be this doesn't work in c/r case because you are actually going to remove the old mappings? But in this case the new exe_file will go away anyway, afaics PR_SET_MM_EXE_FILE is called when you still have the old mappings.

Yes, exactly, I need to remove old mappings first (because VMAs we're about to restore may intersect with current map the host program has). And yes, once they all are removed I don't have /proc/pid/exe anymore. That's why I need num_exe_file_vmas == 0 case.

Quoted 6 lines

And I don't think the unconditional

	down_write(&mm->mmap_sem);
	set_mm_exe_file(mm, exe_file);
	up_write(&mm->mmap_sem);

Quoted 6 lines

is 100% right, this clears ->num_exe_file_vmas. This means that (if you still have the old mapping) the new exe_file can go away after added_exe_file_vma() + removed_exe_file_vma(). Normally this should happen, but afaics this is possible. Note that even, say, mprotect() can trigger added_exe_file_vma().

Wait, Oleg, I'm confused, in case if there *is* exitsting VM_EXECUTABLEs then we jump into first banch and simply replace old exe_file.

Quoted 6 lines

Yes, exactly, I need to remove old mappings first (because VMAs we're about to restore may intersect with current map the host program has). And yes, once they all are removed I don't have /proc/pid/exe anymore. That's why I need num_exe_file_vmas == 0 case.

OK, in this case PR_SET_MM_EXE_FILE should probably fail if mm->num_exe_file_vmas != 0 ? This way it would be more or less consistent or at least understandable. Just we add the new special case: num_exe_file_vmas == 0 but exe_file != NULL because c/r people are crazy.

Quoted 13 lines

Quoted 12 lines

And I don't think the unconditional

	down_write(&mm->mmap_sem);
	set_mm_exe_file(mm, exe_file);
	up_write(&mm->mmap_sem);

is 100% right, this clears ->num_exe_file_vmas. This means that (if you still have the old mapping) the new exe_file can go away after added_exe_file_vma() + removed_exe_file_vma(). Normally this should happen, but afaics this is possible. Note that even, say, mprotect() can trigger added_exe_file_vma().

Quoted 2 lines

Wait, Oleg, I'm confused, in case if there *is* exitsting VM_EXECUTABLEs then we jump into first banch and simply replace old exe_file.

Yes. And then later you remove the old mapping (which do not match the new file anyway) and the new exe_file goes away. Unlikely you want this.

On Thu, Mar 08, 2012 at 08:05:34PM +0100, Oleg Nesterov wrote:
...

Quoted 7 lines

Quoted 6 lines

Yes, exactly, I need to remove old mappings first (because VMAs we're about to restore may intersect with current map the host program has). And yes, once they all are removed I don't have /proc/pid/exe anymore. That's why I need num_exe_file_vmas == 0 case.

Quoted 6 lines

OK, in this case PR_SET_MM_EXE_FILE should probably fail if mm->num_exe_file_vmas != 0 ? This way it would be more or less consistent or at least understandable. Just we add the new special case: num_exe_file_vmas == 0 but exe_file != NULL because c/r people are crazy.

Sure, I can drop num_exe_file_vmas != 0 case and refuse to setup new exe symlink if there some VM_EXECUTABLE remains unmapped. Sounds good?

Quoted 4 lines

Sure, I can drop num_exe_file_vmas != 0 case and refuse to setup new exe symlink if there some VM_EXECUTABLE remains unmapped. Sounds good?

Personally I like this. This is simple and _understable_, even if ->num_exe_file_vmas has no meaning after PR_SET_MM_EXE.

But please-please document the new special case in the changelog.

But please-please document the new special case in the changelog.

kernel/sys.c +20 -0

+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;
+
+	/*
+	 * Because the original mm->exe_file
+	 * points to executable file, make sure
+	 * this one is executable as well to not
+	 * break an overall picture.
+	 */
+	err = -EACCES;
+	if (!S_ISREG(dentry->d_inode->i_mode)	||

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1735

Quoted 4 lines

kernel/sys.c +4 -0

+
+	/*
+	 * Setting new mm::exe_file is only allowed
+	 * when no executable VMAs left. This is

                   ^^^^^^^^^^

Perhaps this is just me, but imho "executable" is not clear enough. I'd suggest VM_EXECUTABLE to avoid the confusion with VM_EXEC.

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1742

Quoted 8 lines

kernel/sys.c +7 -0

+	 * special C/R case when a restored program
+	 * need to change own /proc/$pid/exe symlink.
+	 * After this call mm::num_exe_file_vmas become
+	 * meaningless.
+	 */
+	down_write(&mm->mmap_sem);
+	if (mm->num_exe_file_vmas == 0) {

You can check this at the very start lockless and simplify the code. Once it is zero, it can never grow (or we have a bug anyway).

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1751

Quoted 10 lines

kernel/sys.c +9 -0

+		set_mm_exe_file(mm, exe_file);
+		exe_file = NULL;
+	} else
+		err = -EBUSY;
+	up_write(&mm->mmap_sem);
+
+exit:
+	if (exe_file)
+		fput(exe_file);

This doesn't look correct, you need fput() in any case. set_mm_exe_file() does another get_file().

yeah, thanks, will update.

Quoted 3 lines · Cyrill Gorcunov

yeah, thanks, will update.

This one should fit all requirements I hope.

kernel/sys.c +30 -0

+static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
+{
+	struct file *exe_file;
+	struct dentry *dentry;
+	int err;
+
+	if (mm->num_exe_file_vmas)
+		return -EBUSY;
+
+	exe_file = fget(fd);
+	if (!exe_file)
+		return -EBADF;
+
+	dentry = exe_file->f_path.dentry;
+
+	/*
+	 * Because the original mm->exe_file
+	 * points to executable file, make sure
+	 * this one is executable as well to not
+	 * break an overall picture.
+	 */
+	err = -EACCES;
+	if (!S_ISREG(dentry->d_inode->i_mode)	||
+	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+		goto exit;
+
+	err = inode_permission(dentry->d_inode, MAY_EXEC);
+	if (err)
+		goto exit;
+

Quoted 7 lines · Cyrill Gorcunov

Quoted 3 lines · Cyrill Gorcunov

yeah, thanks, will update.

This one should fit all requirements I hope.

Oh, sorry Cyrill, I simply can't resist...

Quoted 4 lines

kernel/sys.c +4 -0

+	/*
+	 * Setting new mm::exe_file is only allowed
+	 * when no VM_EXECUTABLE vma's left. This is
+	 * a special C/R case when a restored program

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1745

Quoted 6 lines

kernel/sys.c +6 -0

+	 * need to change own /proc/$pid/exe symlink.
+	 * After this call mm::num_exe_file_vmas become
+	 * meaningless. If mm::num_exe_file_vmas will
+	 * ever increase back from zero -- this code
+	 * needs to be revised, thus WARN_ here, just
+	 * to be sure.

To be shure in what??

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1748

Quoted 4 lines

kernel/sys.c +3 -0

+	 */
+	down_write(&mm->mmap_sem);
+	WARN_ON_ONCE(mm->num_exe_file_vmas);

We already checked it is zero. Yes, it shouldn't grow. But why do we need another check here?

If it can grow, it can grow after we drop mmap_sem as well and this would be wrong. So may be we need another WARN_ON() at the end?

I'd understand if you add something like

	WARN_ON(!mm->num_exe_file_vmas && !current->in_exec);

into added_exe_file_vma().

Or

	WARN_ON(mm->num_exe_file_vmas <= 0);

into removed_exe_file_vma().

But imho your WARN looks like "OK, I checked it lockless but I am not sure this is correct".

Quoted 13 lines

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1745

Quoted 11 lines

kernel/sys.c +10 -0

+	/*
+	 * Setting new mm::exe_file is only allowed
+	 * when no VM_EXECUTABLE vma's left. This is
+	 * a special C/R case when a restored program
+	 * need to change own /proc/$pid/exe symlink.
+	 * After this call mm::num_exe_file_vmas become
+	 * meaningless. If mm::num_exe_file_vmas will
+	 * ever increase back from zero -- this code
+	 * needs to be revised, thus WARN_ here, just
+	 * to be sure.

To be shure in what??

To be sure it's not increased somewhere else before down_write taken.

Quoted 6 lines

I'd understand if you add something like

	WARN_ON(!mm->num_exe_file_vmas && !current->in_exec);

into added_exe_file_vma().

Quoted 4 lines

Or

	WARN_ON(mm->num_exe_file_vmas <= 0);

into removed_exe_file_vma().

This one looks like a good idea for me -- it's cheap and not a hot path.

Quoted 2 lines

But imho your WARN looks like "OK, I checked it lockless but I am not sure this is correct".

Oleg, I bet if someone will be changing num_exe_file_vmas overall idea -- this prctl code will be fixed at last moment (if ever) only because it's very specific, so I wanted to not miss such moment and add some check that the rest of the kernel is in a good state. This test is cheap but may prevent potential problem if one day mm::exe_file concept will be reworked.

Sure I can simply drop this WARN_ON ;)

Quoted 3 lines

To be sure it's not increased somewhere else before down_write taken.

Who can do this? Only another CLONE_VM thread. And _only_ if we already have the bug in mm_exe accounting logic. And only if that thread does something to trigger the bug in the small window between.

Quoted 4 lines

Quoted 1 line

into removed_exe_file_vma().

This one looks like a good idea for me -- it's cheap and not a hot path.

But not in this patch, please.

Quoted 9 lines

Quoted 2 lines

But imho your WARN looks like "OK, I checked it lockless but I am not sure this is correct".

Oleg, I bet if someone will be changing num_exe_file_vmas overall idea -- this prctl code will be fixed at last moment (if ever) only because it's very specific, so I wanted to not miss such moment and add some check that the rest of the kernel is in a good state. This test is cheap but may prevent potential problem if one day mm::exe_file concept will be reworked.

The test is cheap indeed. If you mean performance-wise.

But it looks confusing, imho. I do not care about a couple of CPU cycles. The code should be optimized for the reading in the first place, not for executing ;) Imho, of course.

And once again. Following your logic you need another WARN_ON() right after we drop mmap_sem. Why? To be sure it's not increased somewhere else _after_ down_write taken. And another one after fput.

Sure, bugs are possible. And yes, in theory this WARN_ON() can catch some problem. But there is tradeoff. Given that you need another thread to trigger the (potential) bug and the window is tiny, how high do you estimate the probability it can help?

Quoted 1 line

Sure I can simply drop this WARN_ON ;)

Oh, keep it if you like it ;)

Yes I hate it, but you are the author and this is almost cosmetic.

OK, Oleg, can't argue, you've convinced me ;) I'll drop this WARN_ON.

Would it be enough for your Reviewed-by tag? /me hides

But, when you send the patch to Andrew, please remind that he should remove the evidence of my ignorance from -mm first.

Just one note for the record, prctl_set_mm_exe_file() does

	if (mm->num_exe_file_vmas)
		return -EBUSY;

We could do

	if (mm->exe_file)
		return -EBUSY;

This way "because this feature is a special to C/R" becomes really true. IOW, you can't do PR_SET_MM_EXE_FILE twice.

I am fine either way, just I want to ensure you really want the current version.

On Fri, Mar 09, 2012 at 04:21:22PM +0100, Oleg Nesterov wrote:

Quoted 7 lines

Just one note for the record, prctl_set_mm_exe_file() does

	if (mm->num_exe_file_vmas)
		return -EBUSY;

We could do

Quoted 6 lines

	if (mm->exe_file)
		return -EBUSY;

This way "because this feature is a special to C/R" becomes really true. IOW, you can't do PR_SET_MM_EXE_FILE twice.

Sure, i'll make it this way. Thanks a lot, Oleg!!!

Sorry about the other email -- hadn't full caught up on this thread.

This is even better, yes.

Of course I'd prefer it if there was a way to keep num_exe_file_vmas correct and not special-case c/r. The first approximation of a solution

might be to increment the count whenever a new mmap filp == mm->exe_file and decrement on unmap. I think there are a bunch of details needed to make that work but my feeling is it's do-able. Have you investigated this already and rejected it for some reason (did I miss that discussion somehow?)?

On Fri, Mar 09, 2012 at 02:02:44PM -0800, Matt Helsley wrote:
...

Quoted 2 lines

Sorry about the other email -- hadn't full caught up on this thread.

No problem.

Quoted 2 lines

This is even better, yes.

Well, in final version I switched back to

(quoted diff) +2 -0

+	if (mm->num_exe_file_vmas)
+		return -EBUSY;

simply because it's more flexible than mm->exe_file.

With mm->exe_file this prctl option become a one-shot only, and while at moment our user-space tool can perfectly live with that I thought that there is no strict need to limit the option this way from the very beginning.

Quoted 3 lines

Of course I'd prefer it if there was a way to keep num_exe_file_vmas correct and not special-case c/r. The first approximation of a solution

It remains correct actually. There is no way to map new VM_EXECUTABLE from user-space after we've unmapped previous ones, so num_exe_file_vmas will remain 0.

Quoted 5 lines

might be to increment the count whenever a new mmap filp == mm->exe_file and decrement on unmap. I think there are a bunch of details needed to make that work but my feeling is it's do-able. Have you investigated this already and rejected it for some reason (did I miss that discussion somehow?)?

As far as I understand overall num_exe_file_vmas concept -- we track a number of VM_EXECUTABLE with it, so setting new exe_file should not change num_exe_file_vmas I think.

Quoted 7 lines

Well, in final version I switched back to

(quoted diff) +2 -0

+	if (mm->num_exe_file_vmas)
+		return -EBUSY;

simply because it's more flexible than mm->exe_file.

Quoted 4 lines

With mm->exe_file this prctl option become a one-shot only, and while at moment our user-space tool can perfectly live with that I thought that there is no strict need to limit the option this way from the very beginning.

As far as backward compatibility, isn't it better to lift that restriction later rather than add it? I think the latter would very likely "break" things whereas the former would not.

I also prefer that restriction because it establishes a bound on how frequently the symlink can change. Keeping it a one-shot deal makes the values that show up in tools like top more reliable for admins.

Quoted 4 lines

As far as I understand overall num_exe_file_vmas concept -- we track a number of VM_EXECUTABLE with it, so setting new exe_file should not change num_exe_file_vmas I think.

True, it's literally correct. However the whole reason for having it is to turn the mm->exe_file reference into a sort of weak reference which happens to coincide with counting the number of VM_EXECUTABLE vmas until you do c/r (really just the restart side of c/r).

Quoted 7 lines · Matt Helsley

Quoted 7 lines

Well, in final version I switched back to

(quoted diff) +2 -0

+	if (mm->num_exe_file_vmas)
+		return -EBUSY;

simply because it's more flexible than mm->exe_file.

Quoted 8 lines

Quoted 4 lines

With mm->exe_file this prctl option become a one-shot only, and while at moment our user-space tool can perfectly live with that I thought that there is no strict need to limit the option this way from the very beginning.

As far as backward compatibility, isn't it better to lift that restriction later rather than add it? I think the latter would very likely "break" things whereas the former would not.

Indeed. But I think any change will mean compatibility broken, programs may rely on one-shot or multi-shot behaviour. So I personally vote for more flexible approach here.

Quoted 5 lines

I also prefer that restriction because it establishes a bound on how frequently the symlink can change. Keeping it a one-shot deal makes the values that show up in tools like top more reliable for admins.

How? Once exe_file changed -- we already cheating the kernel, it's not bad, not good, just work this way ;) I mean imagine an admin which runs top and sees some program in 'top' ouput (btw, I'm not sure but does top really parse /proc/pid/exe?) so say he sees some programs

names -- how would he know if a program did change own /proc/pid/exe at all? Note it's not that important how many times the symlink was changed there is simply no way to find out if it was changed at all, and actually from my POV it's a win for transparent c/r, that was all the idea.

Quoted 11 lines

Quoted 5 lines

As far as I understand overall num_exe_file_vmas concept -- we track a number of VM_EXECUTABLE with it, so setting new exe_file should not change num_exe_file_vmas I think.

True, it's literally correct. However the whole reason for having it is to turn the mm->exe_file reference into a sort of weak reference which happens to coincide with counting the number of VM_EXECUTABLE vmas until you do c/r (really just the restart side of c/r).

Look. We actually have a period of time where exe_file is set but num_exe_file_vmas = 0 when we start program execution before elf map get parsed, so I dare to say such state is legitime (yes, userspace doesn't see this state and yes, we start mapping elf sections pretty immediately after exe_file assigned). So I don't see much problem in extending this "state" (exe_file!=0,num_exe_file_vmas = 0).

Quoted 4 lines

Indeed. But I think any change will mean compatibility broken, programs may rely on one-shot or multi-shot behaviour. So I personally vote for more flexible approach here.

Very true. In fact thinking about this prctl a bit more makes me more certain that one-shot is better and it ought to stay that way forever. The flexibility to change the /proc/pid/exe symlink could be yet another way for malicious code to obscure a compromised program and masquerade as a benign process. That's a problem inherent in this prctl whether its one-shot or multi-shot. However, if you use the one-shot approach then a security-concious program can use this prctl once during its early initialization to ensure the prctl cannot later be abused for this purpose.

Quoted 5 lines

names -- how would he know if a program did change own /proc/pid/exe at all? Note it's not that important how many times the symlink was changed there is simply no way to find out if it was changed at all, and actually from my POV it's a win for transparent c/r, that was all the idea.

I am quite aware of the c/r use for this prctl :). However I also wonder if there aren't serious malicious uses of it. I'm not saying the symlink has to be perfectly accurate at all times, but it's easy and reasonable to make it much harder to abuse this particular prctl for malicious purposes by making it one-shot.

With this patch -- especially the multi-shot form -- the symlink will be entirely under the control of (potentially untrusted) userspace code and the admin is totally at the mercy of the userspace code. In single-shot form programs could use the prctl() to ensure the symlink could not be changed later -- the restart tool would be the only program that would need to ensure that prctl() had not been used since the last exec*().

If we're going to let userspace do arbitrary things to the symlink I can't help but wonder why we can't skip the prctl() altogether and just enable MAP_EXECUTABLE in mmap().

Quoted 6 lines

I am quite aware of the c/r use for this prctl :). However I also wonder if there aren't serious malicious uses of it. I'm not saying the symlink has to be perfectly accurate at all times, but it's easy and reasonable to make it much harder to abuse this particular prctl for malicious purposes by making it one-shot.

ok, convinced, I'll update the patch ;)

Matt, Oleg, there is a final version I hope,
which should fit everyone.

kernel/sys.c +34 -0

+static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
+{
+	struct file *exe_file;
+	struct dentry *dentry;
+	int err;
+
+	/*
+	 * Setting new mm::exe_file is only allowed
+	 * when no VM_EXECUTABLE vma's left. This is
+	 * a special C/R case when a restored program
+	 * need to change own /proc/$pid/exe symlink.
+	 * After this call mm::num_exe_file_vmas become
+	 * meaningless.
+	 */
+	if (mm->num_exe_file_vmas)
+		return -EBUSY;
+
+	exe_file = fget(fd);
+	if (!exe_file)
+		return -EBADF;
+
+	dentry = exe_file->f_path.dentry;
+
+	/*
+	 * Because the original mm->exe_file
+	 * points to executable file, make sure
+	 * this one is executable as well to not
+	 * break an overall picture.
+	 */
+	err = -EACCES;
+	if (!S_ISREG(dentry->d_inode->i_mode)	||
+	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+		goto exit;
+

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1749

Quoted 7 lines

kernel/sys.c +7 -0

+
+	/*
+	 * For security reason changing mm->exe_file
+	 * is one-shot action.
+	 */
+	down_write(&mm->mmap_sem);
+	if (likely(!mm->exe_file))

This means that the num_exe_file_vmas check at the start is not needed. If you want it as a "fast-path" check, please fix the comment. Or just remove it. Otherwise the code looks as if we have to check them both.

Matt, is it really possible to hit mm->exe_file = NULL in removed_exe_file_vma ? Unless I missed something, this check just hides the potentional problem, no?

IOW, shouldn't it do

	void removed_exe_file_vma(struct mm_struct *mm)
	{
		WARN_ON(!mm->exe_file);
		WARN_ON(mm->num_exe_file_vmas <= 0);

Quoted 4 lines

This means that the num_exe_file_vmas check at the start is not needed. If you want it as a "fast-path" check, please fix the comment. Or just remove it. Otherwise the code looks as if we have to check them both.

Yes, I wanted a fast test first, while the second test will give one-shot condition and the second attempt to setup new exe_file will fail. OK, I'll update the comment block.

Quoted 5 lines · Cyrill Gorcunov

Quoted 4 lines

This means that the num_exe_file_vmas check at the start is not needed. If you want it as a "fast-path" check, please fix the comment. Or just remove it. Otherwise the code looks as if we have to check them both.

Quoted 4 lines

Yes, I wanted a fast test first, while the second test will give one-shot condition and the second attempt to setup new exe_file will fail. OK, I'll update the comment block.

Something like below?

Quoted 9 lines · Cyrill Gorcunov

Quoted 4 lines

This means that the num_exe_file_vmas check at the start is not needed. If you want it as a "fast-path" check, please fix the comment. Or just remove it. Otherwise the code looks as if we have to check them both.

Yes, I wanted a fast test first, while the second test will give one-shot condition and the second attempt to setup new exe_file will fail. OK, I'll update the comment block.

Something like below?

Cyrill
---
From: Cyrill Gorcunov <[email protected]>
Subject: 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 the PR_SET_MM_EXE_FILE code is introduced. This option takes a file descriptor which will be set as a source for new /proc/$pid/exe symlink.

Note it allows to change /proc/$pid/exe iif there are no VM_EXECUTABLE vmas present for current process, simply because this feature is a special to C/R and mm::num_exe_file_vmas become meaningless after that.

Also this action is one-shot only. For secutiry reason we don't allow to chanage the symlink several times.

This feature is available iif CONFIG_CHECKPOINT_RESTORE is set.

Signed-off-by Cyrill Gorcunov <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Pavel Emelyanov <[email protected]>

CC Kees Cook <[email protected]>

CC Tejun Heo <[email protected]>

CC Matt Helsley <[email protected]>

---
 include/linux/prctl.h |    1 
 kernel/sys.c          |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================

include/linux/prctl.h +1 -0

@@ -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
===================================================================

kernel/sys.c +22 -0

@@ -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,57 @@ 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;
+
+	/*
+	 * Setting new mm::exe_file is only allowed
+	 * when no VM_EXECUTABLE vma's left. So make
+	 * a fast test first.
+	 */
+	if (mm->num_exe_file_vmas)
+		return -EBUSY;
+
+	exe_file = fget(fd);
+	if (!exe_file)
+		return -EBADF;
+
+	dentry = exe_file->f_path.dentry;
+

Quoted 14 lines

kernel/sys.c +14 -0

+
+	exe_file = fget(fd);
+	if (!exe_file)
+		return -EBADF;
+
+	dentry = exe_file->f_path.dentry;
+
+	/*
+	 * Because the original mm->exe_file
+	 * points to executable file, make sure
+	 * this one is executable as well to not
+	 * break an overall picture.
+	 */
+	err = -EACCES;

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1735

Quoted 3 lines

kernel/sys.c +3 -0

+	if (!S_ISREG(dentry->d_inode->i_mode)	||
+	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+		goto exit;

You could factor out this portion of the access checking from open_exec() after the do_filp_open() in open_exec() and re-use it here. I know it's tiny helper but tying these two together might be good for maintenance later.

Should it check for some of the flags open_exec() uses? open_exec() passes:

O_LARGEFILE|O_RDONLY|__FMODE_EXEC

to do_filp_open(). I think a O_RDONLY check might be good. I don't think __FMODE_EXEC is something userspace can set so could be ignored. O_LARGEFILE might be important though.

On Tue, Mar 13, 2012 at 06:41:06PM -0700, Matt Helsley wrote:
...

Quoted 14 lines

Quoted 14 lines

kernel/sys.c +14 -0

+
+	exe_file = fget(fd);
+	if (!exe_file)
+		return -EBADF;
+
+	dentry = exe_file->f_path.dentry;
+
+	/*
+	 * Because the original mm->exe_file
+	 * points to executable file, make sure
+	 * this one is executable as well to not
+	 * break an overall picture.
+	 */
+	err = -EACCES;

Quoted 9 lines

kernel/sys.c static bool vma_flags_mismatch(struct vm · line 1735

Quoted 3 lines

kernel/sys.c +3 -0

+	if (!S_ISREG(dentry->d_inode->i_mode)	||
+	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+		goto exit;

You could factor out this portion of the access checking from open_exec() after the do_filp_open() in open_exec() and re-use it here. I know it's tiny helper but tying these two together might be good for maintenance later.

Matt, I really dont wanna touch code outside of prctl and this function in particualar, at least in this patch, ie I can clean up and factor out is on top of the patch, as a separate task.

Quoted 9 lines

Should it check for some of the flags open_exec() uses? open_exec() passes:

O_LARGEFILE|O_RDONLY|__FMODE_EXEC

to do_filp_open(). I think a O_RDONLY check might be good. I don't think __FMODE_EXEC is something userspace can set so could be ignored. O_LARGEFILE might be important though.

Well, we're not going to read from this file, so it is not that important at moment, so previously I've had

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if ((exe_file->f_flags & O_ACCMODE) != O_RDONLY)
+             goto exit;

and Oleg pointed me

 | 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.

so I dropped it. And I think the same applies to O_LARGEFILE. Sure it's not a problem to bring it back but should we?

OK, sorry I must have missed that portion of the discussion. It all looks good to me.

Matt, is it really possible to hit mm->exe_file = NULL in removed_exe_file_vma ? Unless I missed something, this check just hides the potentional problem, no?

IOW, shouldn't it do

	void removed_exe_file_vma(struct mm_struct *mm)
	{
		WARN_ON(!mm->exe_file);
		WARN_ON(mm->num_exe_file_vmas <= 0);

		if (!--mm->num_exe_file_vmas) {
			fput(mm->exe_file);
			mm->exe_file = NULL;
		}
	}

?

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines · Cyrill Gorcunov

Hi Oleg, could you please take a look once you get a minute (no urgency).

Add Matt. I won't touch the text below to keep the patch intact.

Thanks for CC'ing Matt, Oleg (I forgot, sorry).

Quoted 14 lines

With this change

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

I simply do not understand what mm->num_exe_file_vmas means after PR_SET_MM_EXE_FILE.

Quoted 11 lines

I think that you should do

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

Quoted 7 lines

to keep the current "mm->exe_file goes away after the final unmap(MAP_EXECUTABLE)" logic.

OK, may be this doesn't work in c/r case because you are actually going to remove the old mappings? But in this case the new exe_file will go away anyway, afaics PR_SET_MM_EXE_FILE is called when you still have the old mappings.

Yes, exactly, I need to remove old mappings first (because VMAs we're about to restore may intersect with current map the host program has). And yes, once they all are removed I don't have /proc/pid/exe anymore. That's why I need num_exe_file_vmas == 0 case.

When I setup new exe_file with num_exe_file_vmas = 0, this reference to a file brings /proc/pid/exe back to live (and when process exiting it'll call set_mm_exe_file(mm, NULL) and the new exe_file will be dropped, so no leak here).

Quoted 6 lines

And I don't think the unconditional

	down_write(&mm->mmap_sem);
	set_mm_exe_file(mm, exe_file);
	up_write(&mm->mmap_sem);

Quoted 6 lines

is 100% right, this clears ->num_exe_file_vmas. This means that (if you still have the old mapping) the new exe_file can go away after added_exe_file_vma() + removed_exe_file_vma(). Normally this should happen, but afaics this is possible. Note that even, say, mprotect() can trigger added_exe_file_vma().

Wait, Oleg, I'm confused, in case if there *is* exitsting VM_EXECUTABLEs then we jump into first banch and simply replace old exe_file.

If there is no VM_EXECUTABLEs, then we simply setup new exe_file and num_exe_file_vmas remains zero.

Quoted 3 lines

Quoted 3 lines

I simply do not understand what mm->num_exe_file_vmas means after PR_SET_MM_EXE_FILE.

I think it should fail if the num_exe_file_vmas is not 0 when PR_SET_MM_EXE_FILE is used. It's simple, keeps things clear, might catch userspace bugs (harder to accidentally leave a mapping of the original executable), and could avoid kernel bugs too.

Quoted 3 lines

Wait, Oleg, I'm confused, in case if there *is* exitsting VM_EXECUTABLEs then we jump into first banch and simply replace old exe_file.

What happens if multiple prctl calls are made? We'll have a mix of N executable files that've been mapped n_i times. I think we're better off just returning an error in that case -- -EBUSY or something.

Quoted 2 lines

If there is no VM_EXECUTABLEs, then we simply setup new exe_file and num_exe_file_vmas remains zero.

Which is fine.

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 option takes a file descriptor which will be set as new /proc/$pid/exe symlink.

This feature is available iif CONFIG_CHECKPOINT_RESTORE is set. [...]
Index: linux-2.6.git/kernel/sys.c
===================================================================

kernel/sys.c +8 -0

[...]
+       exe_file = fget(fd);
+       if (!exe_file)
+               return -EBADF;
+
+       dentry = exe_file->f_path.dentry;
+
+       /*
+        * Because the original mm->exe_file

Quoted 4 lines

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

I'm starting to notice that this pattern (testing ISREG and MNT_NOEXEC) is getting repeated a few times in the kernel, and at least the no-new-privs patch (not yet in -mm but hopefully soon given the seccomp_filter work) updates this pattern everywhere. Perhaps this should be extracted into a helper first, and then this patch can call that helper here? (And then nnp can just update the single helper.)

On Thu, Mar 08, 2012 at 11:31:58AM -0800, Kees Cook wrote:
...

Quoted 5 lines

Quoted 4 lines

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

Quoted 7 lines

I'm starting to notice that this pattern (testing ISREG and MNT_NOEXEC) is getting repeated a few times in the kernel, and at least the no-new-privs patch (not yet in -mm but hopefully soon given the seccomp_filter work) updates this pattern everywhere. Perhaps this should be extracted into a helper first, and then this patch can call that helper here? (And then nnp can just update the single helper.)

I can do that if Andrew agree.

       Cyrill

Quoted 15 lines · Cyrill Gorcunov

On Thu, Mar 08, 2012 at 11:31:58AM -0800, Kees Cook wrote:
...

Quoted 12 lines

Quoted 4 lines

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

I'm starting to notice that this pattern (testing ISREG and MNT_NOEXEC) is getting repeated a few times in the kernel, and at least the no-new-privs patch (not yet in -mm but hopefully soon given the seccomp_filter work) updates this pattern everywhere. Perhaps this should be extracted into a helper first, and then this patch can call that helper here? (And then nnp can just update the single helper.)

Quoted 1 line

I can do that if Andrew agree.

I'm a bit lost.  nnp updates the MNT_NOSUID checks, not the MNT_NOEXEC checks.  (And the effects of the two flags is different in selinux for historical reasons.)  I'm sure I'm missing something.

Quoted 2 lines

I can do that if Andrew agree.

I'm a bit lost. nnp updates the MNT_NOSUID checks, not the MNT_NOEXEC checks. (And the effects of the two flags is different in selinux for historical reasons.) I'm sure I'm missing something.

Quoted 3 lines · Andy Lutomirski

Quoted 2 lines

I can do that if Andrew agree.

Quoted 4 lines

I'm a bit lost.  nnp updates the MNT_NOSUID checks, not the MNT_NOEXEC checks.  (And the effects of the two flags is different in selinux for historical reasons.)  I'm sure I'm missing something.

Andy, I've no idea what nnp is ;) I was only about to gather those ISREG/MNT_NOEXEC to one helper since we indeed have a few places in kernel which do same thing in open-coded manner.

Quoted 9 lines · Cyrill Gorcunov

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

I can do that if Andrew agree.

I'm a bit lost.  nnp updates the MNT_NOSUID checks, not the MNT_NOEXEC checks.  (And the effects of the two flags is different in selinux for historical reasons.)  I'm sure I'm missing something.

Quoted 3 lines

Andy, I've no idea what nnp is ;) I was only about to gather those ISREG/MNT_NOEXEC to one helper since we indeed have a few places in kernel which do same thing in open-coded manner.

Am I not the Andrew you were referring to?

Quoted 14 lines · Andy Lutomirski

Quoted 12 lines · Cyrill Gorcunov

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

I can do that if Andrew agree.

I'm a bit lost.  nnp updates the MNT_NOSUID checks, not the MNT_NOEXEC checks.  (And the effects of the two flags is different in selinux for historical reasons.)  I'm sure I'm missing something.

Andy, I've no idea what nnp is ;) I was only about to gather those ISREG/MNT_NOEXEC to one helper since we indeed have a few places in kernel which do same thing in open-coded manner.

Quoted 2 lines

Am I not the Andrew you were referring to?

Nope, I meant Andrew Morton /because this patch is for -mm/ ;)

I've been in To: field that's why I replied you about nnp (and, btw, what nnp is? not "Net national product" I suppose, this hint wikipedia gave me)

Quoted 4 lines

I've been in To: field that's why I replied you about nnp (and, btw, what nnp is? not "Net national product" I suppose, this hint wikipedia gave me)

nnp is no_new_privs, which is my patch and is almost, but not quite, very relevant to this discussion. Hence my confusion ;)

FWIW, since I've touched this code recently, the cleanup you're suggesting sounds good.

nnp is no_new_privs, which is my patch and is almost, but not quite, very relevant to this discussion. Hence my confusion ;)

FWIW, since I've touched this code recently, the cleanup you're suggesting sounds good.

Quoted 7 lines · Andy Lutomirski

nnp is no_new_privs, which is my patch and is almost, but not quite, very relevant to this discussion.  Hence my confusion ;)

FWIW, since I've touched this code recently, the cleanup you're suggesting sounds good.

Andy, Kees, I guess the patch below might be a helper we need, while I'm not sure on naming. hm?

       Cyrill
---
 include/linux/fs.h |    6 ++++++

 1 file changed, 6 insertions(+)

Index: linux-2.6.git/include/linux/fs.h
===================================================================

include/linux/fs.h +0 -0

@@ -2669,5 +2669,11 @@ static inline void inode_has_no_xattr(st
                inode->i_flags |= S_NOSEC;

How about "file_is_exec" instead, since it doesn't (and likely shouldn't) include the inode_permission(..., EXEC)? I'd like other people's thoughts on this since maybe it's not needed and I instead have accidentally derailed this patch with useless bike shedding.

Quoted 6 lines · Kees Cook

How about "file_is_exec" instead, since it doesn't (and likely shouldn't) include the inode_permission(..., EXEC)? I'd like other people's thoughts on this since maybe it's not needed and I instead have accidentally derailed this patch with useless bike shedding.

Yup. Anyway, I'm leaving the former mm_struct::exe_file patch with EXEC test opencoded so we can do everything on top then.