Messages in this thread | | | From | Jason Wang <> | Date | Thu, 25 Nov 2021 10:27:54 +0800 | Subject | Re: [PATCH V5 1/4] virtio_ring: validate used buffer length |
| |
On Wed, Nov 24, 2021 at 7:33 PM Halil Pasic <pasic@linux.ibm.com> wrote: > > On Wed, 24 Nov 2021 10:33:28 +0800 > Jason Wang <jasowang@redhat.com> 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.
Yes, I totally agree with you after more thought and discussion.
> > 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.
Yes.
> > Case (2) is nasty. What would be the sanest course of action if we were > reasonably sure we have have case (2.2)?
I think that's why a per driver validation is more preferable. The driver can choose the comfortable action, e.g for networking it may just drop the packets.
> > 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.
This might work but it might cause performance overhead. I still think doing the validation per driver is better, the driver can choose to fix the used length and taint the kernel anyway.
Thanks
> > Regards, > Halil >
| |