Good day Jason, On Wed, Apr 20, 2022 at 6:29 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote: > Hi Bernhard, > > On Wed, Apr 20, 2022 at 3:55 PM Bernhard Reutner-Fischer > <rep.dot....@gmail.com> wrote: > > I've applied this v9 now, thanks for the patch and thanks a lot for your > > patience! > > Excellent! Thank you. Feel free to CC me on other things you have > planned there -- happy to review.
Looking at the current git. In read_new_seed(), if getrandom(GRND_NONBLOCK) reads less than len bytes: static int read_new_seed(uint8_t *seed, size_t len, bool *is_creditable) { ssize_t ret; *is_creditable = false; ret = getrandom(seed, len, GRND_NONBLOCK); if (ret == (ssize_t)len) { *is_creditable = true; return 0; } if (ret < 0 && errno == ENOSYS) { .fd = open("/dev/random", O_RDONLY), ... *is_creditable = poll(&random_fd, 1, 0) == 1; close(random_fd.fd); } else if (getrandom(seed, len, GRND_INSECURE) == (ssize_t)len) return 0; the code reads GRND_INSECURE... overwriting possibly up to len-1 useful, and probably more securely random bytes? if (open_read_close("/dev/urandom", seed, len) == (ssize_t)len) return 0; And here again, if we got less than len bytes, we overwrite them again? I'm not saying it's a bug, I'm asking whether it's a decision to have simple, but suboptimal code, or is this consideration overlooked? if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) { bb_perror_msg("can't%s seed", " remove"); return -1; } Why can't the entire above if() be replaced with xunlink(filename) ? if (dfd < 0 || flock(dfd, LOCK_EX) < 0) bb_perror_msg_and_die("can't %s seed directory", "lock"); People will repeatedly try to remove this flock(), let's add a comment which explains, in detail, what bad things can happen if it is removed. Can we replace all [s]size_t's with ints/unsigneds? We do not expect random pools anywhere near 4 gigabytes... _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox