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

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

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: Wei Liu <liuw%liuw.name@localhost>
Cc: gnats-bugs%netbsd.org@localhost, 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 13:08:11 +0200

 --ew6BAiZeqk4r7MaW
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 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
 --
 
 --ew6BAiZeqk4r7MaW
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename=diff
 
 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
 
 --ew6BAiZeqk4r7MaW--