Re: [virtio-dev] [PATCH RFC 3/3] rng: leak detection support

On 2/11/23 12:25, Michael S. Tsirkin wrote:
>
>
> On Fri, Sep 22, 2023 at 05:40:50PM +0200, Babis Chalios wrote:
>>
>> On 22/9/23 17:06, Michael S. Tsirkin wrote:
>>> On Tue, Sep 19, 2023 at 12:11:37PM +0200, Babis Chalios wrote:
>>>> On 19/9/23 12:01, Michael S. Tsirkin wrote:
>>>>> On Tue, Sep 19, 2023 at 09:32:08AM +0200, Babis Chalios wrote:
>>>>>> Resending to fix e-mail formatting issues (sorry for the spam)
>>>>>>
>>>>>> On 18/9/23 18:30, Babis Chalios wrote:
>>>>>>>>>>> Yes, that's what the driver does now in the RFC patch.
>>>>>>>>>>> However, this just
>>>>>>>>>>> decreases
>>>>>>>>>>> the race window, it doesn't eliminate it. If a third
>>>>>>>>>>> leak event happens it
>>>>>>>>>>> might not
>>>>>>>>>>> find any buffers to use:
>>>>>>>>>>>
>>>>>>>>>>> 1. available buffers to queue 1-X
>>>>>>>>>>> 2. available buffers to queue X
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 3. poll queue X
>>>>>>>>>>> 4. used buffers in queue X       <- leak event 1 will
>>>>>>>>>>> use buffers in X
>>>>>>>>>>> 5. avail buffers in queue X
>>>>>>>>>>> 6. poll queue 1-X                <- leak event 2 will
>>>>>>>>>>> use buffers in 1-X
>>>>>>>>>>> 7. used buffers in queue 1-X
>>>>>>>>>>> 8. avail buffers in queue 1-X
>>>>>>>>>>>                                       <- leak event 3 (it
>>>>>>>>>>> needs buffers in X, race with step 5)
>>>>>>>>>>> 9. goto 3
>>>>>>>>>> I don't get it. we added buffers in step 5.
>>>>>>>>> What if the leak event 3 arrives before step 5 had time to
>>>>>>>>> actually add the
>>>>>>>>> buffers in X and make
>>>>>>>>> them visible to the device?
>>>>>>>> Then it will see a single event in 1-X instead of two events.  A leak is
>>>>>>>> a leak though, I don't see does it matter how many triggered.
>>>>>>>>
>>>>>> So the scenario I have in mind is the following:
>>>>>>
>>>>>> (Epoch here is terminology that I used in the Linux RFC. It is a value
>>>>>> maintained by random.c
>>>>>> that changes every time a leak event happens).
>>>>>>
>>>>>> 1. add buffers to 1-X
>>>>>> 2. add buffers to X
>>>>>> 3. poll queue X
>>>>>> 4. vcpu 0: get getrandom() entropy and cache epoch value
>>>>>> 5. Device: First snapshot, uses buffers in X
>>>>>> 6. vcpu 1: sees used buffers
>>>>>> 7. Device: Second snapshot, uses buffers in 1-X
>>>>>> 8. vcpu 0: getrandom() observes new  epoch value & caches it
>>>>>> 9. Device: Third snapshot, no buffers in either queue, (vcpu 1 from step 6
>>>>>> has not yet finished adding new buffers).
>>>>>> 10. vcpu 1 adds new buffer in X
>>>>>> 11. vcpu 0: getrandom() will not see new epoch and gets stale entropy.
>>>>>>
>>>>>>
>>>>>> In this succession of events, when the third snapshot will happen, the
>>>>>> device won't find
>>>>>> any buffers in either queue, so it won't increase the RNG epoch value. So,
>>>>>> any entropy
>>>>>> gathered after step 8 will be the same across all snapshots. Am I missing
>>>>>> something?
>>>>>>
>>>>>> Cheers,
>>>>>> Babis
>>>>>>
>>>>> Yes but notice how this is followed by:
>>>>>
>>>>> 12. vcpu 1: sees used buffers in 1-X
>>>>>
>>>>> Driver can notify getrandom I guess?
>>>> It could, but then we have the exact race condition that VMGENID had,
>>>> userspace has already consumed stale entropy and there's nothing we
>>>> can do about that.
>>>>
>>>> Although this is indeed a corner case, it feels like it beats the purpose
>>>> of having the hardware update directly userspace (via copy on leak).
>>>>
>>>> How do you feel about the proposal a couple of emails back? It looks to
>>>> me that it avoids completely the race condition.
>>>>
>>>> Cheers,
>>>> Babis
>>> It does. The problem of course is that this means that e.g.
>>> taking a snapshot of a guest that is stuck won't work well.
>> That is true, but does it matter? The intention of the proposal
>> is that if it is not safe to take snapshots (i.e. no buffers in the
>> queue) don't take snapshots.
> OK. Basically I think if there's a way for device to detect that
> guest is stuck and not refilling the queue in a timely
> manner, then we are ok - host will make its own decisions
> on whether to snapshot or not.
>
> However, I feel in that case we need a way to create a big
> backlog of buffers for guest to fill such that this
> ring empty condition is very unlikely.
> One or even 2 queues does not seem enough then.
>
> For example, I can see a "stop" command that will
> tell device: "stop consuming buffers" and device
> will stop consuming buffers until the next leak event.
>
Yup, that seems reasonable to me. In that case, we could
have a single queue, where the driver will fill up with multiple
batches of commands where the last one is "stop" command, and
then back-fill as needed.

That should make it very unlikely for a well-behaving guest to
run out of entropy leak commands in the queue. However, even
with that design, I think there is value in putting something in the
spec about the fact that the host might want to consider if it should,
or not, take a snapshot when it finds the leak queue empty.

Cheers,
Babis

>
>
>>> I have been thinking of adding MAP/UNMAP descriptors for
>>> a while now. Thus it will be possible to modify
>>> userspace memory without consuming buffers.
>>> Would something like this solve the problem?
>> I am not familiar with MAP/UNMAP descriptors. Is there
>> a link where I can read about them?
>>
>> Cheers,
>> Babis


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]