Linux Virtualization - Mar 2023 - [PATCH v2 0/2] [RFC] virtio-rng entropy leak reporting feature

Hey Herbert / Jason / crypto maintainers,


On Mon, 2023-03-13 at 11:42 +0100, bchalios at amazon.es
wrote:
> Hi Amit,
> 
> Thanks for taking the time to look into this.
> 
> On 3/2/23 5:55 PM, Amit Shah <amit at infradead.org> 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

On Mon, 2023-03-13 at 19:05 +0100, Amit Shah wrote:
> Hey Herbert / Jason / crypto maintainers,
[...]
> 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.
I reached out to Jason on IRC, and he mentioned he will follow up with
a patch that incorporates ideas from your patch plus hooking into
random.c.  Sounds promising!

		Amit