Re: [PATCH V5 1/4] virtio_ring: validate used buffer length

[Halil Pasic]

On Wed, 24 Nov 2021 10:33:28 +0800
Jason Wang <jasowang@xxxxxxxxxx> wrote:

> > > > Let's see how far we can get. But yes, maybe we were too aggressive in
> > > > breaking things by default, a warning might be a better choice for a
> > > > couple of cycles.  
> >
> > Ok, considering we saw the issues with balloons I think I can post a
> > patch to use warn instead. I wonder if we need to taint the kernel in
> > this case.  
> 
> Rethink this, consider we still have some time, I tend to convert the
> drivers to validate the length by themselves. Does this make sense?

I do find value in doing the validation in a single place for every
driver. This is really a common concern. But I think, not breaking
what used to work before is a good idea. So I would opt for producing
a warning, but otherwise preserving old behavior unless there is an
explicit opt-in for something more strict.

BTW AFAIU if we detect a problem here, there are basically two
cases:
(1) Either the device is over-reporting what it has written, or
(2) we have a memory corruption in the guest because the device has
written beyond the end of the provided buffer. This can be because
  (2.1) the driver provided a smaller buffer than mandated by the spec,
  or
  (2.2) the device is broken.

Case (1) is relatively harmless, and I believe a warning for it is more
than appropriate. Whoever sees the warning should push for a fixed device
if possible.

Case (2) is nasty. What would be the sanest course of action if we were
reasonably sure we have have case (2.2)?

Maybe we can detect case (2) with a canary. I.e. artificially extend 
the buffer with an extra descriptor that has a poisoned buffer, and
check if the value of that poisoned buffer is different than poison. I'm
not sure it is worth the effort though in production.

Regards,
Halil