[PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Cyrill Gorcunov]

The mm::start_code, end_code, start_data, end_data members
are set during startup of executable file and are not changed
after.

But the program itself might map new executable or/and data areas in
time so the original values written into mm fields mentioned above
might not have correspond VMA area at all, thus if one try to
use this prctl codes without underlied VMA, the error will be
returned.

Drop this requirement. This shrinks the code and eliminates
redundant calls to vma_flags_mismatch. The worst thing one can
do (if say to write some bad values here) -- the weird results
will be shown in /proc/$pid/statm or in /proc/pid/stat.

Still, assignement of data on stack (such as command line and
environment variables) requires the underlied VMA to exist.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
This code is under CONFIG_CHECKPOINT_RESTORE. I would really
appreciate some review, just to check I've not missed something.

 kernel/sys.c |   34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -1777,38 +1777,18 @@ static int prctl_set_mm(int opt, unsigne
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, addr);
 
-	if (opt != PR_SET_MM_START_BRK &&
-	    opt != PR_SET_MM_BRK &&
-	    opt != PR_SET_MM_AUXV) {
-		/* It must be existing VMA */
-		if (!vma || vma->vm_start > addr)
-			goto out;
-	}
-
-	error = -EINVAL;
 	switch (opt) {
 	case PR_SET_MM_START_CODE:
+		mm->start_code = addr;
+		break;
 	case PR_SET_MM_END_CODE:
-		if (vma_flags_mismatch(vma, VM_READ | VM_EXEC,
-				       VM_WRITE | VM_MAYSHARE))
-			goto out;
-
-		if (opt == PR_SET_MM_START_CODE)
-			mm->start_code = addr;
-		else
-			mm->end_code = addr;
+		mm->end_code = addr;
 		break;
-
 	case PR_SET_MM_START_DATA:
+		mm->start_data = addr;
+		break;
 	case PR_SET_MM_END_DATA:
-		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE,
-				       VM_EXEC | VM_MAYSHARE))
-			goto out;
-
-		if (opt == PR_SET_MM_START_DATA)
-			mm->start_data = addr;
-		else
-			mm->end_data = addr;
+		mm->end_data = addr;
 		break;
 
 	case PR_SET_MM_START_BRK:
@@ -1847,6 +1827,8 @@ static int prctl_set_mm(int opt, unsigne
 	case PR_SET_MM_ARG_END:
 	case PR_SET_MM_ENV_START:
 	case PR_SET_MM_ENV_END:
+		if (!vma)
+			goto out;
 #ifdef CONFIG_STACK_GROWSUP
 		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSUP, 0))
 #else

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Kees Cook]

On Mon, Apr 16, 2012 at 3:55 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> The mm::start_code, end_code, start_data, end_data members
> are set during startup of executable file and are not changed
> after.
>
> But the program itself might map new executable or/and data areas in
> time so the original values written into mm fields mentioned above
> might not have correspond VMA area at all, thus if one try to
> use this prctl codes without underlied VMA, the error will be
> returned.

Hrm, what is the utility of these fields then? If they're not "real",
should the kernel even bother tracking it at all? (Or, should it be
fixed to actually do something useful?)

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Cyrill Gorcunov]

On Tue, Apr 17, 2012 at 09:26:07AM -0700, Kees Cook wrote:
> On Mon, Apr 16, 2012 at 3:55 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > The mm::start_code, end_code, start_data, end_data members
> > are set during startup of executable file and are not changed
> > after.
> >
> > But the program itself might map new executable or/and data areas in
> > time so the original values written into mm fields mentioned above
> > might not have correspond VMA area at all, thus if one try to
> > use this prctl codes without underlied VMA, the error will be
> > returned.
> 
> Hrm, what is the utility of these fields then? If they're not "real",
> should the kernel even bother tracking it at all? (Or, should it be
> fixed to actually do something useful?)

As far as I see they are used to print statistics on /proc. Maybe
here some hidden meaning in them I missed that's why I asked for
review ;)

	Cyrill

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Pavel Emelyanov]

On 04/17/2012 08:26 PM, Kees Cook wrote:
> On Mon, Apr 16, 2012 at 3:55 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>> The mm::start_code, end_code, start_data, end_data members
>> are set during startup of executable file and are not changed
>> after.
>>
>> But the program itself might map new executable or/and data areas in
>> time so the original values written into mm fields mentioned above
>> might not have correspond VMA area at all, thus if one try to
>> use this prctl codes without underlied VMA, the error will be
>> returned.
> 
> Hrm, what is the utility of these fields then? If they're not "real",
> should the kernel even bother tracking it at all? (Or, should it be
> fixed to actually do something useful?)

As far as I see these values are used by aout binfmt core-dumping code
and in flat relocations load. Elf doesn't read either of them, just loads.

> -Kees
> 

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Cyrill Gorcunov]

On Tue, Apr 17, 2012 at 08:32:06PM +0400, Pavel Emelyanov wrote:
> On 04/17/2012 08:26 PM, Kees Cook wrote:
> > On Mon, Apr 16, 2012 at 3:55 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> >> The mm::start_code, end_code, start_data, end_data members
> >> are set during startup of executable file and are not changed
> >> after.
> >>
> >> But the program itself might map new executable or/and data areas in
> >> time so the original values written into mm fields mentioned above
> >> might not have correspond VMA area at all, thus if one try to
> >> use this prctl codes without underlied VMA, the error will be
> >> returned.
> > 
> > Hrm, what is the utility of these fields then? If they're not "real",
> > should the kernel even bother tracking it at all? (Or, should it be
> > fixed to actually do something useful?)
> 
> As far as I see these values are used by aout binfmt core-dumping code
> and in flat relocations load. Elf doesn't read either of them, just loads.

True. But key moment for us (ie c/r) is that where these members are used
as the source operands after checkpoint/restore (ie is there a way to write
some 'bad' values via prctl interface and cause kernel to panic or whatever,
regardless the fact that one need to obtain cap-sys-resource capability
first, before he will be able to use these prctl codes). And I didn't find
any side effect except it may cause incorrect statistics in /proc output.

	Cyrill

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Kees Cook]

On Mon, Apr 16, 2012 at 3:55 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> The mm::start_code, end_code, start_data, end_data members
> are set during startup of executable file and are not changed
> after.
>
> But the program itself might map new executable or/and data areas in
> time so the original values written into mm fields mentioned above
> might not have correspond VMA area at all, thus if one try to
> use this prctl codes without underlied VMA, the error will be
> returned.
>
> Drop this requirement. This shrinks the code and eliminates
> redundant calls to vma_flags_mismatch. The worst thing one can
> do (if say to write some bad values here) -- the weird results
> will be shown in /proc/$pid/statm or in /proc/pid/stat.
>
> Still, assignement of data on stack (such as command line and
> environment variables) requires the underlied VMA to exist.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

Since this is CAP_SYS_RESOURCE, and mmap_min_addr is CAP_SYS_RAWIO,
how about a lower-bounds check against mmap_min_addr? (We're already
doing the TASK_SIZE upper check, so this additional sanity checking
seems reasonable to me.)

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Cyrill Gorcunov]

On Tue, Apr 17, 2012 at 11:22:06AM -0700, Kees Cook wrote:
> On Mon, Apr 16, 2012 at 3:55 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > The mm::start_code, end_code, start_data, end_data members
> > are set during startup of executable file and are not changed
> > after.
> >
> > But the program itself might map new executable or/and data areas in
> > time so the original values written into mm fields mentioned above
> > might not have correspond VMA area at all, thus if one try to
> > use this prctl codes without underlied VMA, the error will be
> > returned.
> >
> > Drop this requirement. This shrinks the code and eliminates
> > redundant calls to vma_flags_mismatch. The worst thing one can
> > do (if say to write some bad values here) -- the weird results
> > will be shown in /proc/$pid/statm or in /proc/pid/stat.
> >
> > Still, assignement of data on stack (such as command line and
> > environment variables) requires the underlied VMA to exist.
> >
> > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> 
> Since this is CAP_SYS_RESOURCE, and mmap_min_addr is CAP_SYS_RAWIO,
> how about a lower-bounds check against mmap_min_addr? (We're already
> doing the TASK_SIZE upper check, so this additional sanity checking
> seems reasonable to me.)

I think this is good idea, thanks Kees. I'll check it out.

	Cyrill

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Cyrill Gorcunov]

On Tue, Apr 17, 2012 at 11:19:16PM +0400, Cyrill Gorcunov wrote:
...
> > Since this is CAP_SYS_RESOURCE, and mmap_min_addr is CAP_SYS_RAWIO,
> > how about a lower-bounds check against mmap_min_addr? (We're already
> > doing the TASK_SIZE upper check, so this additional sanity checking
> > seems reasonable to me.)
> 
> I think this is good idea, thanks Kees. I'll check it out.

Updated and tested version is below.

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [PATCH] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment v2

The mm::start_code, end_code, start_data, end_data members
are set during startup of executable file and are not changed
after.

But the program itself might map new executable or/and data areas in
time so the original values written into mm fields mentioned above
might not have correspond VMA area at all, thus if one try to
use this prctl codes without underlied VMA, the error will be
returned.

Drop this requirement. This shrinks the code and eliminates
redundant calls to vma_flags_mismatch. The worst thing one can
do (if say to write some bad values here) -- the weird results
will be shown in /proc/$pid/statm or in /proc/pid/stat.

Still, assignement of data on stack (such as command line and
environment variables) requires the underlied VMA to exist.

v2:
 Also make sure the address being set is greater than mmap_min_addr.
 Suggested by Kees Cook.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/sys.c |   36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -1771,44 +1771,24 @@ static int prctl_set_mm(int opt, unsigne
 	if (opt == PR_SET_MM_EXE_FILE)
 		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
 
-	if (addr >= TASK_SIZE)
+	if (addr >= TASK_SIZE || addr < mmap_min_addr)
 		return -EINVAL;
 
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, addr);
 
-	if (opt != PR_SET_MM_START_BRK &&
-	    opt != PR_SET_MM_BRK &&
-	    opt != PR_SET_MM_AUXV) {
-		/* It must be existing VMA */
-		if (!vma || vma->vm_start > addr)
-			goto out;
-	}
-
-	error = -EINVAL;
 	switch (opt) {
 	case PR_SET_MM_START_CODE:
+		mm->start_code = addr;
+		break;
 	case PR_SET_MM_END_CODE:
-		if (vma_flags_mismatch(vma, VM_READ | VM_EXEC,
-				       VM_WRITE | VM_MAYSHARE))
-			goto out;
-
-		if (opt == PR_SET_MM_START_CODE)
-			mm->start_code = addr;
-		else
-			mm->end_code = addr;
+		mm->end_code = addr;
 		break;
-
 	case PR_SET_MM_START_DATA:
+		mm->start_data = addr;
+		break;
 	case PR_SET_MM_END_DATA:
-		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE,
-				       VM_EXEC | VM_MAYSHARE))
-			goto out;
-
-		if (opt == PR_SET_MM_START_DATA)
-			mm->start_data = addr;
-		else
-			mm->end_data = addr;
+		mm->end_data = addr;
 		break;
 
 	case PR_SET_MM_START_BRK:
@@ -1847,6 +1827,8 @@ static int prctl_set_mm(int opt, unsigne
 	case PR_SET_MM_ARG_END:
 	case PR_SET_MM_ENV_START:
 	case PR_SET_MM_ENV_END:
+		if (!vma)
+			goto out;
 #ifdef CONFIG_STACK_GROWSUP
 		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSUP, 0))
 #else

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Kees Cook]

On Tue, Apr 17, 2012 at 12:49 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> On Tue, Apr 17, 2012 at 11:19:16PM +0400, Cyrill Gorcunov wrote:
> ...
>> > Since this is CAP_SYS_RESOURCE, and mmap_min_addr is CAP_SYS_RAWIO,
>> > how about a lower-bounds check against mmap_min_addr? (We're already
>> > doing the TASK_SIZE upper check, so this additional sanity checking
>> > seems reasonable to me.)
>>
>> I think this is good idea, thanks Kees. I'll check it out.
>
> Updated and tested version is below.
>
>        Cyrill
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: [PATCH] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment v2
>
> The mm::start_code, end_code, start_data, end_data members
> are set during startup of executable file and are not changed
> after.
>
> But the program itself might map new executable or/and data areas in
> time so the original values written into mm fields mentioned above
> might not have correspond VMA area at all, thus if one try to
> use this prctl codes without underlied VMA, the error will be
> returned.
>
> Drop this requirement. This shrinks the code and eliminates
> redundant calls to vma_flags_mismatch. The worst thing one can
> do (if say to write some bad values here) -- the weird results
> will be shown in /proc/$pid/statm or in /proc/pid/stat.
>
> Still, assignement of data on stack (such as command line and
> environment variables) requires the underlied VMA to exist.
>
> v2:
>  Also make sure the address being set is greater than mmap_min_addr.
>  Suggested by Kees Cook.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
ChromeOS Security

Re: [PATCH c/r -mm] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment

[Serge Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> On Tue, Apr 17, 2012 at 11:19:16PM +0400, Cyrill Gorcunov wrote:
> ...
> > > Since this is CAP_SYS_RESOURCE, and mmap_min_addr is CAP_SYS_RAWIO,
> > > how about a lower-bounds check against mmap_min_addr? (We're already
> > > doing the TASK_SIZE upper check, so this additional sanity checking
> > > seems reasonable to me.)
> > 
> > I think this is good idea, thanks Kees. I'll check it out.
> 
> Updated and tested version is below.
> 
> 	Cyrill
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: [PATCH] c/r: prctl: Simplify PR_SET_MM on mm::code/data assignment v2
> 
> The mm::start_code, end_code, start_data, end_data members
> are set during startup of executable file and are not changed
> after.
> 
> But the program itself might map new executable or/and data areas in
> time so the original values written into mm fields mentioned above
> might not have correspond VMA area at all, thus if one try to
> use this prctl codes without underlied VMA, the error will be
> returned.
> 
> Drop this requirement. This shrinks the code and eliminates
> redundant calls to vma_flags_mismatch. The worst thing one can
> do (if say to write some bad values here) -- the weird results
> will be shown in /proc/$pid/statm or in /proc/pid/stat.
> 
> Still, assignement of data on stack (such as command line and
> environment variables) requires the underlied VMA to exist.
> 
> v2:
>  Also make sure the address being set is greater than mmap_min_addr.
>  Suggested by Kees Cook.

Thanks for that, Kees.

> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  kernel/sys.c |   36 +++++++++---------------------------
>  1 file changed, 9 insertions(+), 27 deletions(-)
> 
> Index: linux-2.6.git/kernel/sys.c
> ===================================================================
> --- linux-2.6.git.orig/kernel/sys.c
> +++ linux-2.6.git/kernel/sys.c
> @@ -1771,44 +1771,24 @@ static int prctl_set_mm(int opt, unsigne
>  	if (opt == PR_SET_MM_EXE_FILE)
>  		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
>  
> -	if (addr >= TASK_SIZE)
> +	if (addr >= TASK_SIZE || addr < mmap_min_addr)
>  		return -EINVAL;
>  
>  	down_read(&mm->mmap_sem);
>  	vma = find_vma(mm, addr);
>  
> -	if (opt != PR_SET_MM_START_BRK &&
> -	    opt != PR_SET_MM_BRK &&
> -	    opt != PR_SET_MM_AUXV) {
> -		/* It must be existing VMA */
> -		if (!vma || vma->vm_start > addr)
> -			goto out;
> -	}
> -
> -	error = -EINVAL;
>  	switch (opt) {
>  	case PR_SET_MM_START_CODE:
> +		mm->start_code = addr;
> +		break;
>  	case PR_SET_MM_END_CODE:
> -		if (vma_flags_mismatch(vma, VM_READ | VM_EXEC,
> -				       VM_WRITE | VM_MAYSHARE))
> -			goto out;
> -
> -		if (opt == PR_SET_MM_START_CODE)
> -			mm->start_code = addr;
> -		else
> -			mm->end_code = addr;
> +		mm->end_code = addr;
>  		break;
> -
>  	case PR_SET_MM_START_DATA:
> +		mm->start_data = addr;
> +		break;
>  	case PR_SET_MM_END_DATA:
> -		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE,
> -				       VM_EXEC | VM_MAYSHARE))
> -			goto out;
> -
> -		if (opt == PR_SET_MM_START_DATA)
> -			mm->start_data = addr;
> -		else
> -			mm->end_data = addr;
> +		mm->end_data = addr;
>  		break;
>  
>  	case PR_SET_MM_START_BRK:
> @@ -1847,6 +1827,8 @@ static int prctl_set_mm(int opt, unsigne
>  	case PR_SET_MM_ARG_END:
>  	case PR_SET_MM_ENV_START:
>  	case PR_SET_MM_ENV_END:
> +		if (!vma)
> +			goto out;
>  #ifdef CONFIG_STACK_GROWSUP
>  		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSUP, 0))
>  #else