Saturday, December 31, 2022

[RFC v1 2/2] Use arc4random_range() instead of arc4random_uniform() when appropriate

This makes the code much more readable and self-documented. While doing
this, I noticed a few bugs, and other cases which may be bugs or not.
Switching to this specialized API makes it easier to spot such bugs, but
since I'm not familiar with the code, I kept some bugs unfixed. The
most obvious ones (although I may be wrong) I fixed them. And in some
cases where it was very unclear, I didn't touch the old *_uniform() code.

Below are the cases where I changed the behavior (I considered it a bug):

* usr.bin/ssh/auth.c:

- *cp = hashchars[arc4random_uniform(sizeof(hashchars) - 1)];
+ *cp = hashchars[arc4random_range(0, sizeof(hashchars) - 1)];

* usr.sbin/ftp-proxy/ftp-proxy.c:

- return (IPPORT_HIFIRSTAUTO +
- arc4random_uniform(IPPORT_HILASTAUTO - IPPORT_HIFIRSTAUTO));
+ return arc4random_range(IPPORT_HIFIRSTAUTO, IPPORT_HILASTAUTO);

* usr.sbin/rad/engine.c:

- tv.tv_sec = MIN_RTR_ADV_INTERVAL +
- arc4random_uniform(MAX_RTR_ADV_INTERVAL - MIN_RTR_ADV_INTERVAL);
+ tv.tv_sec = arc4random_range(MIN_RTR_ADV_INTERVAL, MAX_RTR_ADV_INTERVAL);

In the following change, I didn't use the temporary variable 'num3'.
AFAICS, this doesn't affect other uses of the variable in other places,
because they set it before use. But please check carefully; I may have
missed something:

* usr.sbin/cron/entry.c:

- /* get a random number in the interval [num1, num2]
- */
- num3 = num1;
- num1 = arc4random_uniform(num2 - num3 + 1) + num3;
+ num1 = arc4random_range(num1, num2);

Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
games/boggle/boggle/bog.c | 2 +-
games/canfield/canfield/canfield.c | 2 +-
games/mille/init.c | 2 +-
gnu/gcc/gcc/cfgexpand.c | 2 +-
lib/libevent/select.c | 2 +-
regress/lib/libc/malloc/malloc_general/malloc_general.c | 2 +-
regress/sys/sys/tree/rb/rb-test.c | 3 +--
regress/sys/sys/tree/splay/splay-test.c | 3 +--
sbin/iked/ikev2.c | 2 +-
sys/dev/pci/drm/drm_linux.c | 2 +-
sys/dev/pci/drm/include/linux/random.h | 2 +-
sys/kern/kern_fork.c | 2 +-
sys/net/if_spppsubr.c | 7 ++-----
sys/net/pf.c | 2 +-
sys/net/pf_lb.c | 4 ++--
sys/netinet/ip_ipsp.c | 2 +-
usr.bin/nc/netcat.c | 2 +-
usr.bin/skeyinit/skeyinit.c | 2 +-
usr.bin/ssh/auth.c | 2 +-
usr.sbin/cron/entry.c | 5 +----
usr.sbin/ftp-proxy/ftp-proxy.c | 3 +--
usr.sbin/pppd/chap.c | 5 +----
usr.sbin/rad/engine.c | 3 +--
usr.sbin/relayd/shuffle.c | 2 +-
24 files changed, 26 insertions(+), 39 deletions(-)

diff --git a/games/boggle/boggle/bog.c b/games/boggle/boggle/bog.c
index c0e19454a27..3ed4888fc43 100644
--- a/games/boggle/boggle/bog.c
+++ b/games/boggle/boggle/bog.c
@@ -607,7 +607,7 @@ newgame(char *b)
/* Shuffle the cubes using Fisher-Yates (aka Knuth P). */
p = ncubes;
while (--p) {
- q = (int)arc4random_uniform(p + 1);
+ q = (int)arc4random_range(0, p);
tmp = cubes[p];
cubes[p] = cubes[q];
cubes[q] = tmp;
diff --git a/games/canfield/canfield/canfield.c b/games/canfield/canfield/canfield.c
index 346fd20a1d2..dec75f6531f 100644
--- a/games/canfield/canfield/canfield.c
+++ b/games/canfield/canfield/canfield.c
@@ -531,7 +531,7 @@ shuffle(struct cardtype *deck[])
deck[i]->paid = FALSE;
}
for (i = decksize - 1; i > 0; i--) {
- j = arc4random_uniform(i + 1);
+ j = arc4random_range(0, i);
if (i != j) {
temp = deck[i];
deck[i] = deck[j];
diff --git a/games/mille/init.c b/games/mille/init.c
index a86157739dd..c0cc6ac1f02 100644
--- a/games/mille/init.c
+++ b/games/mille/init.c
@@ -90,7 +90,7 @@ shuffle(void)
CARD temp;

for (i = DECK_SZ - 1; i > 0; i--) {
- r = arc4random_uniform(i + 1);
+ r = arc4random_range(0, i);
temp = Deck[r];
Deck[r] = Deck[i];
Deck[i] = temp;
diff --git a/gnu/gcc/gcc/cfgexpand.c b/gnu/gcc/gcc/cfgexpand.c
index 17aff165f6d..0cb8a21289b 100644
--- a/gnu/gcc/gcc/cfgexpand.c
+++ b/gnu/gcc/gcc/cfgexpand.c
@@ -438,7 +438,7 @@ partition_stack_vars (void)
for (si = n - 1; si > 0; si--)
{
size_t tmp;
- sj = arc4random_uniform(si + 1);
+ sj = arc4random_range(0, si);

tmp = stack_vars_sorted[si];
stack_vars_sorted[si] = stack_vars_sorted[sj];
diff --git a/lib/libevent/select.c b/lib/libevent/select.c
index fb38b850155..8d5ff6ff34f 100644
--- a/lib/libevent/select.c
+++ b/lib/libevent/select.c
@@ -155,7 +155,7 @@ select_dispatch(struct event_base *base, void *arg, struct timeval *tv)
event_debug(("%s: select reports %d", __func__, res));

check_selectop(sop);
- i = arc4random_uniform(sop->event_fds + 1);
+ i = arc4random_range(0, sop->event_fds);
for (j = 0; j <= sop->event_fds; ++j) {
struct event *r_ev = NULL, *w_ev = NULL;
if (++i >= sop->event_fds+1)
diff --git a/regress/lib/libc/malloc/malloc_general/malloc_general.c b/regress/lib/libc/malloc/malloc_general/malloc_general.c
index b243787bcf7..3980ed5e277 100644
--- a/regress/lib/libc/malloc/malloc_general/malloc_general.c
+++ b/regress/lib/libc/malloc/malloc_general/malloc_general.c
@@ -27,7 +27,7 @@
size_t
size(void)
{
- int p = arc4random_uniform(17) + 3;
+ int p = arc4random_range(3, 19);
return arc4random_uniform(1 << p);
}

diff --git a/regress/sys/sys/tree/rb/rb-test.c b/regress/sys/sys/tree/rb/rb-test.c
index 409cc22393a..5bf54013d95 100644
--- a/regress/sys/sys/tree/rb/rb-test.c
+++ b/regress/sys/sys/tree/rb/rb-test.c
@@ -67,8 +67,7 @@ main(int argc, char **argv)
tmp = malloc(sizeof(struct node));
if (tmp == NULL) err(1, "malloc");
do {
- tmp->key = arc4random_uniform(MAX-MIN);
- tmp->key += MIN;
+ tmp->key = arc4random_uniform(MIN, MAX - 1);
} while (RB_FIND(tree, &root, tmp) != NULL);
if (i == 0)
max = min = tmp->key;
diff --git a/regress/sys/sys/tree/splay/splay-test.c b/regress/sys/sys/tree/splay/splay-test.c
index 56084a0c71e..69c9e87fa6c 100644
--- a/regress/sys/sys/tree/splay/splay-test.c
+++ b/regress/sys/sys/tree/splay/splay-test.c
@@ -67,8 +67,7 @@ main(int argc, char **argv)
tmp = malloc(sizeof(struct node));
if (tmp == NULL) err(1, "malloc");
do {
- tmp->key = arc4random_uniform(MAX-MIN);
- tmp->key += MIN;
+ tmp->key = arc4random_range(MIN, MAX - 1);
} while (SPLAY_FIND(tree, &root, tmp) != NULL);
if (i == 0)
max = min = tmp->key;
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index 9579b8a2ea5..3b4566bb3ae 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -7186,7 +7186,7 @@ ikev2_cp_setaddr_pool(struct iked *env, struct iked_sa *sa,
if (upper < lower)
upper = lower;
/* Randomly select start from [lower, upper-1] */
- start = arc4random_uniform(upper - lower) + lower;
+ start = arc4random_range(lower, upper - 1);

for (host = start;;) {
log_debug("%s: mask %x start %x lower %x host %x upper %x",
diff --git a/sys/dev/pci/drm/drm_linux.c b/sys/dev/pci/drm/drm_linux.c
index 2b975cda9fb..4e5aa29c228 100644
--- a/sys/dev/pci/drm/drm_linux.c
+++ b/sys/dev/pci/drm/drm_linux.c
@@ -776,7 +776,7 @@ idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp_mask)
end = INT_MAX;

#ifdef notyet
- id->id = begin = start + arc4random_uniform(end - start);
+ id->id = begin = arc4random_range(start, end - 1);
#else
id->id = begin = start;

No comments:

Post a Comment