compel: Do not loose sign of result in compat syscall

Submitted by Cyrill Gorcunov on Oct. 30, 2017, 2:57 p.m.

Details

Message ID 1509375451-23591-1-git-send-email-gorcunov@openvz.org
State New
Series "compel: Do not loose sign of result in compat syscall"
Headers show

Commit Message

Cyrill Gorcunov Oct. 30, 2017, 2:57 p.m.
From: Cyrill Gorcunov <gorcunov@virtuozzo.com>

Regs are present in unsigned format so convert them
into signed first to provide results.

In particular if memfd_create syscall failed we won't
notice -ENOMEM error but rather treat it as unsigned
hex value

 | (05.303002) Putting parasite blob into 0x7f1c6ffe0000->0xfffffff4
 | (05.303234) Putting tsock into pid 42773

Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
---

Travis is running by now https://travis-ci.org/cyrillos/criu/builds/294898355

 compel/arch/x86/src/lib/infect.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
index 9c919e64ef13..ac5f8b05e768 100644
--- a/compel/arch/x86/src/lib/infect.c
+++ b/compel/arch/x86/src/lib/infect.c
@@ -293,9 +293,10 @@  int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
 		unsigned long arg6)
 {
 	user_regs_struct_t regs = ctl->orig.regs;
+	bool native = user_regs_native(&regs);
 	int err;
 
-	if (user_regs_native(&regs)) {
+	if (native) {
 		user_regs_struct64 *r = &regs.native;
 
 		r->ax  = (uint64_t)nr;
@@ -321,7 +322,9 @@  int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
 		err = compel_execute_syscall(ctl, &regs, code_int_80);
 	}
 
-	*ret = get_user_reg(&regs, ax);
+	*ret = native ?
+		(long)get_user_reg(&regs, ax) :
+		(int)get_user_reg(&regs, ax);
 	return err;
 }
 

Comments

Andrei Vagin Oct. 30, 2017, 7:18 p.m.
On Mon, Oct 30, 2017 at 05:57:31PM +0300, Cyrill Gorcunov wrote:
> From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> 
> Regs are present in unsigned format so convert them
> into signed first to provide results.
> 
> In particular if memfd_create syscall failed we won't
> notice -ENOMEM error but rather treat it as unsigned
> hex value
> 
>  | (05.303002) Putting parasite blob into 0x7f1c6ffe0000->0xfffffff4
>  | (05.303234) Putting tsock into pid 42773
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> ---
> 
> Travis is running by now https://travis-ci.org/cyrillos/criu/builds/294898355
> 
>  compel/arch/x86/src/lib/infect.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
> index 9c919e64ef13..ac5f8b05e768 100644
> --- a/compel/arch/x86/src/lib/infect.c
> +++ b/compel/arch/x86/src/lib/infect.c
> @@ -293,9 +293,10 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
>  		unsigned long arg6)
>  {
>  	user_regs_struct_t regs = ctl->orig.regs;
> +	bool native = user_regs_native(&regs);
>  	int err;
>  
> -	if (user_regs_native(&regs)) {
> +	if (native) {
>  		user_regs_struct64 *r = &regs.native;
>  
>  		r->ax  = (uint64_t)nr;
> @@ -321,7 +322,9 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
>  		err = compel_execute_syscall(ctl, &regs, code_int_80);
>  	}
>  
> -	*ret = get_user_reg(&regs, ax);
> +	*ret = native ?
> +		(long)get_user_reg(&regs, ax) :
> +		(int)get_user_reg(&regs, ax);

mmap() can return a negative value, but it will be actually a valid
address and we have to return it without modifications. How does this
code handle this case?

>  	return err;
>  }
>  
> -- 
> 2.7.5
>
Cyrill Gorcunov Oct. 30, 2017, 8:03 p.m.
On Mon, Oct 30, 2017 at 12:18:12PM -0700, Andrey Vagin wrote:
> On Mon, Oct 30, 2017 at 05:57:31PM +0300, Cyrill Gorcunov wrote:
> > From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> > 
> > Regs are present in unsigned format so convert them
> > into signed first to provide results.
> > 
> > In particular if memfd_create syscall failed we won't
> > notice -ENOMEM error but rather treat it as unsigned
> > hex value
> > 
> >  | (05.303002) Putting parasite blob into 0x7f1c6ffe0000->0xfffffff4
> >  | (05.303234) Putting tsock into pid 42773
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> > ---
> > 
> > Travis is running by now https://travis-ci.org/cyrillos/criu/builds/294898355
> > 
> >  compel/arch/x86/src/lib/infect.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
> > index 9c919e64ef13..ac5f8b05e768 100644
> > --- a/compel/arch/x86/src/lib/infect.c
> > +++ b/compel/arch/x86/src/lib/infect.c
> > @@ -293,9 +293,10 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
> >  		unsigned long arg6)
> >  {
> >  	user_regs_struct_t regs = ctl->orig.regs;
> > +	bool native = user_regs_native(&regs);
> >  	int err;
> >  
> > -	if (user_regs_native(&regs)) {
> > +	if (native) {
> >  		user_regs_struct64 *r = &regs.native;
> >  
> >  		r->ax  = (uint64_t)nr;
> > @@ -321,7 +322,9 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
> >  		err = compel_execute_syscall(ctl, &regs, code_int_80);
> >  	}
> >  
> > -	*ret = get_user_reg(&regs, ax);
> > +	*ret = native ?
> > +		(long)get_user_reg(&regs, ax) :
> > +		(int)get_user_reg(&regs, ax);
> 
> mmap() can return a negative value, but it will be actually a valid
> address and we have to return it without modifications. How does this
> code handle this case?

This code has nothing to do with such issues, it's up to a caller
to verify the values obtained.

In particular for x86 IS_ERR_VALUE will catch the error if only
sign of the value is not lost, thus we have to preserve it.

More likely we need an additional check on top of this patch
to strip off sign propagated before it get converted into the
void pointer.
Andrei Vagin Oct. 30, 2017, 8:14 p.m.
On Mon, Oct 30, 2017 at 11:03:25PM +0300, Cyrill Gorcunov wrote:
> On Mon, Oct 30, 2017 at 12:18:12PM -0700, Andrey Vagin wrote:
> > On Mon, Oct 30, 2017 at 05:57:31PM +0300, Cyrill Gorcunov wrote:
> > > From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> > > 
> > > Regs are present in unsigned format so convert them
> > > into signed first to provide results.
> > > 
> > > In particular if memfd_create syscall failed we won't
> > > notice -ENOMEM error but rather treat it as unsigned
> > > hex value
> > > 
> > >  | (05.303002) Putting parasite blob into 0x7f1c6ffe0000->0xfffffff4
> > >  | (05.303234) Putting tsock into pid 42773
> > > 
> > > Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> > > ---
> > > 
> > > Travis is running by now https://travis-ci.org/cyrillos/criu/builds/294898355
> > > 
> > >  compel/arch/x86/src/lib/infect.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
> > > index 9c919e64ef13..ac5f8b05e768 100644
> > > --- a/compel/arch/x86/src/lib/infect.c
> > > +++ b/compel/arch/x86/src/lib/infect.c
> > > @@ -293,9 +293,10 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
> > >  		unsigned long arg6)
> > >  {
> > >  	user_regs_struct_t regs = ctl->orig.regs;
> > > +	bool native = user_regs_native(&regs);
> > >  	int err;
> > >  
> > > -	if (user_regs_native(&regs)) {
> > > +	if (native) {
> > >  		user_regs_struct64 *r = &regs.native;
> > >  
> > >  		r->ax  = (uint64_t)nr;
> > > @@ -321,7 +322,9 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
> > >  		err = compel_execute_syscall(ctl, &regs, code_int_80);
> > >  	}
> > >  
> > > -	*ret = get_user_reg(&regs, ax);
> > > +	*ret = native ?
> > > +		(long)get_user_reg(&regs, ax) :
> > > +		(int)get_user_reg(&regs, ax);
> > 
> > mmap() can return a negative value, but it will be actually a valid
> > address and we have to return it without modifications. How does this
> > code handle this case?
> 
> This code has nothing to do with such issues, it's up to a caller
> to verify the values obtained.

This code modifies a valid value, so you fix one issue and create a new
one. Could you fix both of them? ;)

> 
> In particular for x86 IS_ERR_VALUE will catch the error if only
> sign of the value is not lost, thus we have to preserve it.
> 
> More likely we need an additional check on top of this patch
> to strip off sign propagated before it get converted into the
> void pointer.
Dmitry Safonov Oct. 31, 2017, 12:11 p.m.
2017-10-30 14:57 GMT+00:00 Cyrill Gorcunov <gorcunov@openvz.org>:
> From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
>
> Regs are present in unsigned format so convert them
> into signed first to provide results.
>
> In particular if memfd_create syscall failed we won't
> notice -ENOMEM error but rather treat it as unsigned
> hex value
>
>  | (05.303002) Putting parasite blob into 0x7f1c6ffe0000->0xfffffff4
>  | (05.303234) Putting tsock into pid 42773
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>

Looks like a right thing to me,
Reviewed-by: Dmitry Safonov <dima@arista.com>

> ---
>
> Travis is running by now https://travis-ci.org/cyrillos/criu/builds/294898355
>
>  compel/arch/x86/src/lib/infect.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
> index 9c919e64ef13..ac5f8b05e768 100644
> --- a/compel/arch/x86/src/lib/infect.c
> +++ b/compel/arch/x86/src/lib/infect.c
> @@ -293,9 +293,10 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
>                 unsigned long arg6)
>  {
>         user_regs_struct_t regs = ctl->orig.regs;
> +       bool native = user_regs_native(&regs);
>         int err;
>
> -       if (user_regs_native(&regs)) {
> +       if (native) {
>                 user_regs_struct64 *r = &regs.native;
>
>                 r->ax  = (uint64_t)nr;
> @@ -321,7 +322,9 @@ int compel_syscall(struct parasite_ctl *ctl, int nr, long *ret,
>                 err = compel_execute_syscall(ctl, &regs, code_int_80);
>         }
>
> -       *ret = get_user_reg(&regs, ax);
> +       *ret = native ?
> +               (long)get_user_reg(&regs, ax) :
> +               (int)get_user_reg(&regs, ax);
>         return err;
>  }
>
> --
> 2.7.5
>
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu