Re: [RFC PATCH 1/1] virtio: write back features before verify
On Fri, Oct 01, 2021 at 09:21:25AM +0200, Halil Pasic wrote:
> On Thu, 30 Sep 2021 07:12:21 -0400
> "Michael S. Tsirkin" wrote:
>
> > On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote:
> > > This patch fixes a regression introduced by commit 82e89ea077b9
> > > ("virtio-blk: Add validation for block size in config space") and
> > > enables similar checks in verify() on big endian platforms.
> > >
> > > The problem with checking multi-byte config fields in the verify
> > > callback, on big endian platforms, and with a possibly transitional
> > > device is the following. The verify() callback is called between
> > > config->get_features() and virtio_finalize_features(). That we have a
> > > device that offered F_VERSION_1 then we have the following options
> > > either the device is transitional, and then it has to present the legacy
> > > interface, i.e. a big endian config space until F_VERSION_1 is
> > > negotiated, or we have a non-transitional device, which makes
> > > F_VERSION_1 mandatory, and only implements the non-legacy interface and
> > > thus presents a little endian config space. Because at this point we
> > > can't know if the device is transitional or non-transitional, we can't
> > > know do we need to byte swap or not.
> >
> > Hmm which transport does this refer to?
>
> It is the same with virtio-ccw and virtio-pci. I see the same problem
> with both on s390x. I didn't try with virtio-blk-pci-non-transitional
> yet (have to figure out how to do that with libvirt) for pci I used
> virtio-blk-pci.
>
> > Distinguishing between legacy and modern drivers is transport
> > specific. PCI presents
> > legacy and modern at separate addresses so distinguishing
> > between these two should be no trouble.
>
> You mean the device id? Yes that is bolted down in the spec, but
> currently we don't exploit that information. Furthermore there
> is a fat chance that with QEMU even the allegedly non-transitional
> devices only present a little endian config space after VERSION_1
> was negotiated. Namely get_config for virtio-blk is implemented in
> virtio_blk_update_config() which does virtio_stl_p(vdev,
> &blkcfg.blk_size, blk_size) and in there we don't care
> about transitional or not:
>
> static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
> {
> #if defined(LEGACY_VIRTIO_IS_BIENDIAN)
> return virtio_is_big_endian(vdev);
> #elif defined(TARGET_WORDS_BIGENDIAN)
> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> return false;
> }
> return true;
> #else
> return false;
> #endif
> }
>
ok so that's a QEMU bug. Any virtio 1.0 and up
compatible device must use LE.
It can also present a legacy config space where the
endian depends on the guest.
> > Channel i/o has versioning so same thing?
> >
>
> Don't think so. Both a transitional and a non-transitional device
> would have to accept revisions higher than 0 if the driver tried to
> negotiate those (and we do in our case).
Yes, the modern driver does. And that one is known to be LE.
legacy driver doesn't.
> > > The virtio spec explicitly states that the driver MAY read config
> > > between reading and writing the features so saying that first accessing
> > > the config before feature negotiation is done is not an option. The
> > > specification ain't clear about setting the features multiple times
> > > before FEATURES_OK, so I guess that should be fine.
> > >
> > > I don't consider this patch super clean, but frankly I don't think we
> > > have a ton of options. Another option that may or man not be cleaner,
> > > but is also IMHO much uglier is to figure out whether the device is
> > > transitional by rejecting _F_VERSION_1, then resetting it and proceeding
> > > according tho what we have figured out, hoping that the characteristics
> > > of the device didn't change.
> >
> > I am confused here. So is the problem at the device or at the driver level?
>
> We have a driver regression. Since the 82e89ea077b9 ("virtio-blk: Add
> validation for block size in config space") virtio-blk is broken on
> s390.
Because of a qemu bug. I agree. It's worth working around in the driver
since the qemu bug has been around for a very long time.
> The deeper problem is in the spec. We stated that the driver may read
> config space before the feature negotiation is finalized, but we didn't
> think enough about what happens when native endiannes is not little
> endian in the different cases.
Because the spec is very clear that endian-ness is LE.
I don't see a spec issue yet here, just an implementation issue.
> I believe, for non-transitional devices we have a problem in the host as
> well (i.e. in QEMU).
Because QEMU ignores the spec and instead relies on the feature
negotiation.
>
> > I suspect it's actually the host that has the issue, not
> > the guest?
>
> I tend to say we have a problem both in the host and in the gue
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote: > > Do you have a list of specific drivers and kernel options that you > > feel you now "trust"? > > For TDX it's currently only virtio net/block/console > > But we expect this list to grow slightly over time, but not at a high rate > (so hopefully <10) Well there are already >10 virtio drivers and I think it's reasonable that all of these will be used with encrypted guests. The list will grow. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote: > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote: > > > Do you have a list of specific drivers and kernel options that you > > > feel you now "trust"? > > > > For TDX it's currently only virtio net/block/console > > > > But we expect this list to grow slightly over time, but not at a high rate > > (so hopefully <10) > > Well there are already >10 virtio drivers and I think it's reasonable > that all of these will be used with encrypted guests. The list will > grow. What is keeping "all" drivers from being on this list? How exactly are you determining what should, and should not, be allowed? How can drivers move on, or off, of it over time? And why not just put all of that into userspace and have it pick and choose? That should be the end-goal here, you don't want to encode policy like this in the kernel, right? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, Sep 30, 2021 at 01:31:04PM +0200, Cornelia Huck wrote:
> On Thu, Sep 30 2021, Halil Pasic wrote:
>
> > On Thu, 30 Sep 2021 11:28:23 +0200
> > Cornelia Huck wrote:
> >
> >> On Thu, Sep 30 2021, Halil Pasic wrote:
> >>
> >> > This patch fixes a regression introduced by commit 82e89ea077b9
> >> > ("virtio-blk: Add validation for block size in config space") and
> >> > enables similar checks in verify() on big endian platforms.
> >> >
> >> > The problem with checking multi-byte config fields in the verify
> >> > callback, on big endian platforms, and with a possibly transitional
> >> > device is the following. The verify() callback is called between
> >> > config->get_features() and virtio_finalize_features(). That we have a
> >> > device that offered F_VERSION_1 then we have the following options
> >> > either the device is transitional, and then it has to present the legacy
> >> > interface, i.e. a big endian config space until F_VERSION_1 is
> >> > negotiated, or we have a non-transitional device, which makes
> >> > F_VERSION_1 mandatory, and only implements the non-legacy interface and
> >> > thus presents a little endian config space. Because at this point we
> >> > can't know if the device is transitional or non-transitional, we can't
> >> > know do we need to byte swap or not.
> >> >
> >> > The virtio spec explicitly states that the driver MAY read config
> >> > between reading and writing the features so saying that first accessing
> >> > the config before feature negotiation is done is not an option. The
> >> > specification ain't clear about setting the features multiple times
> >> > before FEATURES_OK, so I guess that should be fine.
> >> >
> >> > I don't consider this patch super clean, but frankly I don't think we
> >> > have a ton of options. Another option that may or man not be cleaner,
> >> > but is also IMHO much uglier is to figure out whether the device is
> >> > transitional by rejecting _F_VERSION_1, then resetting it and proceeding
> >> > according tho what we have figured out, hoping that the characteristics
> >> > of the device didn't change.
> >> >
> >> > Signed-off-by: Halil Pasic
> >> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in
> >> > config space")
> >> > Reported-by: mark...@us.ibm.com
> >> > ---
> >> > drivers/virtio/virtio.c | 4
> >> > 1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >> > index 0a5b54034d4b..9dc3cfa17b1c 100644
> >> > --- a/drivers/virtio/virtio.c
> >> > +++ b/drivers/virtio/virtio.c
> >> > @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
> >> > if (device_features & (1ULL << i))
> >> > __virtio_set_bit(dev, i);
> >> >
> >> > +/* Write back features before validate to know endianness */
> >> > +if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> >> > +dev->config->finalize_features(dev);
> >>
> >> This really looks like a mess :(
> >>
> >> We end up calling ->finalize_features twice: once before ->validate, and
> >> once after, that time with the complete song and dance. The first time,
> >> we operate on one feature set; after validation, we operate on another,
> >> and there might be interdependencies between the two (like a that a bit
> >> is cleared because of another bit, which would not happen if validate
> >> had a chance to clear that bit before).
> >
> > Basically the second set is a subset of the first set.
>
> I don't think that's clear.
>
> >
> >>
> >> I'm not sure whether that is even a problem in the spec: while the
> >> driver may read the config before finally accepting features
> >
> > I'm not sure I'm following you. Let me please qoute the specification:
> > """
> > 4. Read device feature bits, and write the subset of feature bits
> > understood by the OS and driver to the device. During this step the driver
> > MAY read (but MUST NOT write) the device-specific configuration fields to
> > check that it can support the device before accepting it.
> > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature
> > bits after this step.
> > """
> > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-930001
>
> Yes, exactly, it MAY read before accepting features. How does the device
> know whether the config space is little-endian or not?
I think it knows simply because the spec says it's little-endian.
> >
> >> , it does
> >> not really make sense to do so before a feature bit as basic as
> >> VERSION_1 which determines the endianness has been negotiated.
> >
> > Are you suggesting that ->verify() should be after
> > virtio_finalize_features()?
>
> No, that would defeat the entire purpose of verify. After
> virtio_finalize_features(), we are done with feature negotiation.
>
> > Wouldn't
> > that mean that verify() can't reject feature bits? But that is the whole
> > point of commit 82e89
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote: On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote: On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote: Do you have a list of specific drivers and kernel options that you feel you now "trust"? For TDX it's currently only virtio net/block/console But we expect this list to grow slightly over time, but not at a high rate (so hopefully <10) Well there are already >10 virtio drivers and I think it's reasonable that all of these will be used with encrypted guests. The list will grow. What is keeping "all" drivers from being on this list? It would be too much work to harden them all, and it would be pointless because all these drivers are never legitimately needed in a virtualized environment which only virtualize a very small number of devices. How exactly are you determining what should, and should not, be allowed? Everything that has had reasonable effort at hardening can be added. But if someone proposes to add a driver that should trigger additional scrutiny in code review. We should also request them to do some fuzzing. It's a bit similar to someone trying to add a new syscall interface. That also triggers much additional scrutiny for good reasons and people start fuzzing it. How can drivers move on, or off, of it over time? Adding something is submitting a patch to the allow list. I'm not sure the "off" case would happen, unless the driver is completely removed, or maybe it has some unfixable security problem. But that is all rather unlikely. And why not just put all of that into userspace and have it pick and choose? That should be the end-goal here, you don't want to encode policy like this in the kernel, right? How would user space know what drivers have been hardened? This is really something that the kernel needs to determine. I don't think we can outsource it to anyone else. Also BTW of course user space can still override it, but really the defaults should be a kernel policy. There's also the additional problem that one of the goals of confidential guest is to just move existing guest virtual images into them without much changes. So it's better for such a case if as much as possible of the policy is in the kernel. But that's more a secondary consideration, the first point is really the important part. -Andi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote: > > On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote: > > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote: > > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote: > > > > >Do you have a list of specific drivers and kernel options that you > > > > > feel you now "trust"? > > > > For TDX it's currently only virtio net/block/console > > > > > > > > But we expect this list to grow slightly over time, but not at a high > > > > rate > > > > (so hopefully <10) > > > Well there are already >10 virtio drivers and I think it's reasonable > > > that all of these will be used with encrypted guests. The list will > > > grow. > > What is keeping "all" drivers from being on this list? > > It would be too much work to harden them all, and it would be pointless > because all these drivers are never legitimately needed in a virtualized > environment which only virtualize a very small number of devices. Why would you not want to properly review and fix up all kernel drivers? That feels like you are being lazy. What exactly are you meaning by "harden"? Why isn't it automated? Who is doing this work? Where is it being done? Come on, you have a small number of virtio drivers, to somehow say that they are now divided up into trusted/untrusted feels very very odd. Just do the real work here, everyone will benefit, including yourself. > > How exactly are > > you determining what should, and should not, be allowed? > > Everything that has had reasonable effort at hardening can be added. But if > someone proposes to add a driver that should trigger additional scrutiny in > code review. We should also request them to do some fuzzing. You can provide that fuzzing right now, why isn't syzbot running on these interfaces today? And again, what _exactly_ do you all mean by "hardening" that has happened? Where is that documented and who did that work? > > And why not just put all of that into userspace and have it pick and > > choose? That should be the end-goal here, you don't want to encode > > policy like this in the kernel, right? > > How would user space know what drivers have been hardened? This is really > something that the kernel needs to determine. I don't think we can outsource > it to anyone else. It would "know" just as well as you know today in the kernel. There is no difference here. Just do the real work here, and "harden" all of the virtio drivers please. What prevents that? > Also BTW of course user space can still override it, but really the defaults > should be a kernel policy. > > There's also the additional problem that one of the goals of confidential > guest is to just move existing guest virtual images into them without much > changes. So it's better for such a case if as much as possible of the policy > is in the kernel. But that's more a secondary consideration, the first point > is really the important part. Where exactly are all of these "goals" and "requirements" documented and who is defining them and forcing them on us without actually doing any of the work involved? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Fri, Oct 01, 2021 at 05:18:46PM +0200, Cornelia Huck wrote: > I'd say we need a hack here so that we assume little-endian config space > if VERSION_1 has been offered; if your patch here works, I assume QEMU > does what we expect (assmuming little-endian as well.) I'm mostly > wondering what happens if you use a different VMM; can we expect it to > work similar to QEMU? Hard to say of course ... hopefully other VMMs are actually implementing the spec. E.g. IIUC rust vmm is modern only. > Even if it helps for s390, we should double-check > what happens for other architectures. > > > > >> > >> Anyone have any better suggestions? > >> > > > > There is the conditional compile, as an option but I would not say it is > > better. > > Yes, I agree. > > Anyone else have an idea? This is a nasty regression; we could revert the > patch, which would remove the symptoms and give us some time, but that > doesn't really feel right, I'd do that only as a last resort. Well we have Halil's hack (except I would limit it to only apply to BE, only do devices with validate, and only in modern mode), and we will fix QEMU to be spec compliant. Between these why do we need any conditional compiles? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, Sep 30, 2021 at 01:36:27PM +0200, Cornelia Huck wrote:
> On Thu, Sep 30 2021, "Michael S. Tsirkin" wrote:
>
> > On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote:
> >> This patch fixes a regression introduced by commit 82e89ea077b9
> >> ("virtio-blk: Add validation for block size in config space") and
> >> enables similar checks in verify() on big endian platforms.
> >>
> >> The problem with checking multi-byte config fields in the verify
> >> callback, on big endian platforms, and with a possibly transitional
> >> device is the following. The verify() callback is called between
> >> config->get_features() and virtio_finalize_features(). That we have a
> >> device that offered F_VERSION_1 then we have the following options
> >> either the device is transitional, and then it has to present the legacy
> >> interface, i.e. a big endian config space until F_VERSION_1 is
> >> negotiated, or we have a non-transitional device, which makes
> >> F_VERSION_1 mandatory, and only implements the non-legacy interface and
> >> thus presents a little endian config space. Because at this point we
> >> can't know if the device is transitional or non-transitional, we can't
> >> know do we need to byte swap or not.
> >
> > Hmm which transport does this refer to?
> > Distinguishing between legacy and modern drivers is transport
> > specific. PCI presents
> > legacy and modern at separate addresses so distinguishing
> > between these two should be no trouble.
>
> Hm, what about transitional devices?
transitional devices can be accessed through a modern
or a legacy interface, not both. Device knows how
it's accessed. It should key endian-ness decisions on
this not on feature negotiation.
> > Channel i/o has versioning so same thing?
>
> It can turn off VERSION_1, but not legacy. (I had hacked up a patchset
> to potentially disable legacy some time ago, but did not have any
> resources to follow up on this.)
That's ok, my point is that revision is negotiated before config
accesses, IIUC a legacy driver expecting BE will use revision 0, modern
one will use revision 1 and up.
>
> >
> >> The virtio spec explicitly states that the driver MAY read config
> >> between reading and writing the features so saying that first accessing
> >> the config before feature negotiation is done is not an option. The
> >> specification ain't clear about setting the features multiple times
> >> before FEATURES_OK, so I guess that should be fine.
> >>
> >> I don't consider this patch super clean, but frankly I don't think we
> >> have a ton of options. Another option that may or man not be cleaner,
> >> but is also IMHO much uglier is to figure out whether the device is
> >> transitional by rejecting _F_VERSION_1, then resetting it and proceeding
> >> according tho what we have figured out, hoping that the characteristics
> >> of the device didn't change.
> >
> > I am confused here. So is the problem at the device or at the driver level?
> > I suspect it's actually the host that has the issue, not
> > the guest?
>
> >From my perspective the problem is that the version of the device
> remains in limbo as long as the features have not yet been finalized,
> which means that the endianness of the config space remains in limbo as
> well. Both device and driver might come to different conclusions.
Version === legacy versus modern?
It is true that feature negotiation can not be used by device to decide that
question simply because it happens too late.
So let's not use it for that then ;)
Yes we have VERSION_1 which looks like it should allow this, but
unfortunately it only helps with that for the driver, not the device.
In practice legacy versus modern has to be determined by
transport specific versioning, luckily we have that for all
specified transports (can't say what happens with rproc).
>
> >
> >
> >> Signed-off-by: Halil Pasic
> >> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
> >> space")
> >> Reported-by: mark...@us.ibm.com
> >> ---
> >> drivers/virtio/virtio.c | 4
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >> index 0a5b54034d4b..9dc3cfa17b1c 100644
> >> --- a/drivers/virtio/virtio.c
> >> +++ b/drivers/virtio/virtio.c
> >> @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
> >>if (device_features & (1ULL << i))
> >>__virtio_set_bit(dev, i);
> >>
> >> + /* Write back features before validate to know endianness */
> >> + if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> >> + dev->config->finalize_features(dev);
> >> +
> >>if (drv->validate) {
> >>err = drv->validate(dev);
> >>if (err)
> >>
> >> base-commit: 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
> >> --
> >> 2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/m
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote: > > On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote: > > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote: > > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote: > > > > >Do you have a list of specific drivers and kernel options that you > > > > > feel you now "trust"? > > > > For TDX it's currently only virtio net/block/console > > > > > > > > But we expect this list to grow slightly over time, but not at a high > > > > rate > > > > (so hopefully <10) > > > Well there are already >10 virtio drivers and I think it's reasonable > > > that all of these will be used with encrypted guests. The list will > > > grow. > > What is keeping "all" drivers from being on this list? > > It would be too much work to harden them all, and it would be pointless > because all these drivers are never legitimately needed in a virtualized > environment which only virtualize a very small number of devices. > > > How exactly are > > you determining what should, and should not, be allowed? > > Everything that has had reasonable effort at hardening can be added. But if > someone proposes to add a driver that should trigger additional scrutiny in > code review. We should also request them to do some fuzzing. Looks like out of tree modules get a free pass then. Which is exactly the reverse of what it should be, people who spent the time to get their drivers into the kernel expect that if kernel decides to change some API their driver is automatically updated. This was always the social contract, was it not? > It's a bit similar to someone trying to add a new syscall interface. That > also triggers much additional scrutiny for good reasons and people start > fuzzing it. > > > >How can > > drivers move on, or off, of it over time? > > Adding something is submitting a patch to the allow list. > > I'm not sure the "off" case would happen, unless the driver is completely > removed, or maybe it has some unfixable security problem. But that is all > rather unlikely. > > > > > > And why not just put all of that into userspace and have it pick and > > choose? That should be the end-goal here, you don't want to encode > > policy like this in the kernel, right? > > How would user space know what drivers have been hardened? This is really > something that the kernel needs to determine. I don't think we can outsource > it to anyone else. IIUC userspace is the distro. It can also do more than a binary on/off, e.g. it can decide "only virtio", "no out of tree drivers". A distro can also ship configs with a specific features enabled/disabled. E.g. I can see where some GPU drivers will be included by some distros since they are so useful, and excluded by others since they are so big and hard to audit. I don't see how the kernel can reasonably make a stand here. Is "some audit and some fuzzing" a good policy? How much is enough? > Also BTW of course user space can still override it, but really the defaults > should be a kernel policy. Well if userspace sets the policy then I'm not sure we also want a kernel one ... but if yes I'd like it to be in a central place so whoever is building the kernel can tweak it easily and rebuild, without poking at individual drivers. > There's also the additional problem that one of the goals of confidential > guest is to just move existing guest virtual images into them without much > changes. So it's better for such a case if as much as possible of the policy > is in the kernel. If it's e.g. the kernel command line then we can set that when building the kernel right? No need for a dedicated interface just for this ... > But that's more a secondary consideration, the first point > is really the important part. > > > -Andi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Sat, 2 Oct 2021 14:20:47 -0400 "Michael S. Tsirkin" wrote: > > >From my perspective the problem is that the version of the device > > remains in limbo as long as the features have not yet been finalized, > > which means that the endianness of the config space remains in limbo as > > well. Both device and driver might come to different conclusions. > > Version === legacy versus modern? > It is true that feature negotiation can not be used by device to decide that > question simply because it happens too late. > So let's not use it for that then ;) > > Yes we have VERSION_1 which looks like it should allow this, but > unfortunately it only helps with that for the driver, not the device. > > In practice legacy versus modern has to be determined by > transport specific versioning, luckily we have that for all > specified transports (can't say what happens with rproc). So if we look at ccw, you say that the revision negotiation already determines whether VERSION_1 is negotiated or not, and the feature bit VERSION_1 is superfluous? That would also imply, that 1) if revision > 0 was negotiated then the device must offer VERSION_1 2) if revision > 0 was negotiated and the driver cleared VERSION_1 the device must refuse to operate. 3) if revision > 0 was negotiated then the driver should reject to drive a device if it does not offer VERSION_1 4) if revision > 0 was negotiated the driver must accept VERSION_1 5) if revision > 0 was *not* negotiated then the device should not offer VERSION_1 because at this point it is already certain that the device can not act in accordance to the virtio 1.0 or higher interface. Does that sound about right? IMHO we should also change https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-160003 and the definition of VERSION_1 because both sides have to know what is going on before features are fully negotiated. Or? Regards, Halil ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Sat, Oct 02, 2021 at 02:40:55PM -0400, Michael S. Tsirkin wrote: > On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote: > > > > On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote: > > > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote: > > > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote: > > > > > >Do you have a list of specific drivers and kernel options that > > > > > > you > > > > > > feel you now "trust"? > > > > > For TDX it's currently only virtio net/block/console > > > > > > > > > > But we expect this list to grow slightly over time, but not at a high > > > > > rate > > > > > (so hopefully <10) > > > > Well there are already >10 virtio drivers and I think it's reasonable > > > > that all of these will be used with encrypted guests. The list will > > > > grow. > > > What is keeping "all" drivers from being on this list? > > > > It would be too much work to harden them all, and it would be pointless > > because all these drivers are never legitimately needed in a virtualized > > environment which only virtualize a very small number of devices. > > > > > How exactly are > > > you determining what should, and should not, be allowed? > > > > Everything that has had reasonable effort at hardening can be added. But if > > someone proposes to add a driver that should trigger additional scrutiny in > > code review. We should also request them to do some fuzzing. > > Looks like out of tree modules get a free pass then. That's not good. As we already know if a module is in or out of the tree, you all should be banning all out-of-tree modules if you care about these things. That should be very easy to do if you care. > > How would user space know what drivers have been hardened? This is really > > something that the kernel needs to determine. I don't think we can outsource > > it to anyone else. > > IIUC userspace is the distro. It can also do more than a binary on/off, > e.g. it can decide "only virtio", "no out of tree drivers". > A distro can also ship configs with a specific features > enabled/disabled. E.g. I can see where some GPU drivers will be > included by some distros since they are so useful, and excluded > by others since they are so big and hard to audit. > I don't see how the kernel can reasonably make a stand here. > Is "some audit and some fuzzing" a good policy? How much is enough? Agreed, that is why the policy for this should be in userspace. > Well if userspace sets the policy then I'm not sure we also want > a kernel one ... but if yes I'd like it to be in a central > place so whoever is building the kernel can tweak it easily > and rebuild, without poking at individual drivers. And here I thought the requirement was that no one could rebuild their kernel as it was provided by someone else. Again, these requirements seem contradicting, but as no one has actually pointed me at the real list of them, who knows what they are? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Sun, Oct 03, 2021 at 07:00:30AM +0200, Halil Pasic wrote:
> On Sat, 2 Oct 2021 14:20:47 -0400
> "Michael S. Tsirkin" wrote:
>
> > > >From my perspective the problem is that the version of the device
> > > remains in limbo as long as the features have not yet been finalized,
> > > which means that the endianness of the config space remains in limbo as
> > > well. Both device and driver might come to different conclusions.
> >
> > Version === legacy versus modern?
> > It is true that feature negotiation can not be used by device to decide that
> > question simply because it happens too late.
> > So let's not use it for that then ;)
> >
> > Yes we have VERSION_1 which looks like it should allow this, but
> > unfortunately it only helps with that for the driver, not the device.
> >
> > In practice legacy versus modern has to be determined by
> > transport specific versioning, luckily we have that for all
> > specified transports (can't say what happens with rproc).
>
> So if we look at ccw, you say that the revision negotiation already
> determines whether VERSION_1 is negotiated or not, and the
> feature bit VERSION_1 is superfluous?
>
> That would also imply, that
> 1) if revision > 0 was negotiated then the device must offer VERSION_1
> 2) if revision > 0 was negotiated and the driver cleared VERSION_1
>the device must refuse to operate.
> 3) if revision > 0 was negotiated then the driver should reject
>to drive a device if it does not offer VERSION_1
> 4) if revision > 0 was negotiated the driver must accept VERSION_1
> 5) if revision > 0 was *not* negotiated then the device should not offer
>VERSION_1 because at this point it is already certain that the device
>can not act in accordance to the virtio 1.0 or higher interface.
>
> Does that sound about right?
To me, it does.
> IMHO we should also change
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-160003
> and the definition of VERSION_1 because both sides have to know what is
> going on before features are fully negotiated. Or?
>
> Regards,
> Halil
>
I guess so. And I guess we need transport-specific sections
describing this behaviour for each transport.
So something like this, for starters?
diff --git a/content.tex b/content.tex
index 1398390..c526dd3 100644
--- a/content.tex
+++ b/content.tex
@@ -140,10 +140,13 @@ \subsection{Legacy Interface: A Note on Feature
Bits}\label{sec:Basic Facilities of a Virtio Device / Feature
Bits / Legacy Interface: A Note on Feature Bits}
-Transitional Drivers MUST detect Legacy Devices by detecting that
-the feature bit VIRTIO_F_VERSION_1 is not offered.
-Transitional devices MUST detect Legacy drivers by detecting that
-VIRTIO_F_VERSION_1 has not been acknowledged by the driver.
+Transitional drivers MAY support operating legacy devices.
+Transitional devices MAY support operation by legacy drivers.
+
+Transitional drivers MUST detect legacy devices in a way that is
+transport specific.
+Transitional devices MUST detect legacy drivers in a way that
+is transport specific.
In this case device is used through the legacy interface.
@@ -160,6 +163,25 @@ \subsection{Legacy Interface: A Note on Feature
Specification text within these sections generally does not apply
to non-transitional devices.
+\begin{note}
+The device offers different features when used through
+the legacy interface and when operated in accordance with this
+specification.
+\end{note}
+
+Transitional drivers MUST use Devices only through the legacy interface
+if the feature bit VIRTIO_F_VERSION_1 is not offered.
+Transitional devices MUST NOT offer VIRTIO_F_VERSION_1 when used through
+the legacy interface.
+
+When the driver uses a device through the legacy interface, then it
+MUST only accept the features the device offered through the
+legacy interface.
+
+When used through the legacy interface, the device SHOULD
+validate that the driver only accepted the features it
+offered through the legacy interface.
+
\section{Notifications}\label{sec:Basic Facilities of a Virtio Device
/ Notifications}
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
