Hey Herbert / Jason / crypto maintainers, On Mon, 2023-03-13 at 11:42 +0100, bchalios@xxxxxxxxx wrote: > Hi Amit, > > Thanks for taking the time to look into this. > > On 3/2/23 5:55 PM, Amit Shah <amit@xxxxxxxxxxxxx> wrote: > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > > > > > Hey all, > > > > On Tue, 2023-01-31 at 17:27 +0100, Jason A. Donenfeld wrote: > > > You sent a v2, but I'm not back until the 11th to provide comments on > > > v1. I still think this isn't the right direction, as this needs tie-ins > > > to the rng to actually be useful. Please stop posting new versions of > > > this for now, so that somebody doesn't accidentally merge it; that'd be > > > a big mistake. I'll paste what I wrote you prior: > > > > I sat down to review the patchset but looks like there's some > > background I'm not aware of. > > > > It looks like Babis has implemented the guest side here according to > > the spec change that Michael has posted. > > > > Jason, do you have an alternative in mind? In that case, we should > > pick this up in the spec update thread instead. > > I am not sure what Jason meant here by "This needs to be integrated more closely with random.c itself, similar to how vmgenid works", > but here's my take on this. > > The point of the patchset is to provide an implementation of Michael's spec on which we can discuss. It implements the HW API and it has > some hooks showing how this API could be used. It is mainly directed towards the user-space where we did not have a proper API to consume > VMGENID-like functionality. With regards to the random.c components it does exactly what VMGENID does currently, i.e. whenever an entropy-leak > is detected it uses the new random bytes provided by the virtio-rng device as entropy. This is as racy as VMGENID, as I mention in the cover > letter of the patchset. Yea, this does solve the race condition from the userspace pov, so does look better. Thanks for the details! Not sure if Jason's back yet - but Herbert, or other crypto maintainers, can you chime in from the crypto/rng perspective if this looks sane? Jason has previously NACKed the patch without follow-up, and I don't want the patch to linger without a path to merging, especially when it's not clear what Jason meant. > However, the new spec does allow us to do things _correctly_, i.e. not rely on asynchronous handling of events to re-seed the kernel. For example, we > could achieve something like that by making use of the "copy-on-leak" operation, so that a flag changes value before vCPUs get resumed, so we know > when a leak has happened when needed, e.g. before returning random bytes to user-space. At least, that's what I remember us discussing during LPC. > Jason, Michael, Alex, please keep me honest here :) > > Unfortunately, I am not very familiar with the random.c code and did not want to do something there that would most certainly be wrong, hence I posted > this as an RFC, asking for input on how we could achieve this better integration. Hopefully, when Jason is back from his vacation he can share his thoughts > on this, but if yourself (or anyone else interested) have any ideas on how we could design this properly, I 'm happy to discuss! Let's wait a couple more days for responses, otherwise I suggest you resubmit to kickstart a new discussion, with the note that Jason had something else in mind - so that it doesn't appear as though we're trying to override that. Thanks for the patience, Amit
