NetBSD Problem Report #49919

From www@NetBSD.org  Fri May 22 10:09:37 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 94BE9A6551
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 22 May 2015 10:09:37 +0000 (UTC)
Message-Id: <20150522100936.4B28BA65CA@mollari.NetBSD.org>
Date: Fri, 22 May 2015 10:09:36 +0000 (UTC)
From: liuw@liuw.name
Reply-To: liuw@liuw.name
To: gnats-bugs@NetBSD.org
Subject: Bugs in xenevt.c
X-Send-Pr-Version: www-1.0

>Number:         49919
>Category:       port-xen
>Synopsis:       Bugs in xenevt.c
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    port-xen-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri May 22 10:10:07 +0000 2015
>Last-Modified:  Fri May 22 11:10:06 +0000 2015
>Originator:     Wei Liu
>Release:        NetBSD HEAD
>Organization:
>Environment:
>Description:
There are several bugs in xenevt.c (userspace event channel driver).

1. The critical region is too small in xenevt_fread.
2. Range check under IOCTL_EVTCHN_UNBIND should be ">=".
3. Range check under IOCTL_EVTCHN_NOTIFY should be ">=".
>How-To-Repeat:

>Fix:

>Audit-Trail:
From: Manuel Bouyer <bouyer@antioche.eu.org>
To: gnats-bugs@NetBSD.org
Cc: port-xen-maintainer@NetBSD.org, gnats-admin@NetBSD.org,
        netbsd-bugs@NetBSD.org
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 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.

 > 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().

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

From: Manuel Bouyer <bouyer@antioche.eu.org>
To: Wei Liu <liuw@liuw.name>
Cc: gnats-bugs@netbsd.org, port-xen-maintainer@netbsd.org,
        gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
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> wrote:
 > > The following reply was made to PR port-xen/49919; it has been noted by GNATS.
 > >
 > > From: Manuel Bouyer <bouyer@antioche.eu.org>
 > > To: gnats-bugs@NetBSD.org
 > > Cc: port-xen-maintainer@NetBSD.org, gnats-admin@NetBSD.org,
 > >         netbsd-bugs@NetBSD.org
 > > 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 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>
      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--

From: Wei Liu <liuw@liuw.name>
To: gnats-bugs@netbsd.org
Cc: port-xen-maintainer@netbsd.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org
Subject: Re: port-xen/49919: Bugs in xenevt.c
Date: Fri, 22 May 2015 11:42:34 +0100

 On Fri, May 22, 2015 at 11:35 AM, Manuel Bouyer <bouyer@antioche.eu.org> wrote:
 > The following reply was made to PR port-xen/49919; it has been noted by GNATS.
 >
 > From: Manuel Bouyer <bouyer@antioche.eu.org>
 > To: gnats-bugs@NetBSD.org
 > Cc: port-xen-maintainer@NetBSD.org, gnats-admin@NetBSD.org,
 >         netbsd-bugs@NetBSD.org
 > 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 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.

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

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

Home	PR Database Search