[PATCH] virtio: rng: gracefully handle 0 byte returns

[Heinrich Schuchardt]

On 11/10/23 15:16, Andre Przywara wrote:
> On Fri, 10 Nov 2023 05:53:59 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
> Hi Simon,
>
>> On Tue, 7 Nov 2023 at 09:09, Andre Przywara <andre.przywara at arm.com> wrote:
>>>
>>> According to the virtio v1.x "entropy device" specification, a virtio-rng
>>> device is supposed to always return at least one byte of entropy.
>>> However the virtio v0.9 spec does not mention such a requirement.

v0.9 was a draft. So nothing we have to care about.

>>>
>>> The Arm Fixed Virtual Platform (FVP) implementation of virtio-rng always
>>> returns 8 bytes less of entropy than requested. If 8 bytes or less are
>>> requested, it will return 0 bytes.
>>> This behaviour makes U-Boot's virtio_rng_read() implementation go into an
>>> endless loop, hanging the system.
>>>
>>> Work around this problem by always requesting 8 bytes more than needed,
>>> but only if a previous call to virtqueue_get_buf() returned 0 bytes.
>>>
>>> This should never trigger on a v1.x spec compliant implementation, but
>>> fixes the hang on the Arm FVP.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>>> Reported-by: Peter Hoyes <peter.hoyes at arm.com>

The bug should be fixed in the Arm Fixed Virtual Platform instead of
working around it in U-Boot.

Our driver should return -EIO if the virtio host is not compliant.

Best regards

Heinrich

>>> ---
>>>   drivers/virtio/virtio_rng.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> Unrelated to this patch, but do you know the hardware architecture of
>> the ARM RNG? Is there one RNG unit per CPU or one for the whole SoC?
>
> Architecturally and from a software perspective the ARMv8.5 FEAT_RNG
> feature is a system register, so per-core. Theoretically the availability
> could differ between cores, but the CPU ID feature registers are also
> per-core, so as long as we run on a single core, or always at least
> read from the same core, it's all good.
>
> Now the architecture only describes the CPU instruction aspect of the
> feature, and establishes rules for the quality and spec conformance, but
> leaves the actual source of the entropy open to the integrator.
>
> The manual in the Neoverse V1 core[1] (still not a SoC!) states that the
> actual entropy source is a memory mapped peripheral, its address being
> held in an internal, per-core register. So you can have one shared entropy
> source per SoC, or a private instance for each core, that's up to the
> actual integrator to design.
>
>  From the software perspective this shouldn't matter, though: the feature
> is "per-core", how this is backed is an implementation detail.
>
> Cheers,
> Andre
>
> [1]
> https://developer.arm.com/documentation/101427/0102/Functional-description/Random-number-support/About-the-random-number-support
>
>
>>> diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
>>> index b85545c2ee5..786359a6e36 100644
>>> --- a/drivers/virtio/virtio_rng.c
>>> +++ b/drivers/virtio/virtio_rng.c
>>> @@ -20,7 +20,7 @@ struct virtio_rng_priv {
>>>   static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
>>>   {
>>>          int ret;
>>> -       unsigned int rsize;
>>> +       unsigned int rsize = 1;
>>>          unsigned char buf[BUFFER_SIZE] __aligned(4);
>>>          unsigned char *ptr = data;
>>>          struct virtio_sg sg;
>>> @@ -29,7 +29,12 @@ static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
>>>
>>>          while (len) {
>>>                  sg.addr = buf;
>>> -               sg.length = min(len, sizeof(buf));
>>> +               /*
>>> +                * Work around implementations which always return 8 bytes
>>> +                * less than requested, down to 0 bytes, which would
>>> +                * cause an endless loop otherwise.
>>> +                */
>>> +               sg.length = min(rsize ? len : len + 8, sizeof(buf));
>>>                  sgs[0] = &sg;
>>>
>>>                  ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
>>> --
>>> 2.25.1
>>>
>>
>> Regards,
>> Simon
>