NetBSD Problem Report #59148

From www@netbsd.org  Thu Mar  6 14:35:53 2025
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 8BD621A9239
	for <gnats-bugs@gnats.NetBSD.org>; Thu,  6 Mar 2025 14:35:53 +0000 (UTC)
Message-Id: <20250306143551.B69141A923C@mollari.NetBSD.org>
Date: Thu,  6 Mar 2025 14:35:51 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: arc4random calls malloc so it can't be used in an ELF constructor
X-Send-Pr-Version: www-1.0

>Number:         59148
>Category:       lib
>Synopsis:       arc4random calls malloc so it can't be used in an ELF constructor
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 06 14:40:00 +0000 2025
>Closed-Date:    
>Last-Modified:  Tue Mar 11 14:35:01 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current, 10
>Organization:
The Arc4BSD Constrepochtor
>Environment:
>Description:
For PR kern/58632: getentropy(2) and arc4random(3) do not reseed on VM fork <https://gnats.NetBSD.org/58632>, we exposed the entropy epoch to userland and taught arc4random(3) to query it.

Unfortunately, this had the side effect of creating a path from arc4random(3) to malloc(3), which means it can't be used in ELF constructors, ifunc selectors, and similar contexts.  The path goes via sysctlnametomib(3), which uses malloc(3) to handle unbounded memory allocation, as required by the sysctl(2) interface -- an unfortunate requirement, chronicled in PR kern/59147: sysctl: bounded-memory lookups by name <https://gnats.NetBSD.org/59147>.  And kern.* is fairly hefty to list, requiring tens of kilobytes of struct sysctlnodes.
>How-To-Repeat:
call arc4random in an elf constructor and roll the dice to see if sparks fly (not guaranteed, you might get lucky and malloc initialization might happen first)
>Fix:
Checking the entropy epoch, and reseeding if it has changed, is an important security property, so simply reverting the changes for PR 58632 is a nonstarter.

Perhaps, until we either resolve PR kern/59147 by implementing a path for bounded-memory lookup by name, or implement vDSO and expose the entropy epoch through that, we should use an ELF constructor for arc4random to resolve the sysctl when the stack is nearly empty so eating tens of kilobytes out of it isn't that big a deal, like we did for PR port-arm/59147: libc constructors on arm use malloc <https://gnats.NetBSD.org/59147>.

Or, we could just statically assign KERN_ENTROPY and KERN_ENTROPY_EPOCH numbers and skip the runtime name resolution altogether.

>Release-Note:

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: netbsd-bugs@NetBSD.org
Subject: Re: lib/59148: arc4random calls malloc so it can't be used in an ELF constructor
Date: Sat, 8 Mar 2025 22:56:38 +0000

 This is a multi-part message in MIME format.
 --=_zuuYtksK0C8RJaF9J8AwVVP/KZSlw/L+

 The attached patches address this in two alternative ways:

 1. [pr59148-arc4randomsysctlqueryconstructor.patch] Resolve the sysctl
    kern.entropy.epoch mib in an ELF constructor, where the stack usage
    is fairly shallow so it's not too onerous to allocate >12KB(!) on
    the stack for listing the kern.* children.  I'm not really happy
    about this because it incurs the cost for all applications, even
    those that don't call arc4random, because it does the work in a
    constructor.

 2. [pr59148-arc4randomsysctlstaticnum.patch] Statically assign sysctl
    mib numbers to kern.entropy.epoch, so userland can just query
    {CTL_KERN, KERN_ENTROPY, KERN_ENTROPY_EPOCH}.  It's annoying to
    have to statically assign new numbers but not really that big a
    deal and I think I'm willing to argue that kern.entropy.epoch is a
    long-term useful thing that is worth committing to.

 Ideally, we would eliminate this whole sysctl lookup path in favour of
 reading a single word out of a vDSO mapped in userland, but that
 requires the whole vDSO machinery which we don't have yet.  I'm
 leaning toward (2).

 --=_zuuYtksK0C8RJaF9J8AwVVP/KZSlw/L+
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr59148-arc4randomsysctlqueryconstructor"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr59148-arc4randomsysctlqueryconstructor.patch"

 # HG changeset patch
 # User Taylor R Campbell <riastradh@NetBSD.org>
 # Date 1741308823 0
 #      Fri Mar 07 00:53:43 2025 +0000
 # Branch trunk
 # Node ID 1c29ada773b0a0e9ad17221151d839be0e711eb9
 # Parent  e3b67aeee565934995180b3b1a684fa2ea3c47b4
 # EXP-Topic riastradh-pr59117-arc4randomfail
 WIP: arc4random(3): Resolve kern.entropy.epoch sysctl without malloc.

 PR lib/59148: arc4random calls malloc so it can't be used in an ELF
 constructor

 diff -r e3b67aeee565 -r 1c29ada773b0 lib/libc/gen/arc4random.c
 --- a/lib/libc/gen/arc4random.c	Fri Mar 07 01:22:46 2025 +0000
 +++ b/lib/libc/gen/arc4random.c	Fri Mar 07 00:53:43 2025 +0000
 @@ -64,7 +64,6 @@
 =20
  #include <assert.h>
  #include <sha2.h>
 -#include <stdatomic.h>
  #include <stdbool.h>
  #include <stdint.h>
  #include <stdlib.h>
 @@ -480,6 +479,79 @@ crypto_onetimestream_selftest(void)
  	return 0;
  }
 =20
 +static int kern_entropy_epoch_mib[3];
 +
 +__attribute__((constructor))
 +__section(".text.startup")
 +static void
 +entropy_epoch_init(void)
 +{
 +	struct sysctlnode query, nodes[128]; /* XXX 12k of stack space */
 +	size_t i, len;
 +
 +	/*
 +	 * Before constructors have run, this may be called multiple
 +	 * times in case a caller uses arc4random from a constructor --
 +	 * but only from a single thread, so no synchronization is
 +	 * needed.
 +	 *
 +	 * After constructors have run, kern_entropy_epoch_mib is
 +	 * stable, because this is one of the constructors.
 +	 */
 +	if (__predict_true(kern_entropy_epoch_mib[0] !=3D 0))
 +		return;
 +
 +	/*
 +	 * Start with CTL_KERN which is statically assigned.
 +	 */
 +	kern_entropy_epoch_mib[0] =3D CTL_KERN;
 +
 +	/*
 +	 * Resolve kern.entropy.
 +	 */
 +	kern_entropy_epoch_mib[1] =3D CTL_QUERY;
 +	memset(&query, 0, sizeof(query));
 +	query.sysctl_flags =3D SYSCTL_VERSION;
 +	len =3D sizeof(nodes);
 +	if (sysctl(kern_entropy_epoch_mib, 2, nodes, &len,
 +		&query, sizeof(query)) =3D=3D -1)
 +		goto fail;
 +	for (i =3D 0; i < len/sizeof(nodes[0]); i++) {
 +		if (strcmp(nodes[i].sysctl_name, "entropy") =3D=3D 0) {
 +			kern_entropy_epoch_mib[1] =3D nodes[i].sysctl_num;
 +			break;
 +		}
 +	}
 +	if (i =3D=3D len/sizeof(nodes[0]))
 +		goto fail;
 +
 +	/*
 +	 * Resolve kern.entropy.epoch.
 +	 */
 +	kern_entropy_epoch_mib[2] =3D CTL_QUERY;
 +	memset(&query, 0, sizeof(query));
 +	query.sysctl_flags =3D SYSCTL_VERSION;
 +	len =3D sizeof(nodes);
 +	if (sysctl(kern_entropy_epoch_mib, 3, nodes, &len,
 +		&query, sizeof(query)) =3D=3D -1)
 +		goto fail;
 +	for (i =3D 0; i < len/sizeof(nodes[0]); i++) {
 +		if (strcmp(nodes[i].sysctl_name, "epoch") =3D=3D 0) {
 +			kern_entropy_epoch_mib[2] =3D nodes[i].sysctl_num;
 +			break;
 +		}
 +	}
 +	if (i =3D=3D len/sizeof(nodes[0]))
 +		goto fail;
 +
 +	/*
 +	 * Success!
 +	 */
 +	return;
 +
 +fail:	kern_entropy_epoch_mib[0] =3D CTL_EOL;
 +}
 +
  /*
   * entropy_epoch()
   *
 @@ -499,36 +571,15 @@ crypto_onetimestream_selftest(void)
  static unsigned
  entropy_epoch(void)
  {
 -	static atomic_int mib0[3];
 -	static atomic_bool initialized =3D false;
 -	int mib[3];
  	unsigned epoch =3D (unsigned)-1;
  	size_t epochlen =3D sizeof(epoch);
 =20
 -	/*
 -	 * Resolve kern.entropy.epoch if we haven't already.  Cache it
 -	 * for the next caller.  Initialization is idempotent, so it's
 -	 * OK if two threads do it at once.
 -	 */
 -	if (atomic_load_explicit(&initialized, memory_order_acquire)) {
 -		mib[0] =3D atomic_load_explicit(&mib0[0], memory_order_relaxed);
 -		mib[1] =3D atomic_load_explicit(&mib0[1], memory_order_relaxed);
 -		mib[2] =3D atomic_load_explicit(&mib0[2], memory_order_relaxed);
 -	} else {
 -		size_t nmib =3D __arraycount(mib);
 -
 -		if (sysctlnametomib("kern.entropy.epoch", mib, &nmib) =3D=3D -1)
 -			return (unsigned)-1;
 -		if (nmib !=3D __arraycount(mib))
 -			return (unsigned)-1;
 -		atomic_store_explicit(&mib0[0], mib[0], memory_order_relaxed);
 -		atomic_store_explicit(&mib0[1], mib[1], memory_order_relaxed);
 -		atomic_store_explicit(&mib0[2], mib[2], memory_order_relaxed);
 -		atomic_store_explicit(&initialized, true,
 -		    memory_order_release);
 -	}
 -
 -	if (sysctl(mib, __arraycount(mib), &epoch, &epochlen, NULL, 0) =3D=3D -1)
 +	entropy_epoch_init();
 +	if (kern_entropy_epoch_mib[0] =3D=3D CTL_EOL)
 +		return (unsigned)-1;
 +	if (sysctl(kern_entropy_epoch_mib,
 +		__arraycount(kern_entropy_epoch_mib),
 +		&epoch, &epochlen, NULL, 0) =3D=3D -1)
  		return (unsigned)-1;
  	if (epochlen !=3D sizeof(epoch))
  		return (unsigned)-1;

 --=_zuuYtksK0C8RJaF9J8AwVVP/KZSlw/L+
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr59148-arc4randomsysctlstaticnum"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr59148-arc4randomsysctlstaticnum.patch"

 # HG changeset patch
 # User Taylor R Campbell <riastradh@NetBSD.org>
 # Date 1741314350 0
 #      Fri Mar 07 02:25:50 2025 +0000
 # Branch trunk
 # Node ID c8aac91a801fc586d432bf9164a68934a0902580
 # Parent  e3b67aeee565934995180b3b1a684fa2ea3c47b4
 # EXP-Topic riastradh-pr59117-arc4randomfail
 WIP: Assign static MIB numbers for kern.entropy.epoch.

 This sidesteps any need for dynamically sized memory in userland to
 resolve sysctl names to read it out, or for a new syscall interface
 to sysctl resolution by name.

 It would really be better to expose this through a page shared with
 userland, so querying it doesn't cost a syscall, but this will serve
 for now.

 PR lib/59148: arc4random calls malloc so it can't be used in an ELF
 constructor

 diff -r e3b67aeee565 -r c8aac91a801f lib/libc/gen/arc4random.c
 --- a/lib/libc/gen/arc4random.c	Fri Mar 07 01:22:46 2025 +0000
 +++ b/lib/libc/gen/arc4random.c	Fri Mar 07 02:25:50 2025 +0000
 @@ -64,7 +64,6 @@
 =20
  #include <assert.h>
  #include <sha2.h>
 -#include <stdatomic.h>
  #include <stdbool.h>
  #include <stdint.h>
  #include <stdlib.h>
 @@ -499,35 +498,10 @@ crypto_onetimestream_selftest(void)
  static unsigned
  entropy_epoch(void)
  {
 -	static atomic_int mib0[3];
 -	static atomic_bool initialized =3D false;
 -	int mib[3];
 +	const int mib[] =3D { CTL_KERN, KERN_ENTROPY, KERN_ENTROPY_EPOCH };
  	unsigned epoch =3D (unsigned)-1;
  	size_t epochlen =3D sizeof(epoch);
 =20
 -	/*
 -	 * Resolve kern.entropy.epoch if we haven't already.  Cache it
 -	 * for the next caller.  Initialization is idempotent, so it's
 -	 * OK if two threads do it at once.
 -	 */
 -	if (atomic_load_explicit(&initialized, memory_order_acquire)) {
 -		mib[0] =3D atomic_load_explicit(&mib0[0], memory_order_relaxed);
 -		mib[1] =3D atomic_load_explicit(&mib0[1], memory_order_relaxed);
 -		mib[2] =3D atomic_load_explicit(&mib0[2], memory_order_relaxed);
 -	} else {
 -		size_t nmib =3D __arraycount(mib);
 -
 -		if (sysctlnametomib("kern.entropy.epoch", mib, &nmib) =3D=3D -1)
 -			return (unsigned)-1;
 -		if (nmib !=3D __arraycount(mib))
 -			return (unsigned)-1;
 -		atomic_store_explicit(&mib0[0], mib[0], memory_order_relaxed);
 -		atomic_store_explicit(&mib0[1], mib[1], memory_order_relaxed);
 -		atomic_store_explicit(&mib0[2], mib[2], memory_order_relaxed);
 -		atomic_store_explicit(&initialized, true,
 -		    memory_order_release);
 -	}
 -
  	if (sysctl(mib, __arraycount(mib), &epoch, &epochlen, NULL, 0) =3D=3D -1)
  		return (unsigned)-1;
  	if (epochlen !=3D sizeof(epoch))
 diff -r e3b67aeee565 -r c8aac91a801f sys/kern/kern_entropy.c
 --- a/sys/kern/kern_entropy.c	Fri Mar 07 01:22:46 2025 +0000
 +++ b/sys/kern/kern_entropy.c	Fri Mar 07 02:25:50 2025 +0000
 @@ -358,7 +358,7 @@ entropy_init(void)
  	    CTLFLAG_PERMANENT, CTLTYPE_NODE, "entropy",
  	    SYSCTL_DESCR("Entropy (random number sources) options"),
  	    NULL, 0, NULL, 0,
 -	    CTL_KERN, CTL_CREATE, CTL_EOL);
 +	    CTL_KERN, KERN_ENTROPY, CTL_EOL);
 =20
  	/* Create the sysctl knobs.  */
  	/* XXX These shouldn't be writable at securelevel>0.  */
 @@ -402,7 +402,7 @@ entropy_init(void)
  	sysctl_createv(&entropy_sysctllog, 0, &entropy_sysctlroot, NULL,
  	    CTLFLAG_PERMANENT|CTLFLAG_READONLY, CTLTYPE_INT,
  	    "epoch", SYSCTL_DESCR("Entropy epoch"),
 -	    NULL, 0, &E->epoch, 0, CTL_CREATE, CTL_EOL);
 +	    NULL, 0, &E->epoch, 0, KERN_ENTROPY_EPOCH, CTL_EOL);
 =20
  	/* Initialize the global state for multithreaded operation.  */
  	mutex_init(&E->lock, MUTEX_DEFAULT, IPL_SOFTSERIAL);
 diff -r e3b67aeee565 -r c8aac91a801f sys/sys/sysctl.h
 --- a/sys/sys/sysctl.h	Fri Mar 07 01:22:46 2025 +0000
 +++ b/sys/sys/sysctl.h	Fri Mar 07 02:25:50 2025 +0000
 @@ -275,6 +275,7 @@ struct ctlname {
  #define	KERN_BOOTTIME		83	/* struct: time kernel was booted */
  #define	KERN_EVCNT		84	/* struct: evcnts */
  #define	KERN_SOFIXEDBUF		85	/* bool: fixed socket buffer sizes */
 +#define	KERN_ENTROPY		86	/* node: entropy(9) subsystem */
 =20
  /*
   *  KERN_CLOCKRATE structure
 @@ -780,6 +781,12 @@ typedef int	(*hashstat_func_t)(struct ha
  void		hashstat_register(const char *, hashstat_func_t);
 =20
  /*
 + * kern.entropy.* variables
 + */
 +
 +#define	KERN_ENTROPY_EPOCH		1
 +
 +/*
   * CTL_VM identifiers in <uvm/uvm_param.h>
   */
 =20

 --=_zuuYtksK0C8RJaF9J8AwVVP/KZSlw/L+--

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/59148: arc4random calls malloc so it can't be used in an ELF constructor
Date: Mon, 10 Mar 2025 07:14:55 +0700

     Date:        Sat, 8 Mar 2025 22:56:38 +0000
     From:        Taylor R Campbell <riastradh@NetBSD.org>
     Message-ID:  <20250308225643.5B81184E3A@mail.netbsd.org>


   | I'm leaning toward (2).

 That's what I would select too.


Responsible-Changed-From-To: lib-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Tue, 11 Mar 2025 14:32:03 +0000
Responsible-Changed-Why:
mine


State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 11 Mar 2025 14:32:03 +0000
State-Changed-Why:
fixed in HEAD, needs pullup-10, inapplicable <10


From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59148 CVS commit: src
Date: Tue, 11 Mar 2025 14:30:28 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue Mar 11 14:30:28 UTC 2025

 Modified Files:
 	src/lib/libc/gen: arc4random.c
 	src/sys/kern: kern_entropy.c
 	src/sys/sys: sysctl.h

 Log Message:
 Assign static MIB numbers for kern.entropy.epoch.

 This sidesteps any need for dynamically sized memory in userland to
 resolve sysctl names to read it out, or for a new syscall interface
 to sysctl resolution by name.

 It would really be better to expose this through a page shared with
 userland, so querying it doesn't cost a syscall, but this will serve
 for now.

 PR lib/59148: arc4random calls malloc so it can't be used in an ELF
 constructor


 To generate a diff of this commit:
 cvs rdiff -u -r1.49 -r1.50 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.72 -r1.73 src/sys/kern/kern_entropy.c
 cvs rdiff -u -r1.239 -r1.240 src/sys/sys/sysctl.h

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

>Unformatted:

Home	PR Database Search