On Fri, Jan 18, 2019 at 10:39:35AM +0100, Erik Skultety wrote: > Hi, > this is a summary of a private discussion I've had with guys CC'd on this email > about finding a solution to [1] - basically, the default permissions on > /dev/sev (below) make it impossible to query for SEV platform capabilities, > since by default we run QEMU as qemu:qemu when probing for capabilities. It's > worth noting is that this is only relevant to probing, since for a proper QEMU > VM we create a mount namespace for the process and chown all the nodes (needs a > SEV fix though). > > # ll /dev/sev > crw-------. 1 root root > > I suggested either force running QEMU as root for probing (despite the obvious > security implications) or using namespaces for probing too. Dan argued that > this would have a significant perf impact and suggested we ask systemd to add a > global udev rule. I've just realized there is a potential 3rd solution. Remember there is actually nothing inherantly special about the 'root' user as an account ID. 'root' gains its powers from the fact that it has many capabilities by default. 'qemu' can't access /dev/sev because it is owned by a different user (happens to be root) and 'qemu' does not have capabilities. So we can make probing work by using our capabilities code to grant CAP_DAC_OVERRIDE to the qemu process we spawn. So probing still runs as 'qemu', but can none the less access /dev/sev while it is owned by root. We were not using 'qemu' for sake of security, as the probing process is not executing any untrusthworthy code, so we don't loose any security protection by granting CAP_DAC_OVERRIDE.

> > So, in conclusion, we absolutely need input from Brijesh (AMD) whether there > was something more than the low limit on number of guests behind the default > permissions. Also, we'd like to get some details on how the limit is managed, > helping to assess the approaches mentioned above. Regardless of this problem, I think it is important to have some docs in either libvirt or QEMU that describe the resource usage constraints so that management apps can decide how to best take advantage of SEV.

On Fri, Jan 18, 2019 at 10:16:38AM +0000, Daniel P. Berrangé wrote: > I've just realized there is a potential 3rd solution. Remember there is > actually nothing inherantly special about the 'root' user as an account > ID. 'root' gains its powers from the fact that it has many capabilities > by default. 'qemu' can't access /dev/sev because it is owned by a > different user (happens to be root) and 'qemu' does not have capabilities. > > So we can make probing work by using our capabilities code to grant > CAP_DAC_OVERRIDE to the qemu process we spawn. So probing still runs > as 'qemu', but can none the less access /dev/sev while it is owned > by root. We were not using 'qemu' for sake of security, as the probing > process is not executing any untrusthworthy code, so we don't loose any > security protection by granting CAP_DAC_OVERRIDE. > IMHO CAP_DAC_OVERRIDE is a lot, especially on systems without SELinux.

Hi, this is a summary of a private discussion I've had with guys CC'd on this email about finding a solution to [1] - basically, the default permissions on /dev/sev (below) make it impossible to query for SEV platform capabilities, since by default we run QEMU as qemu:qemu when probing for capabilities. It's worth noting is that this is only relevant to probing, since for a proper QEMU VM we create a mount namespace for the process and chown all the nodes (needs a SEV fix though). # ll /dev/sev crw-------. 1 root root I suggested either force running QEMU as root for probing (despite the obvious security implications) or using namespaces for probing too. Dan argued that this would have a significant perf impact and suggested we ask systemd to add a global udev rule. I proceeded with cloning [1] to systemd and creating an udev rule that I planned on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and make it world accessible to which Brijesh from AMD expressed a concern that regular users might deplete the resources (limit on the number of guests allowed by the platform).

But since the limit is claimed to be around 4, Dan

discouraged me to continue with restricting the udev rule to only the 'kvm' group which Laszlo suggested earlier as the limit is so small that a malicious QEMU could easily deplete this during probing. This fact also ruled out any kind of ACL we could create dynamically. Instead, he suggested that we filter out the kvm-capable QEMU and put only that one in the namespace without a significant perf impact. - my take on this is that there could potentially be more than a single kvm-enabled QEMU and therefore we'd need to create more than just a single namespace. - I also argued that I can image that the same kind of DOS attack might be possible from within the namespace, even if we created the /dev/sev node only in SEV-enabled guests (which we currently don't). All of us have agreed that allowing /dev/sev in the namespace for only SEV-enabled guests is worth doing nonetheless. In the meantime, Christophe went through the kernel code to verify how the SEV resources are managed and what protection is currently in place to mitigate the chance of a process easily depleting the limit on SEV guests. He found that ASID, which determines the encryption key, is allocated from a single ASID bitmap and essentially guarded by a single 'sev->active' flag. So, in conclusion, we absolutely need input from Brijesh (AMD) whether there was something more than the low limit on number of guests behind the default permissions. Also, we'd like to get some details on how the limit is managed, helping to assess the approaches mentioned above.

Thanks and please do share your ideas, Erik [1] https://bugzilla.redhat.com/show_bug.cgi?id=1665400 [2] https://bugzilla.redhat.com/show_bug.cgi?id=1561113

On Fri, Jan 18, 2019 at 12:51:50PM +0000, Singh, Brijesh wrote: > > On 1/18/19 3:39 AM, Erik Skultety wrote: > > Hi, > > this is a summary of a private discussion I've had with guys CC'd on this email > > about finding a solution to [1] - basically, the default permissions on > > /dev/sev (below) make it impossible to query for SEV platform capabilities, > > since by default we run QEMU as qemu:qemu when probing for capabilities. It's > > worth noting is that this is only relevant to probing, since for a proper QEMU > > VM we create a mount namespace for the process and chown all the nodes (needs a > > SEV fix though). > > > > # ll /dev/sev > > crw-------. 1 root root > > > > I suggested either force running QEMU as root for probing (despite the obvious > > security implications) or using namespaces for probing too. Dan argued that > > this would have a significant perf impact and suggested we ask systemd to add a > > global udev rule. > > > > I proceeded with cloning [1] to systemd and creating an udev rule that I planned > > on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and > > make it world accessible to which Brijesh from AMD expressed a concern that > > regular users might deplete the resources (limit on the number of guests > > allowed by the platform). > > > During private discussion I didn't realized that we are discussing a > probe issue hence things I have said earlier may not be applicable > during the probe. The /dev/sev is managed by the CCP (aka PSP) driver. > The /dev/sev is used for communicating with the SEV FW running inside > the PSP. The SEV FW offers platform and guest specific services. The > guest specific services are used during the guest launch, these services > are available through KVM driver only. Whereas the platform services can > be invoked at anytime. A typical platform specific services are: > > - importing certificates > > - exporting certificates > > - querying the SEV FW version etc etc > > In case of the probe we are not launch SEV guest hence we should not be > worried about depleting the SEV ASID resources. > > IIRC, libvirt uses QEMP query-sev-capabilities to probe the SEV support. > QEMU executes the below sequence to complete the request: > > 1. Exports the platform certificates  (this is when /dev/sev is accessed). > > 2. Read the host MSR to determine the C-bit and reduced phys-bit position > > I don't see any reason why we can't give world a 'read' permission to > /dev/sev. Anyone should be able to export the certificates and query Okay, makes sense to me. The problem I see is the sev_platform_ioctl function in QEMU which makes an _IOWR request, therefore the file descriptor being opened in sev_get_capabilities is O_RDWR. Now, I only understand ioctl from what I've read in the man page, so I don't quite understand the need for IOWR here - but my honest guess would be that it's because the commands like SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS need to be copied from userspace to kernel to instruct kernel which services we want, ergo _IOWR, is that right?

In any case, a fix of some sort needs to land in QEMU first, because no udev rule would fix the current situation. Afterwards, I expect that having a rule like this: KERNEL=="sev", GROUP="kvm", MODE="0644" and a selinux policy rule adding the kvm_device_t label, we should be fine, do we agree on that?

On Wed, Jan 23, 2019 at 01:10:42PM +0000, Daniel P. Berrangé wrote: > On Wed, Jan 23, 2019 at 01:55:06PM +0100, Erik Skultety wrote: > > On Fri, Jan 18, 2019 at 12:51:50PM +0000, Singh, Brijesh wrote: > > > > > > On 1/18/19 3:39 AM, Erik Skultety wrote: > > > > Hi, > > > > this is a summary of a private discussion I've had with guys CC'd on this email > > > > about finding a solution to [1] - basically, the default permissions on > > > > /dev/sev (below) make it impossible to query for SEV platform capabilities, > > > > since by default we run QEMU as qemu:qemu when probing for capabilities. It's > > > > worth noting is that this is only relevant to probing, since for a proper QEMU > > > > VM we create a mount namespace for the process and chown all the nodes (needs a > > > > SEV fix though). > > > > > > > > # ll /dev/sev > > > > crw-------. 1 root root > > > > > > > > I suggested either force running QEMU as root for probing (despite the obvious > > > > security implications) or using namespaces for probing too. Dan argued that > > > > this would have a significant perf impact and suggested we ask systemd to add a > > > > global udev rule. > > > > > > > > I proceeded with cloning [1] to systemd and creating an udev rule that I planned > > > > on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and > > > > make it world accessible to which Brijesh from AMD expressed a concern that > > > > regular users might deplete the resources (limit on the number of guests > > > > allowed by the platform). > > > > > > > > > During private discussion I didn't realized that we are discussing a > > > probe issue hence things I have said earlier may not be applicable > > > during the probe. The /dev/sev is managed by the CCP (aka PSP) driver. > > > The /dev/sev is used for communicating with the SEV FW running inside > > > the PSP. The SEV FW offers platform and guest specific services. The > > > guest specific services are used during the guest launch, these services > > > are available through KVM driver only. Whereas the platform services can > > > be invoked at anytime. A typical platform specific services are: > > > > > > - importing certificates > > > > > > - exporting certificates > > > > > > - querying the SEV FW version etc etc > > > > > > In case of the probe we are not launch SEV guest hence we should not be > > > worried about depleting the SEV ASID resources. > > > > > > IIRC, libvirt uses QEMP query-sev-capabilities to probe the SEV support. > > > QEMU executes the below sequence to complete the request: > > > > > > 1. Exports the platform certificates  (this is when /dev/sev is accessed). > > > > > > 2. Read the host MSR to determine the C-bit and reduced phys-bit position > > > > > > I don't see any reason why we can't give world a 'read' permission to > > > /dev/sev. Anyone should be able to export the certificates and query > > > > Okay, makes sense to me. The problem I see is the sev_platform_ioctl function > > in QEMU which makes an _IOWR request, therefore the file descriptor being > > opened in sev_get_capabilities is O_RDWR. Now, I only understand ioctl from > > what I've read in the man page, so I don't quite understand the need for IOWR > > here - but my honest guess would be that it's because the commands like > > SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS need to be copied from userspace to > > kernel to instruct kernel which services we want, ergo _IOWR, is that right? > > I'm not seeing any permissions checks in the sev_ioctl() function in the > kernel, so IIUC, that means any permissions are entirely based on whether > you can open the /dev/sev, once open you can run any ioctl. What, if anything, > enforces which ioctls you can run when the device is only O_RDONLY vs O_RDWR ? I don't know, that's why I'm asking, because the manual didn't make it any clear for me whether there's a connection between the device permissions and ioctls that you're allowed to run. > > > In any case, a fix of some sort needs to land in QEMU first, because no udev > > rule would fix the current situation. Afterwards, I expect that having a rule > > like this: > > > > KERNEL=="sev", GROUP="kvm", MODE="0644" > > > > and a selinux policy rule adding the kvm_device_t label, we should be fine, do > > we agree on that? > > Based on what I think I see above, this looks like a bad idea. > > It still looks like we can solve this entirely in libvirt by just giving > the libvirt capabilities probing code CAP_DAC_OVERRIDE. This would make > libvirt work for all currently released SEV support in kernel/qemu. Sure we can, but that would make libvirt the only legitimate user of /dev/sev and everything else would require the admin to change the permissions explicitly so that other apps could at least retrieve the platform info, if it was intended to be for public use? Additionally, we'll still get shot down by SELinux because svirt_t wouldn't be able to access /dev/sev by default.

On Wed, Jan 23, 2019 at 01:24:13PM +0000, Daniel P. Berrangé wrote: > On Wed, Jan 23, 2019 at 02:22:12PM +0100, Erik Skultety wrote: > > On Wed, Jan 23, 2019 at 01:10:42PM +0000, Daniel P. Berrangé wrote: > > > On Wed, Jan 23, 2019 at 01:55:06PM +0100, Erik Skultety wrote: > > > > On Fri, Jan 18, 2019 at 12:51:50PM +0000, Singh, Brijesh wrote: > > > > > > > > > > On 1/18/19 3:39 AM, Erik Skultety wrote: > > > > > > Hi, > > > > > > this is a summary of a private discussion I've had with guys CC'd on this email > > > > > > about finding a solution to [1] - basically, the default permissions on > > > > > > /dev/sev (below) make it impossible to query for SEV platform capabilities, > > > > > > since by default we run QEMU as qemu:qemu when probing for capabilities. It's > > > > > > worth noting is that this is only relevant to probing, since for a proper QEMU > > > > > > VM we create a mount namespace for the process and chown all the nodes (needs a > > > > > > SEV fix though). > > > > > > > > > > > > # ll /dev/sev > > > > > > crw-------. 1 root root > > > > > > > > > > > > I suggested either force running QEMU as root for probing (despite the obvious > > > > > > security implications) or using namespaces for probing too. Dan argued that > > > > > > this would have a significant perf impact and suggested we ask systemd to add a > > > > > > global udev rule. > > > > > > > > > > > > I proceeded with cloning [1] to systemd and creating an udev rule that I planned > > > > > > on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and > > > > > > make it world accessible to which Brijesh from AMD expressed a concern that > > > > > > regular users might deplete the resources (limit on the number of guests > > > > > > allowed by the platform). > > > > > > > > > > > > > > > During private discussion I didn't realized that we are discussing a > > > > > probe issue hence things I have said earlier may not be applicable > > > > > during the probe. The /dev/sev is managed by the CCP (aka PSP) driver. > > > > > The /dev/sev is used for communicating with the SEV FW running inside > > > > > the PSP. The SEV FW offers platform and guest specific services. The > > > > > guest specific services are used during the guest launch, these services > > > > > are available through KVM driver only. Whereas the platform services can > > > > > be invoked at anytime. A typical platform specific services are: > > > > > > > > > > - importing certificates > > > > > > > > > > - exporting certificates > > > > > > > > > > - querying the SEV FW version etc etc > > > > > > > > > > In case of the probe we are not launch SEV guest hence we should not be > > > > > worried about depleting the SEV ASID resources. > > > > > > > > > > IIRC, libvirt uses QEMP query-sev-capabilities to probe the SEV support. > > > > > QEMU executes the below sequence to complete the request: > > > > > > > > > > 1. Exports the platform certificates  (this is when /dev/sev is accessed). > > > > > > > > > > 2. Read the host MSR to determine the C-bit and reduced phys-bit position > > > > > > > > > > I don't see any reason why we can't give world a 'read' permission to > > > > > /dev/sev. Anyone should be able to export the certificates and query > > > > > > > > Okay, makes sense to me. The problem I see is the sev_platform_ioctl function > > > > in QEMU which makes an _IOWR request, therefore the file descriptor being > > > > opened in sev_get_capabilities is O_RDWR. Now, I only understand ioctl from > > > > what I've read in the man page, so I don't quite understand the need for IOWR > > > > here - but my honest guess would be that it's because the commands like > > > > SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS need to be copied from userspace to > > > > kernel to instruct kernel which services we want, ergo _IOWR, is that right? > > > > > > I'm not seeing any permissions checks in the sev_ioctl() function in the > > > kernel, so IIUC, that means any permissions are entirely based on whether > > > you can open the /dev/sev, once open you can run any ioctl. What, if anything, > > > enforces which ioctls you can run when the device is only O_RDONLY vs O_RDWR ? > > > > I don't know, that's why I'm asking, because the manual didn't make it any > > clear for me whether there's a connection between the device permissions and > > ioctls that you're allowed to run. > > > > > > > > > In any case, a fix of some sort needs to land in QEMU first, because no udev > > > > rule would fix the current situation. Afterwards, I expect that having a rule > > > > like this: > > > > > > > > KERNEL=="sev", GROUP="kvm", MODE="0644" > > > > > > > > and a selinux policy rule adding the kvm_device_t label, we should be fine, do > > > > we agree on that? > > > > > > Based on what I think I see above, this looks like a bad idea. > > > > > > It still looks like we can solve this entirely in libvirt by just giving > > > the libvirt capabilities probing code CAP_DAC_OVERRIDE. This would make > > > libvirt work for all currently released SEV support in kernel/qemu. > > > > Sure we can, but that would make libvirt the only legitimate user of /dev/sev > > and everything else would require the admin to change the permissions > > explicitly so that other apps could at least retrieve the platform info, if > > it was intended to be for public use? > > Additionally, we'll still get shot down by SELinux because svirt_t wouldn't be > > able to access /dev/sev by default. > > That's separate form probing and just needs SELinux policy to define > a new sev_device_t type and grant svirt_t access to it. I know, I misread "we can solve this entirely in libvirt" then, I thought you the SELinux part was included in the statement, my bad then. Still, back to the original issue, we could technically do both, libvirt would have run qemu with CAP_DAC_OVERRIDE and we keep working with everything's been released for SEV in kernel/qemu and for everyone else, systemd might add 0644 for /dev/sev, that way, everyone's happy, not that I'd be a fan of libvirt often having to work around something because projects underneath wouldn't backport fixes to all the distros we support, thus leaving the dirty work to us.

On 1/23/19 7:36 AM, Daniel P. Berrangé wrote: > On Wed, Jan 23, 2019 at 02:33:01PM +0100, Erik Skultety wrote: >> On Wed, Jan 23, 2019 at 01:24:13PM +0000, Daniel P. Berrangé wrote: >>> On Wed, Jan 23, 2019 at 02:22:12PM +0100, Erik Skultety wrote: >>>> On Wed, Jan 23, 2019 at 01:10:42PM +0000, Daniel P. Berrangé wrote: >>>>> On Wed, Jan 23, 2019 at 01:55:06PM +0100, Erik Skultety wrote: >>>>>> On Fri, Jan 18, 2019 at 12:51:50PM +0000, Singh, Brijesh wrote: >>>>>>> >>>>>>> On 1/18/19 3:39 AM, Erik Skultety wrote: >>>>>>>> Hi, >>>>>>>> this is a summary of a private discussion I've had with guys CC'd on this email >>>>>>>> about finding a solution to [1] - basically, the default permissions on >>>>>>>> /dev/sev (below) make it impossible to query for SEV platform capabilities, >>>>>>>> since by default we run QEMU as qemu:qemu when probing for capabilities. It's >>>>>>>> worth noting is that this is only relevant to probing, since for a proper QEMU >>>>>>>> VM we create a mount namespace for the process and chown all the nodes (needs a >>>>>>>> SEV fix though). >>>>>>>> >>>>>>>> # ll /dev/sev >>>>>>>> crw-------. 1 root root >>>>>>>> >>>>>>>> I suggested either force running QEMU as root for probing (despite the obvious >>>>>>>> security implications) or using namespaces for probing too. Dan argued that >>>>>>>> this would have a significant perf impact and suggested we ask systemd to add a >>>>>>>> global udev rule. >>>>>>>> >>>>>>>> I proceeded with cloning [1] to systemd and creating an udev rule that I planned >>>>>>>> on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and >>>>>>>> make it world accessible to which Brijesh from AMD expressed a concern that >>>>>>>> regular users might deplete the resources (limit on the number of guests >>>>>>>> allowed by the platform). >>>>>>> >>>>>>> >>>>>>> During private discussion I didn't realized that we are discussing a >>>>>>> probe issue hence things I have said earlier may not be applicable >>>>>>> during the probe. The /dev/sev is managed by the CCP (aka PSP) driver. >>>>>>> The /dev/sev is used for communicating with the SEV FW running inside >>>>>>> the PSP. The SEV FW offers platform and guest specific services. The >>>>>>> guest specific services are used during the guest launch, these services >>>>>>> are available through KVM driver only. Whereas the platform services can >>>>>>> be invoked at anytime. A typical platform specific services are: >>>>>>> >>>>>>> - importing certificates >>>>>>> >>>>>>> - exporting certificates >>>>>>> >>>>>>> - querying the SEV FW version etc etc >>>>>>> >>>>>>> In case of the probe we are not launch SEV guest hence we should not be >>>>>>> worried about depleting the SEV ASID resources. >>>>>>> >>>>>>> IIRC, libvirt uses QEMP query-sev-capabilities to probe the SEV support. >>>>>>> QEMU executes the below sequence to complete the request: >>>>>>> >>>>>>> 1. Exports the platform certificates  (this is when /dev/sev is accessed). >>>>>>> >>>>>>> 2. Read the host MSR to determine the C-bit and reduced phys-bit position >>>>>>> >>>>>>> I don't see any reason why we can't give world a 'read' permission to >>>>>>> /dev/sev. Anyone should be able to export the certificates and query >>>>>> >>>>>> Okay, makes sense to me. The problem I see is the sev_platform_ioctl function >>>>>> in QEMU which makes an _IOWR request, therefore the file descriptor being >>>>>> opened in sev_get_capabilities is O_RDWR. Now, I only understand ioctl from >>>>>> what I've read in the man page, so I don't quite understand the need for IOWR >>>>>> here - but my honest guess would be that it's because the commands like >>>>>> SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS need to be copied from userspace to >>>>>> kernel to instruct kernel which services we want, ergo _IOWR, is that right? >>>>> >>>>> I'm not seeing any permissions checks in the sev_ioctl() function in the >>>>> kernel, so IIUC, that means any permissions are entirely based on whether >>>>> you can open the /dev/sev, once open you can run any ioctl. What, if anything, >>>>> enforces which ioctls you can run when the device is only O_RDONLY vs O_RDWR ? >>>> >>>> I don't know, that's why I'm asking, because the manual didn't make it any >>>> clear for me whether there's a connection between the device permissions and >>>> ioctls that you're allowed to run. >>>> >>>>> >>>>>> In any case, a fix of some sort needs to land in QEMU first, because no udev >>>>>> rule would fix the current situation. Afterwards, I expect that having a rule >>>>>> like this: >>>>>> >>>>>> KERNEL=="sev", GROUP="kvm", MODE="0644" >>>>>> >>>>>> and a selinux policy rule adding the kvm_device_t label, we should be fine, do >>>>>> we agree on that? >>>>> >>>>> Based on what I think I see above, this looks like a bad idea. >>>>> >>>>> It still looks like we can solve this entirely in libvirt by just giving >>>>> the libvirt capabilities probing code CAP_DAC_OVERRIDE. This would make >>>>> libvirt work for all currently released SEV support in kernel/qemu. >>>> >>>> Sure we can, but that would make libvirt the only legitimate user of /dev/sev >>>> and everything else would require the admin to change the permissions >>>> explicitly so that other apps could at least retrieve the platform info, if >>>> it was intended to be for public use? >>>> Additionally, we'll still get shot down by SELinux because svirt_t wouldn't be >>>> able to access /dev/sev by default. >>> >>> That's separate form probing and just needs SELinux policy to define >>> a new sev_device_t type and grant svirt_t access to it. >> >> I know, I misread "we can solve this entirely in libvirt" then, I thought you >> the SELinux part was included in the statement, my bad then. Still, back to the >> original issue, we could technically do both, libvirt would have run qemu with >> CAP_DAC_OVERRIDE and we keep working with everything's been released for >> SEV in kernel/qemu and for everyone else, systemd might add 0644 for /dev/sev, >> that way, everyone's happy, not that I'd be a fan of libvirt often having >> to work around something because projects underneath wouldn't backport fixes to >> all the distros we support, thus leaving the dirty work to us. > > Setting 0644 for /dev/sev looks unsafe to me unless someone can show > where the permissions checks take place for the many ioctls that > /dev/sev allows, such that only SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS > is allowed when /dev/sev is opened by a user who doesn't have write > permissions. > Agree its not safe to do 0644. Currently, anyone who has access to /dev/sev (read or write) will be able to execute SEV platform command. In other words there is no permission check per command basis. I must admit that while developing the driver I was under assumption that only root will ever access the /dev/sev device hence overlooked it. But now knowing that others may also need to access the /dev/sev, I can submit patch in kernel to do per command access control. Until then, can we follow Daniel's recommendation to elevate privilege of the probing code?

Thanks for ^this bit which helped me understand the bits below. When I read the man page yesterday the first question was, okay, how do I figure out whether the file capabilities bit is set? Well, use xattrs...which didn't return anything, so I was puzzled what exactly it should look like, but now that you explained that most binaries actually lack the file capabilities, I see the issue clearly :).

> + > ret = 0; > cleanup: > return ret; > > > though, we need a #ifdef check for existance of PR_CAP_AMBIENT > > > An alternative question I've been playing ever since we exchanged the last few > > emails is that can't we wait until the ioctls are compared against permissions > > in kernel so that upstream libvirt (and downstream too for that matter) doesn't > > have to work around it and stick with that workaround for eternity? > > IIUC, the SEV feature has already shipped with distros, so we'd effectively > be saying that what we already shipped is unusable to libvirt. This doesn't > feel like a desirable story to me. It was, but it never worked, it always has been broken in this way. When we were merging this upstream, we had a terrible shortage of machines and we had to share, so the first person to provision the machine had already taken care of the permissions in order to test so that led to this issue having been overlooked until now. If it ever worked as expected and then we broke it, then any fix from our side would make sense but otherwise I believe we should fix this bottom up.

>>> though, we need a #ifdef check for existance of PR_CAP_AMBIENT >>> >>>> An alternative question I've been playing ever since we exchanged the last few >>>> emails is that can't we wait until the ioctls are compared against permissions >>>> in kernel so that upstream libvirt (and downstream too for that matter) doesn't >>>> have to work around it and stick with that workaround for eternity? >>> >>> IIUC, the SEV feature has already shipped with distros, so we'd effectively >>> be saying that what we already shipped is unusable to libvirt. This doesn't >>> feel like a desirable story to me. >> >> It was, but it never worked, it always has been broken in this way. When we >> were merging this upstream, we had a terrible shortage of machines and we had >> to share, so the first person to provision the machine had already taken care >> of the permissions in order to test so that led to this issue having been >> overlooked until now. If it ever worked as expected and then we broke it, then >> any fix from our side would make sense but otherwise I believe we should fix >> this bottom up. > > Well technically it would work if libvirt was configured to run as > root:root, but yes, that is not a normal or recommended configuration. > > Personally I have a preference for userspace solutions, as those are > pretty straightforward to roll out to people as patches in existing > releases. Deploying kernel updates is a higher bar to cross for an > existing release. So, can you compile the prctl stuff in kernel conditionally? If so, then that's a problem because you may end up with a platform where SEV is supported within kernel, but you don't have the ambient stuff we have to conditionally compile in libvirt, so you end up with broken SEV support anyway, I wanted to argue with centos 7, but the ambient set support was backported to 3.10, so the only distro where we'd have a problem from userspace POV would be debian 8, but then again the kernel there is so old that neither SEV is supported there.

I understand your point, but it also sounds very agile and I don't think that compensating with "something that is fast" for "something that is right" is the way to go in the long term. Especially since we almost never deprecate stuff and we can't break compatibility. Trying to work around every issue coming from your dependencies in your project is highly unsustainable.

On Wed, Jan 30, 2019 at 02:39:54PM +0100, Erik Skultety wrote: > > > > though, we need a #ifdef check for existance of PR_CAP_AMBIENT > > > > > > > > > An alternative question I've been playing ever since we exchanged the last few > > > > > emails is that can't we wait until the ioctls are compared against permissions > > > > > in kernel so that upstream libvirt (and downstream too for that matter) doesn't > > > > > have to work around it and stick with that workaround for eternity? > > > > > > > > IIUC, the SEV feature has already shipped with distros, so we'd effectively > > > > be saying that what we already shipped is unusable to libvirt. This doesn't > > > > feel like a desirable story to me. > > > > > > It was, but it never worked, it always has been broken in this way. When we > > > were merging this upstream, we had a terrible shortage of machines and we had > > > to share, so the first person to provision the machine had already taken care > > > of the permissions in order to test so that led to this issue having been > > > overlooked until now. If it ever worked as expected and then we broke it, then > > > any fix from our side would make sense but otherwise I believe we should fix > > > this bottom up. > > > > Well technically it would work if libvirt was configured to run as > > root:root, but yes, that is not a normal or recommended configuration. > > > > Personally I have a preference for userspace solutions, as those are > > pretty straightforward to roll out to people as patches in existing > > releases. Deploying kernel updates is a higher bar to cross for an > > existing release. > > So, can you compile the prctl stuff in kernel conditionally? If so, then that's > a problem because you may end up with a platform where SEV is supported within > kernel, but you don't have the ambient stuff we have to conditionally compile > in libvirt, so you end up with broken SEV support anyway, I wanted to argue > with centos 7, but the ambient set support was backported to 3.10, so the only > distro where we'd have a problem from userspace POV would be debian 8, but then > again the kernel there is so old that neither SEV is supported there. > > I understand your point, but it also sounds very agile and I don't think that > compensating with "something that is fast" for "something that is right" is the > way to go in the long term. Especially since we almost never deprecate stuff > and we can't break compatibility. Trying to work around every issue coming > from your dependencies in your project is highly unsustainable. With the launching of VMs we've got to a place where libvirt is pretty robust about being able to grant access regardless of what the host OS has done for permissions in /dev. I think its desirable that this same flexibility extends to capabilities probing, which is somethign the dac_override approach allows. IOW, even if the kernel changes /dev/sev as previously discussed, I would keep the dac_override stuff for probing capabilities forever. This makes sure we'll work even if the distro in question has strictly locked down permissions on /dev/kvm or /dev/sev, diverging from the default udev settup

On Fri, Jan 18, 2019 at 12:51:50PM +0000, Singh, Brijesh wrote: > > On 1/18/19 3:39 AM, Erik Skultety wrote: >> I proceeded with cloning [1] to systemd and creating an udev rule that I planned >> on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and >> make it world accessible to which Brijesh from AMD expressed a concern that >> regular users might deplete the resources (limit on the number of guests >> allowed by the platform). [snip] >> But since the limit is claimed to be around 4, Dan > > > FYI, the limit on EPYC is 15. Do any cRyzen CPUs support SEV, and if so is their limit also 15 ?

Regardless, I'm assuming this limit is liable to change at any time in future CPU generations, so from the the mgmt app perspective I think is is important that QEMU / libvirt can both report what this limit is.

For QEMU I think query-sev-capabilities probably should report the guest limit. I guess QEMU would in turn want to ask the kernel, rather than hardcode info itself. So if this info isn't already exposed by the kernel we might need work there too.

For libvirt we can then put this in the domain capabilities where we report SEV support. > This will enable OpenStack and similar apps to plan which host they place a new VM on, to ensure there is SEV resource available for it to use. Regards, Daniel