Re: [PATCH v2 0/2] [RFC] virtio-rng entropy leak reporting feature

[Amit Shah]

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