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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>,
	virtualization <virtualization@lists.linux-foundation.org>,
	"Hetzelt, Felicitas" <f.hetzelt@tu-berlin.de>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"kaplan, david" <david.kaplan@amd.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Stefano Garzarella <sgarzare@redhat.com>
Subject: Re: [PATCH V5 1/4] virtio_ring: validate used buffer length
Date: Tue, 23 Nov 2021 06:05:39 -0500	[thread overview]
Message-ID: <20211123055906-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <CACGkMEtmhwDEAvMuMhQEUB-b+=n713pVvjyct8QAqMUk1H-A-g@mail.gmail.com>

On Tue, Nov 23, 2021 at 10:25:20AM +0800, Jason Wang wrote:
> On Tue, Nov 23, 2021 at 4:24 AM Halil Pasic <pasic@linux.ibm.com> wrote:
> >
> > On Mon, 22 Nov 2021 14:25:26 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >
> > > I think the fixes are:
> > >
> > > 1) fixing the vhost vsock
> > > 2) use suppress_used_validation=true to let vsock driver to validate
> > > the in buffer length
> > > 3) probably a new feature so the driver can only enable the validation
> > > when the feature is enabled.
> >
> > I'm not sure, I would consider a F_DEV_Y_FIXED_BUG_X a perfectly good
> > feature. Frankly the set of such bugs is device implementation
> > specific and it makes little sense to specify a feature bit
> > that says the device implementation claims to adhere to some
> > aspect of the specification. Also what would be the semantic
> > of not negotiating F_DEV_Y_FIXED_BUG_X?
> 
> Yes, I agree. Rethink of the feature bit, it seems unnecessary,
> especially considering the driver should not care about the used
> length for tx.
> 
> >
> > On the other hand I see no other way to keep the validation
> > permanently enabled for fixed implementations, and get around the problem
> > with broken implementations. So we could have something like
> > VHOST_USED_LEN_STRICT.
> 
> It's more about a choice of the driver's knowledge. For vsock TX it
> should be fine. If we introduce a parameter and disable it by default,
> it won't be very useful.
> 
> >
> > Maybe, we can also think of 'warn and don't alter behavior' instead of
> > 'warn' and alter behavior. Or maybe even not having such checks on in
> > production, but only when testing.
> 
> I think there's an agreement that virtio drivers need more hardening,
> that's why a lot of patches were merged. Especially considering the
> new requirements came from confidential computing, smart NIC and
> VDUSE. For virtio drivers, enabling the validation may help to
> 
> 1) protect the driver from the buggy and malicious device
> 2) uncover the bugs of the devices (as vsock did, and probably rpmsg)
> 3) force the have a smart driver that can do the validation itself
> then we can finally remove the validation in the core
> 
> So I'd like to keep it enabled.
> 
> Thanks

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.

> >
> > Regards,
> > Halil
> >