[CRIU] [PATCH] compel: Do not loose sign of result in compat syscall

[Andrey Vagin]

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