| Message ID | 20230419170526.1883812-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix virtio/rng handling in low entropy situations | expand |
On Wed, Apr 19, 2023 at 06:05:26PM +0100, Andre Przywara wrote: > In contrast to the original v0.9 virtio spec (which was rather vague), > the virtio 1.0+ spec demands that a RNG request returns at least one > byte: > "The device MUST place one or more random bytes into the buffer, but it > MAY use less than the entire buffer length." > > Our current implementation does not prevent returning zero bytes, which > upsets an assert in EDK II. /dev/urandom should always return at least > 256 bytes of entropy, unless interrupted by a signal. > > Repeat the read if that happens, and give up if that fails as well. > This makes sure we return some entropy and become spec compliant. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > Reported-by: Sami Mujawar <sami.mujawar@arm.com> > --- > virtio/rng.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/virtio/rng.c b/virtio/rng.c > index e6e70ced3..d5959d358 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -66,8 +66,18 @@ static bool virtio_rng_do_io_request(struct kvm *kvm, struct rng_dev *rdev, stru > > head = virt_queue__get_iov(queue, iov, &out, &in, kvm); > len = readv(rdev->fd, iov, in); > - if (len < 0 && errno == EAGAIN) > - len = 0; > + if (len < 0 && (errno == EAGAIN || errno == EINTR)) { > + /* > + * The virtio 1.0 spec demands at least one byte of entropy, > + * so we cannot just return with 0 if something goes wrong. > + * The urandom(4) manpage mentions that a read from /dev/urandom > + * should always return at least 256 bytes of randomness, so I guess that's implied, but strictly speaking the manpage only states that reads of <=256 bytes succeed. Larger reads may return an error again or (if you read the man naively) zero bytes. We could increase the chance of this succeeding by setting in = 1 and iov_len = min(iov_len, 256) Thanks, Jean > + * just retry here in case we were interrupted by a signal. > + */ > + len = readv(rdev->fd, iov, in); > + if (len < 1) > + return false; > + } > > virt_queue__set_used_elem(queue, head, len); > > -- > 2.25.1 >
On Thu, Apr 20, 2023 at 02:46:27PM +0100, Jean-Philippe Brucker wrote: > On Wed, Apr 19, 2023 at 06:05:26PM +0100, Andre Przywara wrote: > > In contrast to the original v0.9 virtio spec (which was rather vague), > > the virtio 1.0+ spec demands that a RNG request returns at least one > > byte: > > "The device MUST place one or more random bytes into the buffer, but it > > MAY use less than the entire buffer length." > > > > Our current implementation does not prevent returning zero bytes, which > > upsets an assert in EDK II. /dev/urandom should always return at least > > 256 bytes of entropy, unless interrupted by a signal. > > > > Repeat the read if that happens, and give up if that fails as well. > > This makes sure we return some entropy and become spec compliant. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > Reported-by: Sami Mujawar <sami.mujawar@arm.com> > > --- > > virtio/rng.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/virtio/rng.c b/virtio/rng.c > > index e6e70ced3..d5959d358 100644 > > --- a/virtio/rng.c > > +++ b/virtio/rng.c > > @@ -66,8 +66,18 @@ static bool virtio_rng_do_io_request(struct kvm *kvm, struct rng_dev *rdev, stru > > > > head = virt_queue__get_iov(queue, iov, &out, &in, kvm); > > len = readv(rdev->fd, iov, in); > > - if (len < 0 && errno == EAGAIN) > > - len = 0; > > + if (len < 0 && (errno == EAGAIN || errno == EINTR)) { > > + /* > > + * The virtio 1.0 spec demands at least one byte of entropy, > > + * so we cannot just return with 0 if something goes wrong. > > + * The urandom(4) manpage mentions that a read from /dev/urandom > > + * should always return at least 256 bytes of randomness, so > > I guess that's implied, but strictly speaking the manpage only states that > reads of <=256 bytes succeed. Larger reads may return an error again or > (if you read the man naively) zero bytes. We could increase the chance of > this succeeding by setting in = 1 and iov_len = min(iov_len, 256) Andre -- do you plan to respin with Jean's suggestion above? Will
On Tue, 23 May 2023 11:45:03 +0100 Will Deacon <will@kernel.org> wrote: Hi Will, > On Thu, Apr 20, 2023 at 02:46:27PM +0100, Jean-Philippe Brucker wrote: > > On Wed, Apr 19, 2023 at 06:05:26PM +0100, Andre Przywara wrote: > > > In contrast to the original v0.9 virtio spec (which was rather vague), > > > the virtio 1.0+ spec demands that a RNG request returns at least one > > > byte: > > > "The device MUST place one or more random bytes into the buffer, but it > > > MAY use less than the entire buffer length." > > > > > > Our current implementation does not prevent returning zero bytes, which > > > upsets an assert in EDK II. /dev/urandom should always return at least > > > 256 bytes of entropy, unless interrupted by a signal. > > > > > > Repeat the read if that happens, and give up if that fails as well. > > > This makes sure we return some entropy and become spec compliant. > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > Reported-by: Sami Mujawar <sami.mujawar@arm.com> > > > --- > > > virtio/rng.c | 14 ++++++++++++-- > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/virtio/rng.c b/virtio/rng.c > > > index e6e70ced3..d5959d358 100644 > > > --- a/virtio/rng.c > > > +++ b/virtio/rng.c > > > @@ -66,8 +66,18 @@ static bool virtio_rng_do_io_request(struct kvm *kvm, struct rng_dev *rdev, stru > > > > > > head = virt_queue__get_iov(queue, iov, &out, &in, kvm); > > > len = readv(rdev->fd, iov, in); > > > - if (len < 0 && errno == EAGAIN) > > > - len = 0; > > > + if (len < 0 && (errno == EAGAIN || errno == EINTR)) { > > > + /* > > > + * The virtio 1.0 spec demands at least one byte of entropy, > > > + * so we cannot just return with 0 if something goes wrong. > > > + * The urandom(4) manpage mentions that a read from /dev/urandom > > > + * should always return at least 256 bytes of randomness, so > > > > I guess that's implied, but strictly speaking the manpage only states that > > reads of <=256 bytes succeed. Larger reads may return an error again or > > (if you read the man naively) zero bytes. We could increase the chance of > > this succeeding by setting in = 1 and iov_len = min(iov_len, 256) > > Andre -- do you plan to respin with Jean's suggestion above? Yes, sorry, I read too much into the suggestion at first, so it got pushed off the table. But reading that again now and looking at the code I realise that it's indeed easy to implement. I will post something shortly. Thanks, Andre.
diff --git a/virtio/rng.c b/virtio/rng.c index e6e70ced3..d5959d358 100644 --- a/virtio/rng.c +++ b/virtio/rng.c @@ -66,8 +66,18 @@ static bool virtio_rng_do_io_request(struct kvm *kvm, struct rng_dev *rdev, stru head = virt_queue__get_iov(queue, iov, &out, &in, kvm); len = readv(rdev->fd, iov, in); - if (len < 0 && errno == EAGAIN) - len = 0; + if (len < 0 && (errno == EAGAIN || errno == EINTR)) { + /* + * The virtio 1.0 spec demands at least one byte of entropy, + * so we cannot just return with 0 if something goes wrong. + * The urandom(4) manpage mentions that a read from /dev/urandom + * should always return at least 256 bytes of randomness, so + * just retry here in case we were interrupted by a signal. + */ + len = readv(rdev->fd, iov, in); + if (len < 1) + return false; + } virt_queue__set_used_elem(queue, head, len);
In contrast to the original v0.9 virtio spec (which was rather vague), the virtio 1.0+ spec demands that a RNG request returns at least one byte: "The device MUST place one or more random bytes into the buffer, but it MAY use less than the entire buffer length." Our current implementation does not prevent returning zero bytes, which upsets an assert in EDK II. /dev/urandom should always return at least 256 bytes of entropy, unless interrupted by a signal. Repeat the read if that happens, and give up if that fails as well. This makes sure we return some entropy and become spec compliant. Signed-off-by: Andre Przywara <andre.przywara@arm.com> Reported-by: Sami Mujawar <sami.mujawar@arm.com> --- virtio/rng.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)