NetBSD Problem Report #48119

From www@NetBSD.org  Tue Aug 13 20:52:57 2013
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id EAE45710EF
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 13 Aug 2013 20:52:57 +0000 (UTC)
Message-Id: <20130813205256.CC2CE71304@mollari.NetBSD.org>
Date: Tue, 13 Aug 2013 20:52:56 +0000 (UTC)
From: brad.harder@gmail.com
Reply-To: brad.harder@gmail.com
To: gnats-bugs@NetBSD.org
Subject: /dev/random kernel crash
X-Send-Pr-Version: www-1.0

>Number:         48119
>Category:       kern
>Synopsis:       /dev/random kernel crash
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Aug 13 20:55:00 +0000 2013
>Closed-Date:    Tue May 31 04:47:02 +0000 2016
>Last-Modified:  Tue May 31 04:47:02 +0000 2016
>Originator:     brad harder
>Release:        -current/amd64
>Organization:
method logic digital
>Environment:
NetBSD kamloops 6.99.23 NetBSD 6.99.23 (GENERIC) #284: Mon Aug 12 13:04:28 PDT 2013  root@kamloops:/usr/obj/sys/arch/amd64/compile/GENERIC amd64
>Description:
dd if=/dev/random bs=8192 count=99  | sed 's,[^a-zA-Z0-9],,' panics kernel.

Once it happened "spontaneously", next try, issuing ^C caused the kernel to panic/reboot.


>How-To-Repeat:
dd if=/dev/random bs=8192 count=99 | sed 's/[^a-zA-Z0-9]//g'

>Fix:

>Release-Note:

>Audit-Trail:
From: B Harder <brad.harder@gmail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/48119: /dev/random kernel crash
Date: Wed, 14 Aug 2013 09:40:33 -0700

 From savecore (transcribed):
 reboot after panic: panic: kernel diagnositc assertion "(0 < n_read)
 || (ctx->rc_hard && ISSET(fp->f_flag, FNONBLOCK))" failed: file
 "/usr/src/sys/dev/rndpseudo.c", line 400


 On 8/13/13, gnats-admin@netbsd.org <gnats-admin@netbsd.org> wrote:
 > Thank you very much for your problem report.
 > It has the internal identification `kern/48119'.
 > The individual assigned to look at your
 > report is: kern-bug-people.
 >
 >>Category:       kern
 >>Responsible:    kern-bug-people
 >>Synopsis:       /dev/random kernel crash
 >>Arrival-Date:   Tue Aug 13 20:55:00 +0000 2013
 >
 >


 -- 
 Brad Harder
 Method Logic Digital Consulting
 http://www.methodlogic.net/
 http://twitter.com/bcharder

From: Sergio Lopez <slp@sinrega.org>
To: gnats-bugs@NetBSD.org
Cc: brad.harder@gmail.com
Subject: Re: kern/48119 (/dev/random kernel crash)
Date: Wed, 28 Aug 2013 07:37:31 +0000

 --tKW2IUtsqtDRztdT
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline

 rnd_read(rndpseudo.c) assumes cprng_strong will only return 0 when
 processing a non-blocking request, but as the latter can eventually
 sit on cv_wait_sig waiting for entropy, this will also happen when the
 request is interrupted, which is what is happening here.

 As as PoC, I've modified (see attached file) cprng_strong to return -1
 when cv_wait_sig is interrupted, and rnd_read to detect this and bail
 out. Probably, it would be better to extend cprng_strong so it returns
 the error code in addition to the number of bytes read.


 --tKW2IUtsqtDRztdT
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="pr48119.diff"

 Index: kern/subr_cprng.c
 ===================================================================
 RCS file: /cvsroot/src/sys/kern/subr_cprng.c,v
 retrieving revision 1.22
 diff -u -r1.22 subr_cprng.c
 --- kern/subr_cprng.c	27 Jul 2013 11:19:09 -0000	1.22
 +++ kern/subr_cprng.c	28 Aug 2013 07:36:29 -0000
 @@ -180,6 +180,7 @@
  size_t
  cprng_strong(struct cprng_strong *cprng, void *buffer, size_t bytes, int flags)
  {
 +	int error;
  	size_t result;

  	/* Caller must loop for more than CPRNG_MAX_LEN bytes.  */
 @@ -193,11 +194,16 @@
  	} else {
  		while (!cprng->cs_ready) {
  			if (ISSET(flags, FNONBLOCK) ||
 -			    !ISSET(cprng->cs_flags, CPRNG_USE_CV) ||
 -			    cv_wait_sig(&cprng->cs_cv, &cprng->cs_lock)) {
 +			    !ISSET(cprng->cs_flags, CPRNG_USE_CV)) {
  				result = 0;
  				goto out;
  			}
 +	
 +			error = cv_wait_sig(&cprng->cs_cv, &cprng->cs_lock);
 +			if (error) {
 +				result = -1;
 +				goto out;
 +			}
  		}
  	}

 Index: dev/rndpseudo.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/rndpseudo.c,v
 retrieving revision 1.16
 diff -u -r1.16 rndpseudo.c
 --- dev/rndpseudo.c	21 Jul 2013 22:30:19 -0000	1.16
 +++ dev/rndpseudo.c	28 Aug 2013 07:36:35 -0000
 @@ -375,6 +375,12 @@
  		    ((ctx->rc_hard && ISSET(fp->f_flag, FNONBLOCK))?
  			FNONBLOCK : 0));

 +		if (n_read == -1) {
 +			printf("rnd EINTR\n");
 +			error = EINTR;
 +			goto out;
 +		}
 +
  		/*
  		 * Equality will hold unless this is /dev/random, in
  		 * which case we get only as many bytes as are left

 --tKW2IUtsqtDRztdT--

From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: Thor Lancelot Simon <tls@NetBSD.org>
Subject: Re: kern/48119 (/dev/random kernel crash)
Date: Tue, 3 Sep 2013 13:17:04 +0000

 If someone has tested it and it works, the patch looks OK to me
 (remove the printf after testing before commit, of course), as a
 provisional measure until we split cprng_strong into two routines.
 When shoe-horning -1 into a size_t, I'd write an explicit cast in both
 cases to make it clear that we're doing something bogus, and perhaps
 add a comment to the effect that returning -1 is and provisional until
 we fix the API.

 Any comments, Thor?

From: Sergio Lopez <slp@sinrega.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48119 (/dev/random kernel crash)
Date: Wed, 4 Sep 2013 22:42:25 +0000

 On Tue, Sep 03, 2013 at 01:20:01PM +0000, Taylor R Campbell wrote:
 >  If someone has tested it and it works, the patch looks OK to me
 >  (remove the printf after testing before commit, of course), as a
 >  provisional measure until we split cprng_strong into two routines.
 >  When shoe-horning -1 into a size_t, I'd write an explicit cast in both
 >  cases to make it clear that we're doing something bogus, and perhaps
 >  add a comment to the effect that returning -1 is and provisional until
 >  we fix the API.

 You're right about the ugliness of stuffing -1 into a size_t. If this
 API change is expected to be applied in a near future, perhaps this fix
 can wait a little.

 Sergio.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48119 CVS commit: src/sys/dev
Date: Wed, 25 Sep 2013 03:14:55 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Sep 25 03:14:55 UTC 2013

 Modified Files:
 	src/sys/dev: rndpseudo.c

 Log Message:
 Fix spurious kassert on interrupted blocking read from /dev/random.

 Return EINTR in this case instead.  While here, clarify comment.

 Fixes PR kern/48119, simpler than the patch attached there, per
 discussion with tls@, who had this right in the earlier version
 of rndpseudo.c before I broke it (oops!).


 To generate a diff of this commit:
 cvs rdiff -u -r1.16 -r1.17 src/sys/dev/rndpseudo.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Tue, 31 May 2016 04:47:02 +0000
State-Changed-Why:
According to audit trail, this PR is fixed by riastradh's commit on
25 Sep 2013 03:14:55.  Please re-open if this problem recurs.


>Unformatted:

Home	PR Database Search