Greg Kroah-Hartman
2021-Oct-01 07:03 UTC
[PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:> > > On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote: > > On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote: > > > > > > > > > On 9/30/21 6:36 AM, Dan Williams wrote: > > > > > And in particular, not all virtio drivers are hardened - > > > > > I think at this point blk and scsi drivers have been hardened - so > > > > > treating them all the same looks wrong. > > > > My understanding was that they have been audited, Sathya? > > > > > > Yes, AFAIK, it has been audited. Andi also submitted some patches > > > related to it. Andi, can you confirm. > > > > What is the official definition of "audited"? > > > In our case (Confidential Computing platform), the host is an un-trusted > entity. So any interaction with host from the drivers will have to be > protected against the possible attack from the host. For example, if we > are accessing a memory based on index value received from host, we have > to make sure it does not lead to out of bound access or when sharing the > memory with the host, we need to make sure only the required region is > shared with the host and the memory is un-shared after use properly.You have not defined the term "audited" here at all in any way that can be reviewed or verified by anyone from what I can tell. You have only described a new model that you wish the kernel to run in, one in which it does not trust the hardware at all. That is explicitly NOT what the kernel has been designed for so far, and if you wish to change that, lots of things need to be done outside of simply running some fuzzers on a few random drivers. For one example, how do you ensure that the memory you are reading from hasn't been modified by the host between writing to it the last time you did? Do you have a list of specific drivers and kernel options that you feel you now "trust"? If so, how long does that trust last for? Until someonen else modifies that code? What about modifications to functions that your "audited" code touches? Who is doing this auditing? How do you know the auditing has been done correctly? Who has reviewed and audited the tools that are doing the auditing? Where is the specification that has been agreed on how the auditing must be done? And so on... I feel like there are a lot of different things all being mixed up here into one "oh we want this to happen!" type of thread. Please let's just stick to the one request that I had here, which was to move the way that busses are allowed to authorize the devices they wish to control into a generic way instead of being bus-specific logic. Any requests outside of that type of functionality are just that, outside the scope of this patchset and should get their own patch series and discussion. thanks, greg k-h
Andi Kleen
2021-Oct-01 15:49 UTC
[PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On 10/1/2021 12:03 AM, Greg Kroah-Hartman wrote:> On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote: >> >> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote: >>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote: >>>> >>>> On 9/30/21 6:36 AM, Dan Williams wrote: >>>>>> And in particular, not all virtio drivers are hardened - >>>>>> I think at this point blk and scsi drivers have been hardened - so >>>>>> treating them all the same looks wrong. >>>>> My understanding was that they have been audited, Sathya? >>>> Yes, AFAIK, it has been audited. Andi also submitted some patches >>>> related to it. Andi, can you confirm. >>> What is the official definition of "audited"? >> >> In our case (Confidential Computing platform), the host is an un-trusted >> entity. So any interaction with host from the drivers will have to be >> protected against the possible attack from the host. For example, if we >> are accessing a memory based on index value received from host, we have >> to make sure it does not lead to out of bound access or when sharing the >> memory with the host, we need to make sure only the required region is >> shared with the host and the memory is un-shared after use properly. > You have not defined the term "audited" here at all in any way that can > be reviewed or verified by anyone from what I can tell. > > You have only described a new model that you wish the kernel to run in, > one in which it does not trust the hardware at all. That is explicitly > NOT what the kernel has been designed for so far,It has been already done for a few USB/TB drivers, but yes not for the majority of the kernel.> and if you wish to > change that, lots of things need to be done outside of simply running > some fuzzers on a few random drivers.The goal is to do similar work as USB/TB did, but do it for a small set of virtio drivers and use a custom allow list for those for the specific secure guest cases. (there are some other goals, but let's not discuss them here for now)> > For one example, how do you ensure that the memory you are reading from > hasn't been modified by the host between writing to it the last time you > did?It's similar techniques as we do on user space accesses. For example if you bound check some value the code needs to ensure it is cached in private memory, not reread from MMIO or shared memory. Of course that's a good idea anyways for performance because MMIO is slow. In the concrete cases of virtio the main problem was the free list in shared memory, but that has been addressed now.> 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)> If so, how long does that trust last for? Until > someonen else modifies that code? What about modifications to functions > that your "audited" code touches? Who is doing this auditing? How do > you know the auditing has been done correctly? Who has reviewed and > audited the tools that are doing the auditing? Where is the > specification that has been agreed on how the auditing must be done? > And so on...Well, I mean we already have a similar situation with user space APIs. So it's not a new problem. For those we've done it for many years, with audits and extra fuzzing. There are people working on the audit and fuzzing today. How exactly it will be ensured long term is still be worked out, but I expect we can work out something.> > I feel like there are a lot of different things all being mixed up here > into one "oh we want this to happen!" type of thread.Agreed. The thread ended up about a lot of stuff which is outside the scope of the patches.> Please let's just > stick to the one request that I had here, which was to move the way that > busses are allowed to authorize the devices they wish to control into a > generic way instead of being bus-specific logic. > > Any requests outside of that type of functionality are just that, > outside the scope of this patchset and should get their own patch series > and discussion.Yes that's the intention. This patch kit is only about controlling what devices can enumerate. Also please let's avoid the "trusted" term. It's really misleading and confusing in the context of confidential computing. -Andi
Dan Williams
2021-Oct-01 16:13 UTC
[PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
On Fri, Oct 1, 2021 at 12:03 AM Greg Kroah-Hartman <gregkh at linuxfoundation.org> wrote:> > On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote: > > > > > > On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote: > > > On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote: > > > > > > > > > > > > On 9/30/21 6:36 AM, Dan Williams wrote: > > > > > > And in particular, not all virtio drivers are hardened - > > > > > > I think at this point blk and scsi drivers have been hardened - so > > > > > > treating them all the same looks wrong. > > > > > My understanding was that they have been audited, Sathya? > > > > > > > > Yes, AFAIK, it has been audited. Andi also submitted some patches > > > > related to it. Andi, can you confirm. > > > > > > What is the official definition of "audited"? > > > > > > In our case (Confidential Computing platform), the host is an un-trusted > > entity. So any interaction with host from the drivers will have to be > > protected against the possible attack from the host. For example, if we > > are accessing a memory based on index value received from host, we have > > to make sure it does not lead to out of bound access or when sharing the > > memory with the host, we need to make sure only the required region is > > shared with the host and the memory is un-shared after use properly. > > You have not defined the term "audited" here at all in any way that can > be reviewed or verified by anyone from what I can tell.Agree.> > You have only described a new model that you wish the kernel to run in, > one in which it does not trust the hardware at all. That is explicitly > NOT what the kernel has been designed for so far, and if you wish to > change that, lots of things need to be done outside of simply running > some fuzzers on a few random drivers. > > For one example, how do you ensure that the memory you are reading from > hasn't been modified by the host between writing to it the last time you > did? Do you have a list of specific drivers and kernel options that you > feel you now "trust"? If so, how long does that trust last for? Until > someonen else modifies that code? What about modifications to functions > that your "audited" code touches? Who is doing this auditing? How do > you know the auditing has been done correctly? Who has reviewed and > audited the tools that are doing the auditing? Where is the > specification that has been agreed on how the auditing must be done? > And so on... > > I feel like there are a lot of different things all being mixed up here > into one "oh we want this to happen!" type of thread. Please let's just > stick to the one request that I had here, which was to move the way that > busses are allowed to authorize the devices they wish to control into a > generic way instead of being bus-specific logic.I want to continue to shave this proposal down, but that last sentence reads as self-contradictory. Bear with me, and perhaps it's a lack of imagination on my part, but I don't see how to get to a globally generic "authorized" sysfs ABI given that USB and Thunderbolt want to do bus specific actions on authorization toggle events. Certainly a default generic authorized attribute can be defined for all the other buses that don't have legacy here, but Thunderbolt will still require support for '2' as an authorized value, and USB will still want to base probe decisions on the authorization state of both the usb_device and the usb_interface.> Any requests outside of that type of functionality are just that, > outside the scope of this patchset and should get their own patch series > and discussion.A way to shave this further would be to default authorize just enough devices to do the rest of the authorization from initramfs. That would seem to cut out 'virtio-net' and 'virtio-storage' from default authorized list. The generic authorized attribute would only need to appear when 'dev_default_authorization' is set to false.