New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: allow crediting the seed file for some entropy #10621

Open
wants to merge 4 commits into
base: master
from

Conversation

5 participants
@Villemoes
Copy link

Villemoes commented Nov 2, 2018

This provides the administrator with a knob so that the seed file can be credited with providing some amount of entropy. My primary motivation is for use in embedded products [1], but there are also reports (e.g. https://lore.kernel.org/lkml/20181030001807.7wailpm37mlinsli@breakpoint.cc/) suggesting that it might be useful for desktops.

My idea is that the administrator tweaks the random-seed.service file to add the -c option, but I'm not really sure what the most convenient interface is. For our use case, just the first patch would be sufficient, since we could then just patch in the appropriate number with a sed script during the BSP build.

This probably will need some documentation update, but I'll wait with that until getting some feedback and knowing whether this has any chance of being accepted.

[1] In the embedded products we support, we often see (and get complaints) that full system initialization takes a very long time. Part of that is due to the lack of good entropy sources, especially during the early phase. Most, but not all, modern hardware have random generators built in, but legacy hardware still needs to be maintained.

@yuwata yuwata added the random-seed label Nov 3, 2018

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 3, 2018

Also see #4271, #4513.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 3, 2018

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.

I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

_cleanup_close_ int seed_fd = -1, random_fd = -1;
bool read_seed_file, write_seed_file;
_cleanup_free_ void* buf = NULL;
unsigned entropy_count = 0;

This comment has been minimized.

@poettering

poettering Nov 3, 2018

Member

hmm, this is weird, why "unsigned"? the kernel structure actually expects an "int" here (which appears kinda sloppy to me, but anyway...). And philosophically it appears to me this should be a size_t, as ideally if we pass fully random data in then the entropy_count should equal the buffer size, no?

This comment has been minimized.

@Villemoes

Villemoes Nov 3, 2018

Dunno. I started with a size_t, then I think I had some reason to change it, though I don't recall why. The kernel caps it at, I think, 4096 anyway (or some other similar smallish number), so it doesn't matter too much.

This comment has been minimized.

@poettering

poettering Nov 5, 2018

Member

still, we generally care about type issues in our codebase. If the kernel APIs are a bit sloppy with types then this doesn't mean we should be sloppy too. Please pick the most appropriate type always.

@@ -131,8 +133,8 @@ int main(int argc, char *argv[]) {
if ((uint64_t) st.st_size > buf_size)
buf_size = MIN(st.st_size, POOL_SIZE_MAX);

buf = malloc(buf_size);
if (!buf) {
info = malloc(sizeof(*info) + buf_size);

This comment has been minimized.

@poettering

poettering Nov 3, 2018

Member

should be:

info = malloc(offsetof(struct rand_pool_info, buf) + buf_size);

This comment has been minimized.

@Villemoes

Villemoes Nov 3, 2018

So, in this case offsetof() and sizeof() are the same, so I won't bikeshed too much over this. But FTR, the reason I prefer the sizeof() idiom is that it ensures one allocates a full struct, even if buf_size is small. Consider something like struct { long a; char b; char c[]; }. Allocating using offsetof is error-prone if the allocation is hit with a memset(..., sizeof()), or one uses struct assignment. The fully correct thing is to do the offsetof thing, then max() with sizeof(*info), but that's so much of a mouthful that it requires a helper macro.

This comment has been minimized.

@poettering

poettering Nov 5, 2018

Member

Well, the way I understood C sizeof() and offsetof(…) will actually result in the very same results in cases like this. however, one makes very clear what is going on here I think for the reader. hence, this is really about readability, nothing else...

This comment has been minimized.

@poettering

poettering Jan 17, 2019

Member

not addresed yet

else {
log_error("Unknown verb '%s'.", argv[optind]);
return -EINVAL;
}

This comment has been minimized.

@poettering

poettering Nov 3, 2018

Member

(i appreciate the getoptification, but in an ideal world we'd go even further and use verbs.[ch] for dispatching the verbs here... but i guess this is unrelated to this PR)

This comment has been minimized.

@Villemoes

Villemoes Nov 3, 2018

Yeah, and if the interface ends up being an env var, this goes away, so I won't spend too much time polishing this.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 3, 2018

Hmm, so I find it strange to make the numeric entropy count configurable like this, given that it needs to match the properties of the actual random seed data currently on disk. i.e. instead of making the numeric value an admin-facing option possibly not matching the size and quality of the random seed read from disk (which is an automatic system-maintained data element) it should be a boolean I guess and the actual entropy count should be derived from the stored seed. Or what am I missing?

If we assume perfect entropy of the data read from disk we could simply derive the entropy count to credit from the size of the data: just multiply it by 8 to convert bytes to bits (the unit of entropy_count is unfortunately not even documented even though it appears to be bits, grrrrr). Now, I am not sure how the Linux RNG subsystem in this regard really works, and these things are all not documented, but if we read data during shutdown from /dev/urandom, I presume the entropy is actually much lower than the actual byte size we are reading? if so, then we should probably read the current actual entropy from the kernel and store it alongside the seed file we are writing, so that when we come back on next boot we don't request to credit more entropy than the seed actually contains... There appears to be an ioctl (RNDGETENTCNT) that allows us to retrieve the current amount of entropy, so maybe it is as easy as calling that and storing that somewhere on close to the random seed file (maybe as an xattr, so that it is inherently part of the file, and removed/recreated always alongside with it, and can't get out of sync). Of course, ideally we'd atomically read the seed data and the entropy value from the kernel in one go, so that things aren't racy, but I figure we are OK ignoring this, we have to work with what we got,...

That said, I am not sure I grok this all properly even. The man page has example shell scripts explaining what to do during shutdown/reboot. And they of course do not credit the entropy at all. If this is something that should be done these days, then great, but it would be very welcome if the man page would actually say so...

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 3, 2018

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.

Fully agree, this should not be done by default, and requires explicit opt-in.

I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

Yes, as I wrote, I wasn't sure about the best interface. An env var would certainly also work, and would avoid having to do the full getoptization (which then also somewhat compels one to implement -h etc.). Even from the shell (should anyone call systemd-random-seed manually), there's not much difference between 'ENTROPY_COUNT=123 systemd-random-seed load' and 'systemd-random-seed -c 123 load'. Is there some naming scheme/namespace to follow for the env var, or would ENTROPY_COUNT be fine?

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 3, 2018

Hmm, so I find it strange to make the numeric entropy count configurable like this, given that it needs to match the properties of the actual random seed data currently on disk. i.e. instead of making the numeric value an admin-facing option possibly not matching the size and quality of the random seed read from disk (which is an automatic system-maintained data element) it should be a boolean I guess and the actual entropy count should be derived from the stored seed. Or what am I missing?

In an ideal world, you're right. The problem is, one never (not even the kernel) knows the actual amount of entropy. So it will always boil down to a question of not how much entropy is there, but how much does the admin trust the file to provide/contain. And for that reason, it does make sense to make it an admin knob - essentially, he gets to do the tradeoff between boot time and security.

If we assume perfect entropy of the data read from disk we could simply derive the entropy count to credit from the size of the data: just multiply it by 8 to convert bytes to bits (the unit of entropy_count is unfortunately not even documented even though it appears to be bits, grrrrr).

Yup, I went to the one true place for documentation on this (i.e., the kernel source code).

Now, I am not sure how the Linux RNG subsystem in this regard really works, and these things are all not documented, but if we read data during shutdown from /dev/urandom, I presume the entropy is actually much lower than the actual byte size we are reading? if so, then we should probably read the current actual entropy from the kernel and store it alongside the seed file we are writing, so that when we come back on next boot we don't request to credit more entropy than the seed actually contains...

I don't think that will work. Just because you read 16 bytes out of /dev/urandom doesn't decrement /proc/sys/kernel/random/entropy_avail by 128. Again, this is really just a judgment call for the admin to make.

There appears to be an ioctl (RNDGETENTCNT) that allows us to retrieve the current amount of entropy, so maybe it is as easy as calling that and storing that somewhere on close to the random seed file (maybe as an xattr, so that it is inherently part of the file, and removed/recreated always alongside with it, and can't get out of sync). Of course, ideally we'd atomically read the seed data and the entropy value from the kernel in one go, so that things aren't racy, but I figure we are OK ignoring this, we have to work with what we got,...

It might make sense to sample /proc/sys/kernel/random/entropy_avail when we save the new seed file [obviously, on the initial load, sample before load] just to get a ballpark estimate of how good the randomness one reads at that point in time is, but using it as some exact figure is wrong. Not sure what to do with that quality assesment, though. As for storage, please just add another 4 bytes or whatnot to the file, instead of relying on the file system having xattrs.

That said, I am not sure I grok this all properly even. The man page has example shell scripts explaining what to do during shutdown/reboot. And they of course do not credit the entropy at all. If this is something that should be done these days, then great, but it would be very welcome if the man page would actually say so...

So, this is not "something that should be done these days period". Please see the recent thread on (among other lists) lkml, "Bug#912087: openssh-server: Slow startup after the upgrade to 7.9p1", in particular Ted Ts'o (the kernel random maintainer) in https://lore.kernel.org/lkml/20181030205136.GB6236@thunk.org/ :

In any case, if Debian wants to ship a program which reads a seed file
and uses it to initialize the random pull assuming that it's
trustworthy via the RNDADDENTROPY ioctl, that's not an insane thing to
do. My recommendation would be to make it be configurable, however,
just as whether we trust RDRAND should be trusted (in isolation) to
initialize the CRNG.

And that is precisely what I'm trying to do here :)

@sebastianas

This comment has been minimized.

Copy link

sebastianas commented Nov 4, 2018

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.

I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

Please don't do this even optionally. This shouldn't be an issue (after talking with Ted) on "recent" machines. KVM hosts can add a hw-rnd device. Everything embedded that lacks good entropy will turn this on without thinking…

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 5, 2018

Yes, as I wrote, I wasn't sure about the best interface. An env var would certainly also work, and would avoid having to do the full getoptization (which then also somewhat compels one to implement -h etc.).

There's nothing wrong with getoptization... Given that you already did that work for that, please just keep it in, even if it's not strictly necessary now. I mean, a --help text is always useful.

Even from the shell (should anyone call systemd-random-seed manually), there's not much difference between 'ENTROPY_COUNT=123 systemd-random-seed load' and 'systemd-random-seed -c 123 load'. Is there some naming scheme/namespace to follow for the env var, or would ENTROPY_COUNT be fine?

Please prefix the env var with SYSTEMD_. Also, document it in docs/ENVIRONMENT.md which is where we document most env vars.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 5, 2018

In an ideal world, you're right. The problem is, one never (not even the kernel) knows the actual amount of entropy. So it will always boil down to a question of not how much entropy is there, but how much does the admin trust the file to provide/contain. And for that reason, it does make sense to make it an admin knob - essentially, he gets to do the tradeoff between boot time and security.

Well, a file that contains a single byte should never be allowed to credit more than 8bit of entropy...

I am pretty sure the very least we should do is use the file size as upper ceiling for the bits to credit.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 5, 2018

I don't think that will work. Just because you read 16 bytes out of /dev/urandom doesn't decrement /proc/sys/kernel/random/entropy_avail by 128. Again, this is really just a judgment call for the admin to make.

But I am pretty sure the entropy the kernel supports should at least be considered an upper ceiling for what we credit

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 5, 2018

As for storage, please just add another 4 bytes or whatnot to the file, instead of relying on the file system having xattrs.

I do not see how we'd do that without breaking compat. We might end up reading files written out from older versions/other tools which are not aware of the entropy counter, and we'd then credit a random amount of entropy...

All writable, persistent file systems that might be suitable for /var I know do support xattrs just fine. Moreover, we should fail gracefully in case we can't set the xattr, and when we read the random seed and see no xattr set we should not credit any entropy. This basically means we are fine with any kind of /var, and if you really use an fs there that can't do xattr, then all you lose is the entropy crediting, which shouldn't be much of a loss...

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 5, 2018

And that is precisely what I'm trying to do here :)

well, those comments don't really answer the questions I raised above. I'd prefer if we'd have clearer information on all this, i.e. whether to care about the entropy when reading, or whether that doesn't matter.

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 5, 2018

In an ideal world, you're right. The problem is, one never (not even the kernel) knows the actual amount of entropy. So it will always boil down to a question of not how much entropy is there, but how much does the admin trust the file to provide/contain. And for that reason, it does make sense to make it an admin knob - essentially, he gets to do the tradeoff between boot time and security.

Well, a file that contains a single byte should never be allowed to credit more than 8bit of entropy...

I am pretty sure the very least we should do is use the file size as upper ceiling for the bits to credit.

Yes, which I'm already doing... actually not using the file size, but the number of bytes we read from the file, in the unlikely case they're different.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Nov 8, 2018

What about defining the variable (SYSTEMD_ENTROPY_VALUE=?) to be a float, between 0 and 1 inclusive, that would specify how much trust to put in the bytes in the file. So SYSTEMD_ENTROPY_VALUE=1 would mean that every byte in the file counts as 8 bits of entropy, SYSTEMD_ENTROPY_VALUE=0.5 would mean that every byte is 4 bits, etc.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 8, 2018

here's an idea regarding how to store the amount of entropy the kernel had during shutdown on disk in a backwards compatible way: we could simply size the resulting seed file according to how much entropy the kernel reported: if the kernel only has a few bits, we'd only write out a short seed file out, and if it has many we generate a longer seed file. When booting up we'd credit the file size. All subject to some upper bounds, and subject to the fraction multiplier, as @keszybz suggested.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 8, 2018

btw, I figure we should use getrandom() with GRND_NONBLOCK for acquiring the seed to write out during shutdown. With that we should see EAGAIN when there's no entropy around and could then remove the seed file.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Nov 8, 2018

we could simply size the resulting seed file according to how much entropy the kernel reported: if the kernel only has a few bits, we'd only write out a short seed file out, and if it has many we generate a longer seed file.

If the kernel has low-quality random bytes, the randomness will be spread evenly over the bytes we get. So if we think what we got is 50% good, and we just took the first half, then we'd actually store half of the entropy we got. So I don't think this is a good idea.

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 8, 2018

What about defining the variable (SYSTEMD_ENTROPY_VALUE=?) to be a float, between 0 and 1 inclusive, that would specify how much trust to put in the bytes in the file. So SYSTEMD_ENTROPY_VALUE=1 would mean that every byte in the file counts as 8 bits of entropy, SYSTEMD_ENTROPY_VALUE=0.5 would mean that every byte is 4 bits, etc.

I thought of something similar: If we're going to store an estimate of how good the randomness we got when creating the seed file was, it doesn't make sense to have the admin tell some absolute number to use for the entropy amount. Instead, let's just multiply the estimated entropy by the float (I'd actually rather do it as a percentage, to avoid floating point when not strictly necessary).

As for getting that estimate, I'd do a RNDGETENTCNT before and after reading the bytes from /dev/urandom, and use the minimum of the two. For the 'random-seed save', that's the number I'd put it. For 'random-seed load', we need to take into account that we just manually adjusted the entropy estimate, so I'd deduct the info.entropy_count we passed in - the kernel uses a sub-additive formula (see the "Credit: we have to account for the possibility of overwriting already present entropy. " comment in the kernel source), so this should ensure we deduct at least what we were credited for providing. Of course, this "heuristic" fails when the board is shut down shortly after boot.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 8, 2018

regarding the GRND_NONBLOCK thing I suggested above. Would love to see #10676 reviewed and merged first (reviews always welcome, @Villemoes hint hint ;-)). After that we can just issue genuine_random_bytes(… RANDOM_DONT_DRAIN). The benefit over calling getrandom() directly is that it will do an automatic fall back to /dev/urandom internally already if getrandom() is not available on the local kernel.

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 8, 2018

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.
I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

Please don't do this even optionally. This shouldn't be an issue (after talking with Ted) on "recent" machines. KVM hosts can add a hw-rnd device.

I don't understand the "recent machines" argument. Lots of hardware doesn't get replaced every other year, for a variety of practical, economical, technical and/or environmental reasons. We're regularly servicing devices deployed 5,10,15 years ago that are expected to be in use for another similar timespan.

Everything embedded that lacks good entropy will turn this on without thinking…

I agree that this provides admins with rope. But I don't agree that all admins and embedded developers will necessarily use that as an opportunity to hang themselves.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 8, 2018

so here's one more problem with all this: we have kind of a feedback loop here during early boot. So far, at boot, we read the random seed of disk, push it into /dev/urandom, then pull out a new random seed and write that to disk, so that we know for sure that on next boot we'll have a different seed in place we push in. this is great as long as we don#t actually credit the entropy for it. But as soon as we do that, then this falls apart because the stuff we pull out again will likely result from just the entropy we pushed in and this means the entropy in the kernel is gone again... Or what am I missing there?

I have the suspicion that the least we have to do is use blocking getrandom() when reading back the seed after having written it and remove the seed file in between to mark it invalidated. i.e.:

at start:

  1. read seed off disk
  2. delete seed on disk
  3. write seed to to /dev/urandom (if crediting the entropy is enabled we do so at this point)
  4. acquire a new seed (if crediting the entropy is enabled in blocking getrandom() mode, otherwise in /dev/urandom read mode, i.e. without blocking and with possibly returning data from a half initialized pool)
  5. write new seed to disk

at stop:

  1. acquire a new seed (if crediting the entropy is enabled in non-blocking getrandom() mode — which might result in EAGAIN —, otherwise in /dev/urandom read mode, i.e. non-blocking mode possibly returning data from a half initialized pool)
  2. if we got EAGAIN leave the seed as it is, otherwise replace it
@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 8, 2018

Instead, let's just multiply the estimated entropy by the float (I'd actually rather do it as a percentage, to avoid floating point when not strictly necessary).

I prefer a percentage too I guess, and we already have a nice parser for that in parse_percent()

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 8, 2018

I agree that this provides admins with rope. But I don't agree that all admins and embedded developers will necessarily use that as an opportunity to hang themselves.

well, if we hide this in some env var i think doing this is fine. We generally use env vars for hiding options that should not be considered primary API but just some expert/debugging tool.

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 8, 2018

here's an idea regarding how to store the amount of entropy the kernel had during shutdown on disk in a backwards compatible way: we could simply size the resulting seed file according to how much entropy the kernel reported: if the kernel only has a few bits, we'd only write out a short seed file out, and if it has many we generate a longer seed file. When booting up we'd credit the file size. All subject to some upper bounds, and subject to the fraction multiplier, as @keszybz suggested.

No, please don't do it like that. I retract my objection to using xattr. Just because the kernel's estimate of the amount of entropy is 8 bits, the internal state of the pool can still be in 2^4096 different states (some more likely than others, perhaps, but still unknown to any would-be attacker), so we should create a file that has at least 4096 bits. If we just created a one-byte file, regardless of whether we're crediting the entropy on next boot or not, we're throwing away a lot of actual entropy, making it easier to guess the internal state of the pool on next boot (i.e., if everything else in the boot process is completely predictable, it could only be in 256 different states).

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Nov 8, 2018

so here's one more problem with all this: we have kind of a feedback loop here during early boot. So far, at boot, we read the random seed of disk, push it into /dev/urandom, then pull out a new random seed and write that to disk, so that we know for sure that on next boot we'll have a different seed in place we push in. this is great as long as we don#t actually credit the entropy for it. But as soon as we do that, then this falls apart because the stuff we pull out again will likely result from just the entropy we pushed in and this means the entropy in the kernel is gone again... Or what am I missing there?

So I hinted at this problem in my other comment, but it probably crossed yours. As I wrote, we can easily account for that by deducting the entropy count we added. The only problem is if shutdown happens shortly after boot, where the pool would still more or less only contain the stuff we added during boot, but we wouldn't correct for that. (OTOH, I'm not really sure that's a problem, or that we should even do the deducting in the load+save case - the entropy estimate is a measure of how hard it is to guess the contents of the internal state, and it doesn't really matter if the contributions came from a lava lamp or the admin saying "these bytes are $x random" or some combination).

I don't think there's much point in using getrandom() if we're reading the entropy estimate with an ioctl() anyway. getrandom() only differs from direct reads from /dev/random or /dev/urandom in that one can do a potentially-blocking-at-boot read from /dev/urandom, but as soon as the entropy estimate hits 256 bits once (which it's likely to do if we credit some entropy), getrandom() becomes equivalent to reading from /dev/urandom.

Rasmus Villemoes added some commits Oct 31, 2018

Rasmus Villemoes
random-seed: prepare for allowing crediting entropy from seed file
This reworks random-seed to use the RNDADDENTROPY ioctl. With an entropy_count
of 0, this is exactly equivalent to writing the bytes to /dev/urandom, so this
patch in itself does not change behaviour. However, in subsequent patches we
will store an estimate of the amount of entropy of the random data in an xattr
associated to the file, and teach systemd-random-seed to take an option

  --entropy-credit <ratio>

(and/or environment variable SYSTEMD_ENTROPY_CREDIT) to indicate how much of
that entropy should be credited to the the kernel's random pool.
Rasmus Villemoes
random-seed: getopt'ify it
This adds parse_argv() and help() boilerplate in preparation for
teaching the command the --entropy-credit option.

Rasmus Villemoes added some commits Jan 7, 2019

Rasmus Villemoes
random-seed: save estimate of quality of random data in an xattr
Use the ioctl RNDGETENTCNT to get an estimate of how good the random data we
read from the kernel is, and store it in an xattr in the random-seed file.

@Villemoes Villemoes force-pushed the Villemoes:random-credit branch from 426de6f to bc49142 Jan 9, 2019

@Villemoes

This comment has been minimized.

Copy link

Villemoes commented Jan 9, 2019

So I tried doing the xattr thing, this is roughly how it would look. Documentation omitted until there's consensus on the approach.

entropy_credit = parse_permille(e);
if (entropy_credit < 0) {
log_warning("Invalid value '%s' of 'SYSTEMD_ENTROPY_CREDIT'. Ignoring.", e);
entropy_credit = 0;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

please assign the result of parse_permille() to a temporary variable first, and when it's not a failure then assign it to arg_entropy_credit. That way, should eventually the default for arg_entropy_credit change we only have to change it at the top of the file, and not here again.

@@ -26,6 +27,7 @@ static enum {
ACTION_LOAD,
ACTION_SAVE,
} arg_action = ACTION_NONE;
static int entropy_credit;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

please prefix with "arg_", we generally do that for all variables directly mapping to cmdline args.

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

also, to make this explicit, could you explicitly initialize it to zero (yes, i know, that's not strictly necessary, but I think it makes clearer for static variables what their default is)

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

please also rename this in a way it's clear this is permille, i.e. arg_entropy_credit_permille or so.

if (e) {
entropy_credit = parse_permille(e);
if (entropy_credit < 0) {
log_warning("Invalid value '%s' of 'SYSTEMD_ENTROPY_CREDIT'. Ignoring.", e);

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

parse_permille() returns an error. Please pass that to log_warning_errno(), in case structured logging is use.

log_warning("Ignoring invalid argument to --entropy-credit=: %s.",
optarg);
entropy_credit = 0;
}

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

as above, please parse into a temporary variable r first, then assign to arg_entropy_credit only on success. And please pass the error to log_warning_errno()

@@ -212,6 +235,8 @@ static int run(int argc, char *argv[]) {
(void) lseek(seed_fd, 0, SEEK_SET);
entropy_count = 0;
entropy_count = MIN(entropy_count, 8*k);
entropy_count *= entropy_credit;
entropy_count /= 1000;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

why do this in four lines? that's needlessly verbose, no?

return 0;
buf[sizeof(buf) - 1] = '\0';
if (safe_atoi(buf, &entropy_count) < 0)
return 0;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

as before, debug log, and pass error code

return 0;
/* Make sure we never end up crediting the same bits twice. */
if (fremovexattr(fd, XATTR_NAME) < 0)
return 0;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

similar here

if (fremovexattr(fd, XATTR_NAME) < 0)
return 0;

return entropy_count;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

i think some validity checks would be appropriate here, for example, filter out negative entropy and so on

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

maybe add some paranoia here: fsync the file and the dir it is in after dropping the xattr, to make sure the entropy is never credited again ever. i.e. call fsync() on the fd first, followed by fsync_directory_of_file().

k = loop_read(random_fd, info->buf, buf_size, false);
if (ioctl(random_fd, RNDGETENTCNT, &e2) < 0)
e2 = 0;

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

my understanding today is that the random pool never depletes once it is seeded, hence reading this twice is probably not necessary

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

also, I figure some safety check should be added: never credit more than k*8 bits, in the unlikely event the seed we store is smaller than the pool size in the kernel

@@ -278,6 +321,7 @@ static int run(int argc, char *argv[]) {
r = loop_write(seed_fd, info->buf, (size_t) k, false);
if (r < 0)
return log_error_errno(r, "Failed to write new random seed file: %m");
write_entropy_xattr(seed_fd, MIN(e1, e2));

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

cast to (void) here, to tell analyzers we knowingly ignore errors

This comment has been minimized.

@poettering

poettering Jan 18, 2019

Member

what i don't like here though: you have an xattr here you called "entropy_count" and a cmdline arg/env var called "entropy_credit". To me it's not clear at all what the difference is or the unit. The latter takes a permille and the former stores an absolute number of bits. I think that's quite confusing, and they should carry different names, and explicit names. Maybe call the xattr "entropy_credit_bits". And internally call the cmdline arg arg_entropy_share_permille (or maybe "ratio" instead of "share"? or "fraction"? or "quota"), but I figure for command line args it's not customary to contain units in the name, hence maybe drop it there, i.e. simply use --entropy-share=, but take permille.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Jan 18, 2019

btw, might make sense to add a kernel cmdline var for this too. might be helpful for people who find themselves with a frozen boot because entropy is missing, but who have no control on env vars or cmdline args...

hooking that up is easy, just use proc_cmdline_get_key(), it's a ready-made function for this purpose.

and env var and cmdline should override the kernel cmdline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment