Re: port-xen/49919: Bugs in xenevt.c

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Fri, May 22, 2015 at 11:42:34AM +0100, Wei Liu wrote:
> On Fri, May 22, 2015 at 11:35 AM, Manuel Bouyer <bouyer%antioche.eu.org@localhost> wrote:
> > The following reply was made to PR port-xen/49919; it has been noted by GNATS.
> >
> > From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
> > To: gnats-bugs%NetBSD.org@localhost
> > Cc: port-xen-maintainer%NetBSD.org@localhost, gnats-admin%NetBSD.org@localhost,
> >         netbsd-bugs%NetBSD.org@localhost
> > Subject: Re: port-xen/49919: Bugs in xenevt.c
> > Date: Fri, 22 May 2015 12:32:00 +0200
> >
> >  On Fri, May 22, 2015 at 10:10:07AM +0000, liuw%liuw.name@localhost wrote:
> >  > 1. The critical region is too small in xenevt_fread.
> >
> >  Why do you think it's too small ? The code not covered by the
> >  lock only manipulates local (on-stack) variables.
> >
> 
> Multiple concurrent readers reading the same instance.
> 
> Reading while as the same time doing IOCTL_EVTCHN_RESET.
> 
> The main concern is that d->ring_read is updated in second critical
> region in that function. Another thread can come in between the gap
> and manipulate those indices.

Yes, that's possible, although I don't think at this time there are mutiple
userland readers.

The attached patch should fix the problem: it detects if ring_read
was changed while we didn't hold the lock, and if so, don't update the
read pointer and return an error.


> 
> >  > 2. Range check under IOCTL_EVTCHN_UNBIND should be ">=".
> >  > 3. Range check under IOCTL_EVTCHN_NOTIFY should be ">=".
> >
> >  Right, there's a off-by-one error. There's also one in xenevt_fwrite().
> >
> 
> You mean the nentries check? I think that's OK because it's the number
> of entries not index to array.

Yes, this one is harmless but it's better to be consistent. The intent was
to now allow more entries than NR_EVENT_CHANNELS (which would't make
sense anyway)

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: xenevt.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/xenevt.c,v
retrieving revision 1.42
diff -u -p -u -r1.42 xenevt.c
--- xenevt.c	22 May 2015 10:34:13 -0000	1.42
+++ xenevt.c	22 May 2015 11:05:46 -0000
@@ -416,13 +416,13 @@ xenevt_fread(struct file *fp, off_t *off
     kauth_cred_t cred, int flags)
 {
 	struct xenevt_d *d = fp->f_data;
-	int error, ring_read, ring_write;
+	int error, ring_read, oring_read, ring_write;
 	size_t len, uio_len;
 
 	error = 0;
 	mutex_enter(&d->lock);
 	while (error == 0) {
-		ring_read = d->ring_read;
+		oring_read = ring_read = d->ring_read;
 		ring_write = d->ring_write;
 		if (ring_read != ring_write) {
 			break;
@@ -472,10 +472,15 @@ xenevt_fread(struct file *fp, off_t *off
 
 done:
 	mutex_enter(&d->lock);
-	d->ring_read = ring_read;
+	if (d->ring_read == oring_read) {
+		d->ring_read = ring_read;
+		error = 0;
+	} else {
+		error = EAGAIN;
+	}
 	mutex_exit(&d->lock);
 
-	return 0;
+	return error;
 }
 
 static int