NetBSD Problem Report #23756

Received: (qmail 24175 invoked by uid 605); 14 Dec 2003 20:06:55 -0000
Message-Id: <20031214200651.0ACB411172@narn.netbsd.org>
Date: Sun, 14 Dec 2003 20:06:51 +0000 (UTC)
From: auto92089@hushmail.com
Sender: gnats-bugs-owner@NetBSD.org
Reply-To: auto92089@hushmail.com
To: gnats-bugs@gnats.NetBSD.org
Subject: /dev/random should block on writes if the entropy pool is full
X-Send-Pr-Version: www-1.0

>Number:         23756
>Category:       security
>Synopsis:       /dev/random should block on writes if the entropy pool is full
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    security-officer
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sun Dec 14 20:07:00 +0000 2003
>Closed-Date:    
>Last-Modified:  Sun Aug 08 20:13:35 +0000 2004
>Originator:     Ian Cognito
>Release:        NetBSD-1.5.2
>Organization:
>Environment:
n/a
>Description:
I have a little RNG interface application that shuttles bits
from a serial port device to /dev/random.  It would be nice to be able
to stop drawing entropy from the device so it could power down whenever
there is enough in the system (it is battery operated to eliminate 60Hz
signal injection).  So I have it query the entropy level, but that is
polling and not blocking I/O.  So I think that /dev/random should block
on writes when the entropy pool is full, in the same way that it blocks
on reads when it is nearly empty.  One could continue to use urandom if
you wanted to have the old behavior.

For the code in question, see: <URL:http://atom_age.tripod.com/>
>How-To-Repeat:

>Fix:
I'll write a patch if you don't do it first.
(I'm a little out of date on my kernel sources)

This is really a no-brainer though, you can almost cut and paste the
relevant lines from rndread.

>Release-Note:
>Audit-Trail:
State-Changed-From-To: open->feedback 
State-Changed-By: mrg 
State-Changed-When: Fri Apr 2 11:21:21 UTC 2004 
State-Changed-Why:  
this is a good idea- please send a patch! 

From: vax@carolina.rr.com
To: gnats-bugs@gnats.netbsd.org
Cc:  
Subject: security/23756
Date: Wed, 28 Apr 2004 19:52:39 -0400 (EDT)

 >Submitter-Id:	net
 >Originator:	VaX#n8
 >Organization:

 >Confidential:	no 
 >Synopsis:	/dev/random now blocks on write when pool full
 >Severity:	non-critical 
 >Priority:	medium 
 >Category:	security
 >Class:		change-request
 >Release:	NetBSD 1.6.2


 >Environment:


 Architecture: i386
 Machine: i386
 >Description:

 Well if you've got a HW RNG and you want it to fill /dev/random as needed,
 and that RNG runs off of batteries so you only want to use it when you need
 it, it'd be nice if write(2)s blocked when the pool was full.

 This is the first time I've done anything like this so please look it over
 carefully.  I'm a little unclear on spl requirements and sleep/wakeups,
 and I was unclear on poll is implemented.

 Oh, and what is timeout?  I'm not clear on whether it should wake up writers
 or not.

 I chose not to rename the read rnd_selq but I think it should be renamed to
 rnd_read_selq for consistency.

 >How-To-Repeat:

 >Fix:


 Index: NetBSD/sys/dev/rnd.c
 diff -u NetBSD/sys/dev/rnd.c:1.1.1.1 NetBSD/sys/dev/rnd.c:1.2
 --- NetBSD/sys/dev/rnd.c:1.1.1.1	Thu Apr  8 13:08:46 2004
 +++ NetBSD/sys/dev/rnd.c	Wed Apr 28 19:45:04 2004
 @@ -118,11 +118,19 @@
   * our select/poll queue
   */
  struct selinfo rnd_selq;
 +struct selinfo rnd_write_selq;

  /*
   * Set when there are readers blocking on data from us
   */
  #define	RND_READWAITING 0x00000001
 +/*
 + * This is set when there are writers blocking for space.
 + */
 +#define RND_WRITEWAITING 0x00000002
 +/*
 + * This is an OR of the above conditions.
 + */
  volatile u_int32_t rnd_status;

  /*
 @@ -163,6 +171,7 @@
  int	rndpoll __P((dev_t, int, struct proc *));

  static inline void	rnd_wakeup_readers(void);
 +static inline void	rnd_wakeup_writers(void);
  static inline u_int32_t rnd_estimate_entropy(rndsource_t *, u_int32_t);
  static inline u_int32_t rnd_counter(void);
  static        void	rnd_timeout(void *);
 @@ -220,6 +229,33 @@
  }

  /*
 + * Check to see if there are writers waiting on us.  If so, kick them.
 + *
 + * Must be called at splsoftclock().
 + */
 +static inline void
 +rnd_wakeup_writers(void)
 +{
 +
 +	/*
 +	 * If we have removed new bits, and now have space to fill,
 +	 * wake up sleeping writers.
 +	 */
 +	if (rndpool_get_entropy_count(&rnd_pool) <= (RND_POOLBITS - 8)) {
 +		if (rnd_status & RND_WRITEWAITING) {
 +			DPRINTF(RND_DEBUG_SNOOZE,
 +			    ("waking up pending writers.\n"));
 +			rnd_status &= ~RND_WRITEWAITING;
 +			wakeup(&rnd_write_selq);
 +		}
 +		selwakeup(&rnd_write_selq);
 +
 +	}
 +}
 +
 +
 +
 +/*
   * Use the timing of the event to estimate the entropy gathered.
   * If all the differentials (first, second, and third) are non-zero, return
   * non-zero.  If any of these are zero, return zero.
 @@ -425,7 +461,8 @@
  rndwrite(dev_t dev, struct uio *uio, int ioflag)
  {
  	u_int8_t *buf;
 -	int n, ret, s;
 +	u_int32_t entcnt;
 +	int mode, n, ret, s;

  	DPRINTF(RND_DEBUG_WRITE,
  	    ("Random: Write of %d requested\n", uio->uio_resid));
 @@ -433,6 +470,18 @@
  	if (uio->uio_resid == 0)
  		return (0);

 +	switch (minor(dev)) {
 +	case RND_DEV_RANDOM:
 +		mode = RND_INJECT_IF_NEEDED;
 +		break;
 +	case RND_DEV_URANDOM:
 +		mode = RND_INJECT_ALWAYS;
 +		break;
 +	default:
 +		/* Can't happen, but this is cheap */
 +		return (ENXIO);
 +	}
 +
  	ret = 0;

  	buf = malloc(RND_TEMP_BUFFER_SIZE, M_TEMP, M_WAITOK);
 @@ -440,6 +489,43 @@
  	while (uio->uio_resid > 0) {
  		n = min(RND_TEMP_BUFFER_SIZE, uio->uio_resid);

 +		/*
 +		 * When injecting "if needed" with a blocking write,
 +		 * write enough to fill the pool and then sleep.
 +		 */
 +
 +		for (;;) {
 +			/*
 +			 * If using /dev/urandom, don't block.
 +			 */
 +			if (mode == RND_INJECT_ALWAYS)
 +				break;
 +
 +			/*
 +			 * How much entropy do we need?
 +			 */
 +			s = splsoftclock();
 +			entcnt = rndpool_get_entropy_count(&rnd_pool);
 +			splx(s);
 +			if (entcnt <= (RND_POOLBITS - 8))
 +				break;
 +
 +			/*
 +			 * A full byte of entropy is not needed.
 +			 */
 +			if (ioflag & IO_NDELAY) {
 +				ret = EWOULDBLOCK;
 +				goto out;
 +			}
 +
 +			rnd_status |= RND_WRITEWAITING;
 +			ret = tsleep(&rnd_write_selq, PRIBIO|PCATCH,
 +			    "rndwrite", 0);
 +
 +			if (ret)
 +				goto out;
 +		}
 +
  		ret = uiomove((caddr_t)buf, n, uio);
  		if (ret != 0)
  			break;
 @@ -454,6 +540,7 @@
  		DPRINTF(RND_DEBUG_WRITE, ("Random: Copied in %d bytes\n", n));
  	}

 +out:
  	free(buf, M_TEMP);
  	return (ret);
  }
 @@ -635,21 +722,12 @@
  	int revents, s;

  	/*
 -	 * We are always writable.
 -	 */
 -	revents = events & (POLLOUT | POLLWRNORM);
 -
 -	/*
 -	 * Save some work if not checking for reads.
 -	 */
 -	if ((events & (POLLIN | POLLRDNORM)) == 0)
 -		return (revents);
 -
 -	/*
 -	 * If the minor device is not /dev/random, we are always readable.
 +	 * If the minor device is not /dev/random, we are always
 +	 * readable and writable.
  	 */
  	if (minor(dev) != RND_DEV_RANDOM) {
 -		revents |= events & (POLLIN | POLLRDNORM);
 +		revents |= events & (POLLIN | POLLRDNORM \
 +				     | POLLOUT | POLLWRNORM);
  		return (revents);
  	}

 @@ -660,10 +738,19 @@
  	entcnt = rndpool_get_entropy_count(&rnd_pool);
  	splx(s);

 -	if (entcnt >= RND_ENTROPY_THRESHOLD * 8)
 -		revents |= events & (POLLIN | POLLRDNORM);
 -	else
 -		selrecord(p, &rnd_selq);
 +	if (events & (POLLIN | POLLRDNORM)) {
 +		if (entcnt >= RND_ENTROPY_THRESHOLD * 8)
 +			revents |= events & (POLLIN | POLLRDNORM);
 +		else
 +			selrecord(p, &rnd_selq);
 +	}
 +
 +	if (events & (POLLOUT | POLLWRNORM)) {
 +		if (entcnt <= (RND_POOLBITS - 8))
 +			revents |= events & (POLLOUT | POLLWRNORM);
 +		else
 +			selrecord(p, &rnd_write_selq);
 +	}

  	return (revents);
  }
 @@ -965,6 +1052,10 @@

  	s = splsoftclock();
  	retval = rndpool_extract_data(&rnd_pool, p, len, flags);
 +	/*
 +	 * Wake up any potential writers waiting.
 +	 */
 +	rnd_wakeup_writers();
  	splx(s);

  	return (retval);

From: VaX#n8 <vax@carolina.rr.com>
To: gnats-bugs@NetBSD.org
Cc:  
Subject: security/23756
Date: Wed, 28 Apr 2004 19:58:25 -0400

 Oh forgot another part:

 Index: NetBSD/sys/sys/rnd.h
 diff -u NetBSD/sys/sys/rnd.h:1.1.1.1 NetBSD/sys/sys/rnd.h:1.2
 --- NetBSD/sys/sys/rnd.h:1.1.1.1	Thu Apr  8 13:11:45 2004
 +++ NetBSD/sys/sys/rnd.h	Wed Apr 28 19:56:43 2004
 @@ -140,6 +140,12 @@
  #define	RND_EXTRACT_GOOD	1  /* return as many good bytes
  				      (short read ok) */

 +/*
 + * Used by rndwrite to describe whether to block on writes or not.
 + */
 +#define RND_INJECT_ALWAYS	0  /* always add to pool, even if it's full */
 +#define RND_INJECT_IF_NEEDED	1  /* write as many bytes as needed to fill */
 +
  void		rndpool_init __P((rndpool_t *));
  void		rndpool_init_global __P((void));
  u_int32_t	rndpool_get_entropy_count __P((rndpool_t *));

From: VaX#n8 <vax@carolina.rr.com>
To: gnats-bugs@gnats.netbsd.org
Cc:  
Subject: Re: security/23756
Date: Thu, 29 Apr 2004 19:16:56 -0400

 Um, I don't exactly know why I wanted writes to block, because they don't
 actually add any entropy to the pool.  What I really wanted was for
 the RNDADDDATA ioctl to block.  So this patch adds blocking functionality
 where it matters.  I don't see much harm in keeping the prior patch, but
 it's up to you.  Also I'm not sure whether to submit this as an incremental
 or cumulative patch, so this is incremental.

 Again, I'm not really familiar with the kernel so please look this over,
 especially regarding deadlocks and interrupt masking and that kind of
 concurrent programming sort of stuff.

 It behaves the way I want under normal use.

 Index: NetBSD/sys/dev/rnd.c
 diff -u NetBSD/sys/dev/rnd.c:1.2 NetBSD/sys/dev/rnd.c:1.3
 --- NetBSD/sys/dev/rnd.c:1.2	Wed Apr 28 19:45:04 2004
 +++ NetBSD/sys/dev/rnd.c	Thu Apr 29 06:59:54 2004
 @@ -553,8 +553,8 @@
  	rndstat_name_t *rstnm;
  	rndctl_t *rctl;
  	rnddata_t *rnddata;
 -	u_int32_t count, start;
 -	int ret, s;
 +	u_int32_t count, entcnt, start;
 +	int mode, ret, s;

  	ret = 0;

 @@ -697,6 +697,47 @@
  		if ((ret = suser(p->p_ucred, &p->p_acflag)) != 0)
  			return (ret);

 +		switch (minor(dev)) {
 +		case RND_DEV_RANDOM:
 +			mode = RND_INJECT_IF_NEEDED;
 +			break;
 +		case RND_DEV_URANDOM:
 +			mode = RND_INJECT_ALWAYS;
 +			break;
 +		default:
 +			/* Can't happen, but this is cheap */
 +			return (ENXIO);
 +		}
 +
 +		/*
 +		 * When injecting "if needed", inject enough to fill the pool
 +		 * and then sleep.
 +		 */
 +
 +		for (;;) {
 +
 +			/*
 +			 * If using /dev/urandom, don't block.
 +			 */
 +			if (mode == RND_INJECT_ALWAYS)
 +				break;
 +
 +			/*
 +			 * How much entropy do we need?
 +			 */
 +			s = splsoftclock();
 +			entcnt = rndpool_get_entropy_count(&rnd_pool);
 +			splx(s);
 +			if (entcnt <= (RND_POOLBITS - 8))
 +				break;
 +
 +			rnd_status |= RND_WRITEWAITING;
 +			ret = tsleep(&rnd_write_selq, PRIBIO|PCATCH,
 +				     "rndwrite", 0);
 +			if (ret)
 +				goto out;
 +		}
 +
  		rnddata = (rnddata_t *)addr;

  		s = splsoftclock();
 @@ -711,7 +752,7 @@
  	default:
  		return (EINVAL);
  	}
 -
 +out:
  	return (ret);
  }


From: VaX#n8 <vax@carolina.rr.com>
To: gnats-bugs@netbsd.org
Cc:  
Subject: Re: security/23756 renaming rnd_selq for consistency
Date: Sun, 04 Jul 2004 08:46:13 -0400

 This patch accomplishes a renaming of rnd.c's internal variables to
 make them more clearly parallel.

 Index: rnd.c
 ===================================================================
 RCS file: /usr/local/share/cvsroot/NetBSD/sys/dev/rnd.c,v
 retrieving revision 1.3
 diff -u -r1.3 rnd.c
 --- rnd.c	29 Apr 2004 10:59:54 -0000	1.3
 +++ rnd.c	4 Jul 2004 12:41:59 -0000
 @@ -117,7 +117,7 @@
  /*
   * our select/poll queue
   */
 -struct selinfo rnd_selq;
 +struct selinfo rnd_read_selq;
  struct selinfo rnd_write_selq;

  /*
 @@ -217,9 +217,9 @@
  			DPRINTF(RND_DEBUG_SNOOZE,
  			    ("waking up pending readers.\n"));
  			rnd_status &= ~RND_READWAITING;
 -			wakeup(&rnd_selq);
 +			wakeup(&rnd_read_selq);
  		}
 -		selwakeup(&rnd_selq);
 +		selwakeup(&rnd_read_selq);

  		/*
  		 * Allow open of /dev/random now, too.
 @@ -433,7 +433,7 @@
  			}

  			rnd_status |= RND_READWAITING;
 -			ret = tsleep(&rnd_selq, PRIBIO|PCATCH,
 +			ret = tsleep(&rnd_read_selq, PRIBIO|PCATCH,
  			    "rndread", 0);

  			if (ret)
 @@ -783,7 +783,7 @@
  		if (entcnt >= RND_ENTROPY_THRESHOLD * 8)
  			revents |= events & (POLLIN | POLLRDNORM);
  		else
 -			selrecord(p, &rnd_selq);
 +			selrecord(p, &rnd_read_selq);
  	}

  	if (events & (POLLOUT | POLLWRNORM)) {

State-Changed-From-To: feedback->open 
State-Changed-By: fair 
State-Changed-When: Sun Aug 8 20:13:01 UTC 2004 
State-Changed-Why:  

Feedback provided. 

>Unformatted:

Home	PR Database Search