Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Fri, Apr 29, 2022 at 6:57 PM Jason A. Donenfeld wrote:
> On Fri, Apr 29, 2022 at 6:04 PM Denys Vlasenko
> wrote:
> > On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote:
> > > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote:
> > > > 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) ?
> > >
> > > It cannot be replaced with that. The fsync() must happen before other
> > > operations move forward,
> >
> > Why? It should be explained in a comment, and explained well enough
> > so that future developers do not repeatedly try to remove it
> > because "I don't see why it's necessary".
>
> I'll add a comment and submit v2 of my recent patch.
It'll be faster if we just discuss it in this thread.
Describe a realistic scenario where having this fsync()
prevents a problem.
> > > and exiting the program isn't necessarily the
> > > correct course of action.
Of course. I'm not saying that "immediately exit with error message"
_always_ is the correct approach.
What I saw a lot over the years is that error paths
tend to be not well-tested, and nevertheless, even when they
are not buggy per se, the error conditions they are reacting to
are often a part of a bigger problem (usually beyond the ability
of the code in question to detect, much less sensibly correct).
For example, if you get a ENOSPC on write, you might be
tempted to return a special exit code signifying this specific error.
However.
I'd bet you real money that NOT ONE of callers of your tool
will ever bother checking specifically for this exit code.
At best, the callers will react uniformly for any non-zero exit code as
"something went wrong". Quite often, check for !0 exit code will be
entirely forgotten.
What *is* important in this particular example, in practice?
Exiting with nonzero exitcode (any nonzero would do, say 1)
and _printing perror_ - in practice, that's what makes users
understand what exactly the problem is (in this case,
"aha, the disk is full!!!")
(A counter-example when "exit wih error" is totally wrong
is a database server application: on ENOSPC, it almost certainly
should NOT exit! Oracle DB, for example, will wait until
the problem is fixed).
> > Let's see.
> > If we opened the file, read it, and then failed to unlink it,
> > it means that filesystem is read-only, there is a permission problem
> > (e.g. SELinux), or there is an I/O error.
> > All of these cases mean that the system is not in expected state,
> > and further efforts to "continue as normal" (e.g. try to rewrite
> > /var/lib/seedrng/seed.credit) do not seem likely to be productive to me.
> > I think it's better to just complain and exit.
>
> No, this isn't okay. It makes it impossible to distinguish which of
> the seeds was used by the caller of the program from inspecting the
> error code,
Can you show me an existing user of seedrng which checks for this
specific exit code?
> and it means that it can't attempt the second one if the
> first fails (because of, say, the immutable file attribute being set).
If your system starts getting immutable attributes semi-randomly
set on some files but not others, the mitigation of using the second file
to successfully seed the RNG seems like polishing Titanic's bronze.
We need to let the user know there's an unexpected problem
(and we do). Continuing trying to work is not that important.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Fri, 29 Apr 2022 18:35:54 +0200 Denys Vlasenko wrote: > Even partial removal of these complicated error paths > cuts down the size by ~10% It's one of those situations where all you would really want is spend 100b in the kernel to "credit" seed. Be it via /sys or the first bit of the write fops does not matter. If that distinction even makes sense. Distros could then just cat/dd their noise to the pool and be done. The initial proposal had more than 3k. The tool sounds like it should fit in about 1k if we're forced to ioctl and cannot simply use the shell. We'd all be better off to remove that unnecessary ioctl bloat from the kernel and userspace. It does not make any sense at all. In shell it would probably be done in a few hundred bytes, once and for all. PS: See http://lists.busybox.net/pipermail/busybox/2022-April/089590.html for a -260b against an older incarnation. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
Denys Vlasenko wrote in
:
...
|Even partial removal of these complicated error paths
|cuts down the size by ~10%
A bit off topic but i personally am still thinking, really, that
people would rather be reenabled to do what they did for long.
ports/core/rc/rc
^e3afe2298e (Johannes Winkelmann 2006-02-23 15:26:10 + 80) # Load random
seed
^e3afe2298e (Johannes Winkelmann 2006-02-23 15:26:10 + 81) /bin/cat
/var/lib/urandom/seed > /dev/urandom
/ports/core/rc/rc.shutdown
^e3afe2298e (Johannes Winkelmann 2006-02-23 15:26:10 + 33) # Save random
seed
^e3afe2298e (Johannes Winkelmann 2006-02-23 15:26:10 + 34) /bin/dd
if=/dev/urandom of=/var/lib/urandom/seed count=1 2> /dev/null
It is a young Linux distribution. Maybe they would consider to
use /dev/random instead of /dev/urandom, but fed in noise is fed
in noise in the end.
OpenBSD, just as an example, does in /etc/rc
# Push the old seed into the kernel, create a future seed and create a seed
# file for the boot-loader.
random_seed() {
dd if=/var/db/host.random of=/dev/random bs=65536 count=1 status=none
chmod 600 /var/db/host.random
dd if=/dev/random of=/var/db/host.random bs=65536 count=1 status=none
dd if=/dev/random of=/etc/random.seed bs=512 count=1 status=none
chmod 600 /etc/random.seed
}
Once i had to write entropy-saver.c (i later also had to install
haveged and this still do not understand why, rhetorically) it was
only to undo the mental displacement i encountered when the RNG no
longer became seeded upon system boot on my AlpineLinux VM,
resulting in boot hangs of up to half an hour (due to ssh iirc).
--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
Rather than having getrandom() be called in a loop that handles EINTR --
which would require more code bloat -- we just limit the maximum seed
size to 256 bytes, which the kernel guarantees won't be interrupted.
Additionally document the flock() and fsync() usage so that somebody
doesn't remove it. Apparently busybox developers like to remove things
they don't understand with no regards to security implications, so Denys
suggested I leave some comments here.
Cc: Denys Vlasenko
Cc: Bernhard Reutner-Fischer
Signed-off-by: Jason A. Donenfeld
---
util-linux/seedrng.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/util-linux/seedrng.c b/util-linux/seedrng.c
index c42274759..5ee6197e3 100644
--- a/util-linux/seedrng.c
+++ b/util-linux/seedrng.c
@@ -56,7 +56,7 @@
enum {
MIN_SEED_LEN = SHA256_OUTSIZE,
- MAX_SEED_LEN = 512
+ MAX_SEED_LEN = 256 /* Maximum size of getrandom() call without EINTR. */
};
static size_t determine_optimal_seed_len(void)
@@ -142,6 +142,8 @@ static int seed_from_file_if_exists(const char *filename,
int dfd, bool credit,
bb_perror_msg("can't%s seed", " read");
return -1;
}
+ /* The fsync() here is necessary for safety here, so that power being
pulled
+* at the wrong moment doesn't result in the seed being used twice by
accident. */
if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
bb_perror_msg("can't%s seed", " remove");
return -1;
@@ -190,6 +192,8 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
if (mkdir(seed_dir, 0700) < 0 && errno != EEXIST)
bb_perror_msg_and_die("can't %s seed directory", "create");
dfd = open(seed_dir, O_DIRECTORY | O_RDONLY);
+ /* The flock() here is absolutely necessary, as the consistency of this
+* program breaks down with concurrent uses. */
if (dfd < 0 || flock(dfd, LOCK_EX) < 0)
bb_perror_msg_and_die("can't %s seed directory", "lock");
xfchdir(dfd);
@@ -222,6 +226,8 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
printf("Saving %u bits of %screditable seed for next boot\n",
(unsigned)new_seed_len * 8, new_seed_creditable ? "" : "non-");
fd = open(NON_CREDITABLE_SEED_NAME, O_WRONLY | O_CREAT | O_TRUNC, 0400);
+ /* The fsync() here is necessary to ensure the data is written to disk
before
+* we attempt to make it creditable. */
if (fd < 0 || full_write(fd, new_seed, new_seed_len) !=
(ssize_t)new_seed_len || fsync(fd) < 0) {
bb_perror_msg("can't%s seed", " write");
return program_ret | (1 << 4);
--
2.35.1
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Fri, Apr 29, 2022 at 6:36 PM Denys Vlasenko wrote: > Even partial removal of these complicated error paths > cuts down the size by ~10% You know if you just cut out all of the actual code but made it still print the same status messages, you could cut out like 90% of the size... It turns out, specific functionality is actually useful. We can't remove sensible error handling. SeedRNG needs to work all the time and in weird circumstances. Jason ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
Hi Denys,
On Fri, Apr 29, 2022 at 6:04 PM Denys Vlasenko wrote:
> On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote:
> > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote:
> > > 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) ?
> >
> > It cannot be replaced with that. The fsync() must happen before other
> > operations move forward,
>
> Why? It should be explained in a comment, and explained well enough
> so that future developers do not repeatedly try to remove it
> because "I don't see why it's necessary".
I'll add a comment and submit v2 of my recent patch.
> > and exiting the program isn't necessarily the
> > correct course of action.
>
> Let's see.
> If we opened the file, read it, and then failed to unlink it,
> it means that filesystem is read-only, there is a permission problem
> (e.g. SELinux), or there is an I/O error.
> All of these cases mean that the system is not in expected state,
> and further efforts to "continue as normal" (e.g. try to rewrite
> /var/lib/seedrng/seed.credit) do not seem likely to be productive to me.
> I think it's better to just complain and exit.
No, this isn't okay. It makes it impossible to distinguish which of
the seeds was used by the caller of the program from inspecting the
error code, and it means that it can't attempt the second one if the
first fails (because of, say, the immutable file attribute being set).
I'm not going to make that change.
Jason
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Fri, Apr 29, 2022 at 6:04 PM Denys Vlasenko wrote:
> On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote:
> > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote:
> > > 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) ?
> >
> > It cannot be replaced with that. The fsync() must happen before other
> > operations move forward,
>
> Why? It should be explained in a comment, and explained well enough
> so that future developers do not repeatedly try to remove it
> because "I don't see why it's necessary".
>
> > and exiting the program isn't necessarily the
> > correct course of action.
>
> Let's see.
> If we opened the file, read it, and then failed to unlink it,
> it means that filesystem is read-only, there is a permission problem
> (e.g. SELinux), or there is an I/O error.
> All of these cases mean that the system is not in expected state,
> and further efforts to "continue as normal" (e.g. try to rewrite
> /var/lib/seedrng/seed.credit) do not seem likely to be productive to me.
> I think it's better to just complain and exit.
Even partial removal of these complicated error paths
cuts down the size by ~10%
function old new delta
.rodata 104946 104938 -8
seedrng_main12251077-148
--
(add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-156) Total: -156 bytes
diff --git a/util-linux/seedrng.c b/util-linux/seedrng.c
index c42274759..82c69b72b 100644
--- a/util-linux/seedrng.c
+++ b/util-linux/seedrng.c
@@ -100,63 +100,43 @@ static int read_new_seed(uint8_t *seed, size_t
len, bool *is_creditable)
return -1;
}
-static int seed_rng(uint8_t *seed, size_t len, bool credit)
+static void seed_rng(uint8_t *seed, size_t len, bool credit)
{
struct {
int entropy_count;
int buf_size;
uint8_t buffer[MAX_SEED_LEN];
} req;
-int random_fd, ret;
-
-if (len > sizeof(req.buffer)) {
-errno = EFBIG;
-return -1;
-}
+int random_fd;
req.entropy_count = credit ? len * 8 : 0;
req.buf_size = len;
memcpy(req.buffer, seed, len);
-random_fd = open("/dev/urandom", O_RDONLY);
-if (random_fd < 0)
-return -1;
-ret = ioctl(random_fd, RNDADDENTROPY, );
-if (ret)
-ret = -errno ? -errno : -EIO;
+random_fd = xopen("/dev/urandom", O_RDONLY);
+xioctl(random_fd, RNDADDENTROPY, );
if (ENABLE_FEATURE_CLEAN_UP)
close(random_fd);
-errno = -ret;
-return ret ? -1 : 0;
}
-static int seed_from_file_if_exists(const char *filename, int dfd,
bool credit, sha256_ctx_t *hash)
+static void seed_from_file_if_exists(const char *filename, bool
credit, sha256_ctx_t *hash)
{
uint8_t seed[MAX_SEED_LEN];
ssize_t seed_len;
seed_len = open_read_close(filename, seed, sizeof(seed));
if (seed_len < 0) {
-if (errno == ENOENT)
-return 0;
-bb_perror_msg("can't%s seed", " read");
-return -1;
+if (errno != ENOENT)
+bb_perror_msg_and_die("can't%s seed", " read");
+return;
}
-if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
-bb_perror_msg("can't%s seed", " remove");
-return -1;
-} else if (!seed_len)
-return 0;
-
-sha256_hash(hash, _len, sizeof(seed_len));
-sha256_hash(hash, seed, seed_len);
-
-printf("Seeding %u bits %s crediting\n", (unsigned)seed_len * 8,
credit ? "and" : "without");
-if (seed_rng(seed, seed_len, credit) < 0) {
-bb_perror_msg("can't%s seed", "");
-return -1;
+xunlink(filename);
+if (seed_len != 0) {
+sha256_hash(hash, _len, sizeof(seed_len));
+sha256_hash(hash, seed, seed_len);
+printf("Seeding %u bits %s crediting\n", (unsigned)seed_len *
8, credit ? "and" : "without");
+seed_rng(seed, seed_len, credit);
}
-return 0;
}
int seedrng_main(int argc, char *argv[]) MAIN_EXTERNALLY_VISIBLE;
@@ -202,11 +182,9 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
sha256_hash(, , sizeof(timestamp));
for (int i = 1; i < 3; ++i) {
-if (seed_from_file_if_exists(i == 1 ?
NON_CREDITABLE_SEED_NAME : CREDITABLE_SEED_NAME,
-dfd,
+seed_from_file_if_exists(i == 1 ? NON_CREDITABLE_SEED_NAME :
CREDITABLE_SEED_NAME,
i == 1 ? false : !skip_credit,
-) < 0)
-program_ret |= 1 << i;
+);
}
new_seed_len = determine_optimal_seed_len();
___
busybox mailing list
busybox@busybox.net
RE: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
From: Denys Vlasenko > Sent: 29 April 2022 17:17 > > On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote: > > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote: > > > Can we replace all [s]size_t's with ints/unsigneds? We do not expect > > > random pools anywhere near 4 gigabytes... > > > > Probably that's fine. Is the advantage to tossing out consistent types > > worth it though? Does this actually save space? Since [s]size_t is > > usually the word size, won't codegen not really change much? > > For example, on x86-64, 32-bit insns are often shorter. Provided you remember to use 'unsigned int' for array subscripts. Otherwise you can get a lot of 'sign extend' instructions. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote: > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote: > > Can we replace all [s]size_t's with ints/unsigneds? We do not expect > > random pools anywhere near 4 gigabytes... > > Probably that's fine. Is the advantage to tossing out consistent types > worth it though? Does this actually save space? Since [s]size_t is > usually the word size, won't codegen not really change much? For example, on x86-64, 32-bit insns are often shorter. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote:
> On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote:
> > 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) ?
>
> It cannot be replaced with that. The fsync() must happen before other
> operations move forward,
Why? It should be explained in a comment, and explained well enough
so that future developers do not repeatedly try to remove it
because "I don't see why it's necessary".
> and exiting the program isn't necessarily the
> correct course of action.
Let's see.
If we opened the file, read it, and then failed to unlink it,
it means that filesystem is read-only, there is a permission problem
(e.g. SELinux), or there is an I/O error.
All of these cases mean that the system is not in expected state,
and further efforts to "continue as normal" (e.g. try to rewrite
/var/lib/seedrng/seed.credit) do not seem likely to be productive to me.
I think it's better to just complain and exit.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
ACE + TAO with buildroot - GLIBC_2.28 not found
Hi, intergrating ACE was successfull with https://patchwork.ozlabs.org/project/buildroot/patch/20210413134139.13281-1-matthew.we...@rockwellcollins.com/ ACE lib is built. When I integrate TAO in the ace.mk TAO_LIBRARIES += TAO_IDL TAO_LIBRARIEs += tao/PortableServer $(foreach lib,$(TAO_LIBRARIES), ACE_ROOT="$(@D)" $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D)/TAO/$(lib) ACE_ROOT="$(@D)" TAO_ROOT="$(@D)/TAO" all ) The tao_idl complier will be built. The PortableServer uses the created tao_idl complier but exit with: ../../../bin/tao_idl: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.28' not found (required by ../../../lib/libACE.so.7.0.2) ../../../bin/tao_idl: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by ../../../lib/libACE.so.7.0.2) Wrong way to call? Thanks for support, Alex ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote:
> 3) Limit the poolsize to 256 bytes (by changing the MAX_SEED_LEN enum
>value to 256 instead of 512).
I implemented this, and
> > 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.
>
> Fine with me (if you want to CC me on a patch to review etc).
this, here: .
Thanks,
Jason
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] seedrng: limit poolsize to 256 bytes and document flock() usage
Rather than having getrandom() be called in a loop that handles EINTR --
which would require more code bloat -- we just limit the maximum seed
size to 256 bytes, which the kernel guarantees won't be interrupted.
Additionally document the flock() usage so that somebody doesn't remove
it.
Cc: Denys Vlasenko
Cc: Bernhard Reutner-Fischer
Signed-off-by: Jason A. Donenfeld
---
util-linux/seedrng.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/util-linux/seedrng.c b/util-linux/seedrng.c
index c42274759..1257cd941 100644
--- a/util-linux/seedrng.c
+++ b/util-linux/seedrng.c
@@ -56,7 +56,7 @@
enum {
MIN_SEED_LEN = SHA256_OUTSIZE,
- MAX_SEED_LEN = 512
+ MAX_SEED_LEN = 256 /* Maximum size of getrandom() call without EINTR. */
};
static size_t determine_optimal_seed_len(void)
@@ -190,6 +190,8 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
if (mkdir(seed_dir, 0700) < 0 && errno != EEXIST)
bb_perror_msg_and_die("can't %s seed directory", "create");
dfd = open(seed_dir, O_DIRECTORY | O_RDONLY);
+ /* The flock() here is absolutely necessary, as the consistency of this
+* program breaks down with concurrent uses. */
if (dfd < 0 || flock(dfd, LOCK_EX) < 0)
bb_perror_msg_and_die("can't %s seed directory", "lock");
xfchdir(dfd);
--
2.35.1
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
