mbox series

[v2,0/2,RFC] virtio-rng entropy leak reporting feature

Message ID 20230131145543.86369-1-bchalios@amazon.es (mailing list archive)
Headers show
Series virtio-rng entropy leak reporting feature | expand

Message

Babis Chalios Jan. 31, 2023, 2:55 p.m. UTC 
Recently, a proposal has been published [1] for a new feature in the
VirtIO RNG device which will allows the device to report "entropy leaks"
to the guest VM. Such an event occurs when, for example, we take a VM
snapshot, or when we restore a VM from a snapshot.

The feature allows the guest to request for certain operations to be
performed upon an entropy leak event. When such an event occurs, the
device will handle the requests and add the request buffers to the used
queue. Adding these buffers to the used queue operates as a notification
towards the guest about the entropy leak event.

The proposed changes describe two types of requests that can be
performed: (1) fill a buffer in guest memory with random bytes and (2)
perform a memory copy between two buffers in guest memory.

The mechanism provides similar functionality to Microsoft's Virtual
Machine Generation ID and it can be used to re-seed the kernel's PRNG
upon taking a VM snapshot or resuming from one. Additionally, it allows
to (1) avoid the race-condition that exists with our VMGENID
implementation, between the time a VM is resumed after a "leak event"
and the handling of the ACPI notification before adding the new entropy.
Finally, it allows building on top of it to provide a mechanism for
notifying user-space about such events.

The first patch of this series, extends the current virtio-rng driver to
implement the new feature and ensures that there is always a request to
get some random bytes from the device in the event of an entropy leak
and uses these bytes as entropy through the `add_device_randomness`.

The second patch adds a copy-on-leak command as well in the queue,
implementating the idea of a generation counter that has previously been
part of the VMGENID saga. It then exposes the value of the generation
counter over a sysfs file. User-space can read, mmap and poll on the
file in order to be notified about entropy leak events.

I have performed basic tests of the user-space interfaces using a
Firecracker where I implemented virtio-rng with the proposed features.
Instructions on how to replicate this can be found here:
https://github.com/bchalios/virtio-snapsafe-example

The patchset does not solve all problems. We do not define an API for
other parts of the kernel to be able to use directly the new
functionality (add commands to the queue), mainly because I 'm not sure
what would the correct API be. I was toying with the idea of extending
`struct hwrng` with two new hooks that would be implemented only by
virtio-rng but I'm not sure I like it, so I am open to suggestions.

As a result of the above, the way we use the functionality to add new
entropy, i.e. calling `add_device_randomness`, is as racy as the VMGENID
case, since it relies on used buffers been handled by the virtio driver.

As for user-space, the `mmap` interface *is* race-free. Changes in the
generation counter will be observable by user applications the moment VM
vcpus resume. However, the `poll` interface isn't, `sysfs_notify` is
being called as well when the virtio driver handles used buffers. I am
not sure I have a solution for this last one.

Posting this, I hope we can resume the discussion about solving the
above issues (or any other issue that I haven't thought of), especially
with regards to providing a mechanism suitable for user-space
notifications.

Cheers,
Babis

Changes in v2: fix kbuild warnings

Babis Chalios (2):
  virtio-rng: implement entropy leak feature
  virtio-rng: add sysfs entries for leak detection

 drivers/char/hw_random/virtio-rng.c | 372 +++++++++++++++++++++++++++-
 include/uapi/linux/virtio_rng.h     |   3 +
 2 files changed, 368 insertions(+), 7 deletions(-)

Comments

Jason A. Donenfeld Jan. 31, 2023, 4:27 p.m. UTC | #1 
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:

| Hi Babis,
| 
| As I mentioned to you privately this week, I'm about to be out of town,
| so I won't be able to look at this until I'm back in a few weeks. I
| appreciate your patience.
| 
| But as a cursory look, I'm happy that you've written the hardware-side
| code for this. That's a great starting point. The plumbing is not so
| nice, though. This needs to be integrated more closely with random.c
| itself, similar to how vmgenid works.
| 
| When I'm back in a few weeks, I'll see if I can either write a
| description of what I have in mind, or simply integrate the useful
| hardware work here into an expanded patch series.
| 
| [Please don't merge anything for now.]

So: you wrote some maybe useful hardware code. The rest is wrong. And we
haven't even concluded discussions on whether the virtio interface is
the right one. In fact, I had previously asked if we could schedule this
all until March. Marco from your team then sent an impatient email, so I
said, alright, what about Feb 11 when I'm back. That's annoying for me
but I figured I'd just shuffle everything around and prioritize this.
Then, instead of waiting for that, you posted v1 of this patchset the
next day. I asked you again. And now, while I'm away on the first
holiday in a while with very little connectivity and no laptop, you post
a v2. So I'm really annoyed. In order to avoid all doubt about this, let
me then just NACK this, and I'll lift the nack when I'm back:

    Nacked-by: Jason A. Donenfeld <Jason@zx2c4.com>
Babis Chalios Jan. 31, 2023, 5:06 p.m. UTC | #2 
Hi Jason,

I really didn't mean to interrupt your holidays.

I posted this as an RFC exactly because I don't want this to be merged _as is_, it's just a trigger for discussion.
It is v2 just as a response to Michael's comments in the previous version and fixes for the CI.

Also, there are other people that expressed interest in this and they wanted to participate in the coversation, so that's just
that.

I hope you enjoy your vacation, get some rest and speak to you once you' re back :)

Cheers,
Babis

On 1/31/23 5:27 PM, "Jason A. Donenfeld" <Jason@zx2c4.com> 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.
> 
> 
> 
> 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:
> 
> | Hi Babis,
> |
> | As I mentioned to you privately this week, I'm about to be out of town,
> | so I won't be able to look at this until I'm back in a few weeks. I
> | appreciate your patience.
> |
> | But as a cursory look, I'm happy that you've written the hardware-side
> | code for this. That's a great starting point. The plumbing is not so
> | nice, though. This needs to be integrated more closely with random.c
> | itself, similar to how vmgenid works.
> |
> | When I'm back in a few weeks, I'll see if I can either write a
> | description of what I have in mind, or simply integrate the useful
> | hardware work here into an expanded patch series.
> |
> | [Please don't merge anything for now.]
> 
> So: you wrote some maybe useful hardware code. The rest is wrong. And we
> haven't even concluded discussions on whether the virtio interface is
> the right one. In fact, I had previously asked if we could schedule this
> all until March. Marco from your team then sent an impatient email, so I
> said, alright, what about Feb 11 when I'm back. That's annoying for me
> but I figured I'd just shuffle everything around and prioritize this.
> Then, instead of waiting for that, you posted v1 of this patchset the
> next day. I asked you again. And now, while I'm away on the first
> holiday in a while with very little connectivity and no laptop, you post
> a v2. So I'm really annoyed. In order to avoid all doubt about this, let
> me then just NACK this, and I'll lift the nack when I'm back:
> 
>      Nacked-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936
Amit Shah March 2, 2023, 4:55 p.m. UTC | #3 
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.

Somehow it feels like I don't have the complete story for this feature
set, having missed the discussion during LPC.

	Amit
Babis Chalios March 13, 2023, 10:42 a.m. UTC | #4 
Hi Amit,

Thanks for taking the time to look into this.

On 3/2/23 5:55 PM, Amit Shah <amit@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.

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!


> 
> Somehow it feels like I don't have the complete story for this feature
> set, having missed the discussion during LPC.
> 
>          Amit
> 


Cheers,
Babis
Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936
Amit Shah March 13, 2023, 6:05 p.m. UTC | #5 
Hey Herbert / Jason / crypto maintainers,


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