NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: lib/59148: arc4random calls malloc so it can't be used in an ELF constructor
The following reply was made to PR lib/59148; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: netbsd-bugs%NetBSD.org@localhost
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@localhost>
# 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@localhost>
# 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+--
Home |
Main Index |
Thread Index |
Old Index