RE: [PATCH v1 2/6] virtio console: Harden port adding

From: "Reshetova, Elena" <elena.reshetova@intel.com>
To: "Michael S. Tsirkin" <mst@redhat.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"virtualization@lists.linux-foundation.org" 
	<virtualization@lists.linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	Andi Kleen <ak@linux.intel.com>, Amit Shah <amit@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>
Subject: RE: [PATCH v1 2/6] virtio console: Harden port adding
Date: Thu, 2 Feb 2023 12:02:45 +0000	[thread overview]
Message-ID: <DM8PR11MB57503A4325D2955D96F581C8E7D69@DM8PR11MB5750.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230127094425-mutt-send-email-mst@kernel.org>

> On Fri, Jan 27, 2023 at 04:17:46PM +0200, Alexander Shishkin wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >
> > > On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
> > >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > >>
> > >> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> > >> >> We can have shared pages between the host and guest without bounce
> > >> >> buffers in between, so they can be both looking directly at the same
> > >> >> page.
> > >> >>
> > >> >> Regards,
> > >> >
> > >> > How does this configuration work? What else is in this page?
> > >>
> > >> So, for example in TDX, you have certain pages as "shared", as in
> > >> between guest and hypervisor. You can have virtio ring(s) in such
> > >> pages. It's likely that there'd be a swiotlb buffer there instead, but
> > >> sharing pages between host virtio and guest virtio drivers is possible.
> > >
> > > If it is shared, then what does this mean?  Do we then need to copy
> > > everything out of that buffer first before doing anything with it
> > > because the data could change later on?  Or do we not trust anything in
> > > it at all and we throw it away?  Or something else (trust for a short
> > > while and then we don't?)
> >
> > The first one, we need a consistent view of the metadata (the ckpt in
> > this case), so we take a snapshot of it. Then, we validate it (because
> > we don't trust it) to be correct. If it is not, we discard it, otherwise
> > we act on it. Since this is a ring, we just move on to the next record
> > if there is one.
> >
> > Meanwhile, in the shared page, it can change from correct to incorrect,
> > but it won't affect us because we have this consistent view at the
> > moment the snapshot was taken.
> >
> > > Please be specific as to what you want to see happen here, and why.
> >
> > For example, if we get a control message to add a port and
> > cpkt->event==PORT_ADD, we skip validation of cpkt->id (port id), because
> > we're intending to add a new one. At this point, the device can change
> > cpkt->event to PORT_REMOVE, which does require a valid cpkt->id and the
> > subsequent code runs into a NULL dereference on the port value, which
> > should have been looked up from cpkt->id.
> >
> > Now, if we take a snapshot of cpkt, we naturally don't have this
> > problem, because we're looking at a consistent state of cpkt: it's
> > either PORT_ADD or PORT_REMOVE all the way. Which is what this patch
> > does.
> >
> > Does this answer your question?
> >
> > Thanks,
> > --
> > Alex
> 
> 
> Not sure about Greg but it doesn't answer my question because either the
> bad device has access to all memory at which point it's not clear why
> is it changing cpkt->event and not e.g. stack. Or it's restricted to
> only access memory when mapped through the DMA API. Which is not the
> case here.

We do enforce virtio usage via DMA API only for TDX guest. Alex has a patch
queued for that also. 
But not sure if this addresses your concern here. 

Best Regards,
Elena.