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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 25 Nov 2021 10:27:54 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        "Hetzelt, Felicitas" <f.hetzelt@...berlin.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "kaplan, david" <david.kaplan@....com>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>, mcgrof@...nel.org,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH V5 1/4] virtio_ring: validate used buffer length

On Wed, Nov 24, 2021 at 7:33 PM Halil Pasic <pasic@...ux.ibm.com> wrote:
>
> On Wed, 24 Nov 2021 10:33:28 +0800
> Jason Wang <jasowang@...hat.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
>