linux-kernel - Re: [PATCH] xen: xenfs: privcmd: check put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1288375034.8069.66.camel@localhost.localdomain>
Date:	Fri, 29 Oct 2010 18:57:14 +0100
From:	Ian Campbell <Ian.Campbell@...citrix.com>
To:	Jeremy Fitzhardinge <jeremy@...p.org>
CC:	Vasiliy Kulikov <segooon@...il.com>,
	"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
	Jeremy Fitzhardinge <Jeremy.Fitzhardinge@...rix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"virtualization@...ts.osdl.org" <virtualization@...ts.osdl.org>
Subject: Re: [PATCH] xen: xenfs: privcmd: check put_user() return code

On Fri, 2010-10-29 at 18:52 +0100, Jeremy Fitzhardinge wrote:
> On 10/29/2010 10:44 AM, Ian Campbell wrote:
> > On Fri, 2010-10-29 at 18:18 +0100, Jeremy Fitzhardinge wrote:
> >> On 10/28/2010 04:39 AM, Vasiliy Kulikov wrote:
> >>> put_user() may fail.  In this case propagate error code from
> >>> privcmd_ioctl_mmap_batch().
> >> Thanks for looking at this.  I'm in two minds about this; the existing
> >> logic is such that these put_users can only fail if something else has
> >> already failed and its returning an error.  I guess it would be useful
> >> to get an EFAULT if you've got a problem writing back the results.
> >>
> >> IanC, any opinion?
> > Not a strong one.
> >
> > Perhaps what we really want in this case is for traverse_pages to return
> > the total number of callback failures it encountered rather than
> > aborting after the first failure?
> >
> > On the other hand you are correct that gather_array() has already
> > touched all the pages which we are going to be touching here so how
> > likely is a new failure at this point anyway?
> 
> I could think of two cases: the array is mapped RO, so only the
> writeback fails, or someone changes the mapping under our feet from
> another thread.

I guess in that case returning EFAULT and leaving the array in whatever
partially updated state we got to is the right thing to do and the
proposed patch is therefore correct.

> 
>     J
> 
> > Ian.
> >
> >> Thanks,
> >>     J
> >>
> >>> Signed-off-by: Vasiliy Kulikov <segooon@...il.com>
> >>> ---
> >>>  Compile tested.
> >>>
> >>>  drivers/xen/xenfs/privcmd.c |    8 ++------
> >>>  1 files changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/xen/xenfs/privcmd.c b/drivers/xen/xenfs/privcmd.c
> >>> index f80be7f..2eb04c8 100644
> >>> --- a/drivers/xen/xenfs/privcmd.c
> >>> +++ b/drivers/xen/xenfs/privcmd.c
> >>> @@ -266,9 +266,7 @@ static int mmap_return_errors(void *data, void *state)
> >>>  	xen_pfn_t *mfnp = data;
> >>>  	struct mmap_batch_state *st = state;
> >>>  
> >>> -	put_user(*mfnp, st->user++);
> >>> -
> >>> -	return 0;
> >>> +	return put_user(*mfnp, st->user++);
> >>>  }
> >>>  
> >>>  static struct vm_operations_struct privcmd_vm_ops;
> >>> @@ -323,10 +321,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata)
> >>>  	up_write(&mm->mmap_sem);
> >>>  
> >>>  	if (state.err > 0) {
> >>> -		ret = 0;
> >>> -
> >>>  		state.user = m.arr;
> >>> -		traverse_pages(m.num, sizeof(xen_pfn_t),
> >>> +		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> >>>  			       &pagelist,
> >>>  			       mmap_return_errors, &state);
> >>>  	}
> >
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/