kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
@ 2021-05-06  9:18 Maxime Coquelin
  2021-05-06 21:50 ` Alex Williamson
  2021-05-11  2:58 ` Kees Cook
  0 siblings, 2 replies; 7+ messages in thread
From: Maxime Coquelin @ 2021-05-06  9:18 UTC (permalink / raw)
  To: alex.williamson, jmorris, dhowells, linux-kernel,
	linux-security-module, kvm
  Cc: mjg59, keescook, cohuck, Maxime Coquelin

When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
the PCI BARs via the device's sysfs, which is locked down when
the kernel is locked down.

Indeed, it is possible for an attacker to craft DMA requests
to modify kernel's code or leak secrets stored in the kernel,
since the device is not isolated by an IOMMU.

This patch introduces a new integrity lockdown reason for the
unsafe VFIO no-iommu mode.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/vfio/vfio.c      | 13 +++++++++----
 include/linux/security.h |  1 +
 security/security.c      |  1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 5e631c359ef2..fe466d6ea5d8 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -25,6 +25,7 @@
 #include <linux/pci.h>
 #include <linux/rwsem.h>
 #include <linux/sched.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
 #include <linux/string.h>
@@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
 {
 	if (arg != VFIO_NOIOMMU_IOMMU)
 		return ERR_PTR(-EINVAL);
-	if (!capable(CAP_SYS_RAWIO))
+	if (!capable(CAP_SYS_RAWIO) ||
+			security_locked_down(LOCKDOWN_VFIO_NOIOMMU))
 		return ERR_PTR(-EPERM);
 
 	return NULL;
@@ -1280,7 +1282,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 	if (atomic_read(&group->container_users))
 		return -EINVAL;
 
-	if (group->noiommu && !capable(CAP_SYS_RAWIO))
+	if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
+			security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
 		return -EPERM;
 
 	f = fdget(container_fd);
@@ -1362,7 +1365,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	    !group->container->iommu_driver || !vfio_group_viable(group))
 		return -EINVAL;
 
-	if (group->noiommu && !capable(CAP_SYS_RAWIO))
+	if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
+			security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
 		return -EPERM;
 
 	device = vfio_device_get_from_name(group, buf);
@@ -1490,7 +1494,8 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 	if (!group)
 		return -ENODEV;
 
-	if (group->noiommu && !capable(CAP_SYS_RAWIO)) {
+	if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
+			security_locked_down(LOCKDOWN_VFIO_NOIOMMU))) {
 		vfio_group_put(group);
 		return -EPERM;
 	}
diff --git a/include/linux/security.h b/include/linux/security.h
index 06f7c50ce77f..f29388180fab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -120,6 +120,7 @@ enum lockdown_reason {
 	LOCKDOWN_MMIOTRACE,
 	LOCKDOWN_DEBUGFS,
 	LOCKDOWN_XMON_WR,
+	LOCKDOWN_VFIO_NOIOMMU,
 	LOCKDOWN_INTEGRITY_MAX,
 	LOCKDOWN_KCORE,
 	LOCKDOWN_KPROBES,
diff --git a/security/security.c b/security/security.c
index b38155b2de83..33c3ddb6dcab 100644
--- a/security/security.c
+++ b/security/security.c
@@ -58,6 +58,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
 	[LOCKDOWN_MMIOTRACE] = "unsafe mmio",
 	[LOCKDOWN_DEBUGFS] = "debugfs access",
 	[LOCKDOWN_XMON_WR] = "xmon write access",
+	[LOCKDOWN_VFIO_NOIOMMU] = "VFIO unsafe no-iommu mode",
 	[LOCKDOWN_INTEGRITY_MAX] = "integrity",
 	[LOCKDOWN_KCORE] = "/proc/kcore access",
 	[LOCKDOWN_KPROBES] = "use of kprobes",
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
  2021-05-06  9:18 [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down Maxime Coquelin
@ 2021-05-06 21:50 ` Alex Williamson
  2021-05-07  8:37   ` Ondrej Mosnacek
  2021-05-11  2:58 ` Kees Cook
  1 sibling, 1 reply; 7+ messages in thread
From: Alex Williamson @ 2021-05-06 21:50 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: jmorris, dhowells, linux-kernel, linux-security-module, kvm,
	mjg59, keescook, cohuck

On Thu,  6 May 2021 11:18:59 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
> the PCI BARs via the device's sysfs, which is locked down when
> the kernel is locked down.
> 
> Indeed, it is possible for an attacker to craft DMA requests
> to modify kernel's code or leak secrets stored in the kernel,
> since the device is not isolated by an IOMMU.
> 
> This patch introduces a new integrity lockdown reason for the
> unsafe VFIO no-iommu mode.

I'm hoping security folks will chime in here as I'm not familiar with
the standard practices for new lockdown reasons.  The vfio no-iommu
backend is clearly an integrity risk, which is why it's already hidden
behind a separate Kconfig option, requires RAWIO capabilities, and
taints the kernel if it's used, but I agree that preventing it during
lockdown seems like a good additional step.

Is it generally advised to create specific reasons, like done here, or
should we aim to create a more generic reason related to unrestricted
userspace DMA?

I understand we don't want to re-use PCI_ACCESS because the vfio
no-iommu backend is device agnostic, it can be used for both PCI and
non-PCI devices.

> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/vfio/vfio.c      | 13 +++++++++----
>  include/linux/security.h |  1 +
>  security/security.c      |  1 +
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 5e631c359ef2..fe466d6ea5d8 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -25,6 +25,7 @@
>  #include <linux/pci.h>
>  #include <linux/rwsem.h>
>  #include <linux/sched.h>
> +#include <linux/security.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
>  #include <linux/string.h>
> @@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
>  {
>  	if (arg != VFIO_NOIOMMU_IOMMU)
>  		return ERR_PTR(-EINVAL);
> -	if (!capable(CAP_SYS_RAWIO))
> +	if (!capable(CAP_SYS_RAWIO) ||
> +			security_locked_down(LOCKDOWN_VFIO_NOIOMMU))
>  		return ERR_PTR(-EPERM);
>  
>  	return NULL;
> @@ -1280,7 +1282,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
>  	if (atomic_read(&group->container_users))
>  		return -EINVAL;
>  
> -	if (group->noiommu && !capable(CAP_SYS_RAWIO))
> +	if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> +			security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
>  		return -EPERM;
>  
>  	f = fdget(container_fd);
> @@ -1362,7 +1365,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>  	    !group->container->iommu_driver || !vfio_group_viable(group))
>  		return -EINVAL;
>  
> -	if (group->noiommu && !capable(CAP_SYS_RAWIO))
> +	if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> +			security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
>  		return -EPERM;
>  
>  	device = vfio_device_get_from_name(group, buf);
> @@ -1490,7 +1494,8 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
>  	if (!group)
>  		return -ENODEV;
>  
> -	if (group->noiommu && !capable(CAP_SYS_RAWIO)) {
> +	if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> +			security_locked_down(LOCKDOWN_VFIO_NOIOMMU))) {
>  		vfio_group_put(group);
>  		return -EPERM;
>  	}

In these cases where we're testing RAWIO, the idea is to raise the
barrier of passing file descriptors to unprivileged users.  Is lockdown
sufficiently static that we might really only need the test on open?
The latter three cases here only make sense if the user were able to
open a no-iommu context when lockdown is not enabled, then lockdown is
later enabled preventing them from doing anything with that context...
but not preventing ongoing unsafe usage that might already exist.  I
suspect for that reason that lockdown is static and we really only need
the test on open.  Thanks,

Alex

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 06f7c50ce77f..f29388180fab 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -120,6 +120,7 @@ enum lockdown_reason {
>  	LOCKDOWN_MMIOTRACE,
>  	LOCKDOWN_DEBUGFS,
>  	LOCKDOWN_XMON_WR,
> +	LOCKDOWN_VFIO_NOIOMMU,
>  	LOCKDOWN_INTEGRITY_MAX,
>  	LOCKDOWN_KCORE,
>  	LOCKDOWN_KPROBES,
> diff --git a/security/security.c b/security/security.c
> index b38155b2de83..33c3ddb6dcab 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -58,6 +58,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>  	[LOCKDOWN_MMIOTRACE] = "unsafe mmio",
>  	[LOCKDOWN_DEBUGFS] = "debugfs access",
>  	[LOCKDOWN_XMON_WR] = "xmon write access",
> +	[LOCKDOWN_VFIO_NOIOMMU] = "VFIO unsafe no-iommu mode",
>  	[LOCKDOWN_INTEGRITY_MAX] = "integrity",
>  	[LOCKDOWN_KCORE] = "/proc/kcore access",
>  	[LOCKDOWN_KPROBES] = "use of kprobes",


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
  2021-05-06 21:50 ` Alex Williamson
@ 2021-05-07  8:37   ` Ondrej Mosnacek
  2021-05-07  9:11     ` Maxime Coquelin
  0 siblings, 1 reply; 7+ messages in thread
From: Ondrej Mosnacek @ 2021-05-07  8:37 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Maxime Coquelin, James Morris, David Howells,
	Linux kernel mailing list, Linux Security Module list, kvm,
	mjg59, Kees Cook, Cornelia Huck

On Thu, May 6, 2021 at 11:50 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Thu,  6 May 2021 11:18:59 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
> > When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
> > the PCI BARs via the device's sysfs, which is locked down when
> > the kernel is locked down.
> >
> > Indeed, it is possible for an attacker to craft DMA requests
> > to modify kernel's code or leak secrets stored in the kernel,
> > since the device is not isolated by an IOMMU.
> >
> > This patch introduces a new integrity lockdown reason for the
> > unsafe VFIO no-iommu mode.
>
> I'm hoping security folks will chime in here as I'm not familiar with
> the standard practices for new lockdown reasons.  The vfio no-iommu
> backend is clearly an integrity risk, which is why it's already hidden
> behind a separate Kconfig option, requires RAWIO capabilities, and
> taints the kernel if it's used, but I agree that preventing it during
> lockdown seems like a good additional step.
>
> Is it generally advised to create specific reasons, like done here, or
> should we aim to create a more generic reason related to unrestricted
> userspace DMA?
>
> I understand we don't want to re-use PCI_ACCESS because the vfio
> no-iommu backend is device agnostic, it can be used for both PCI and
> non-PCI devices.
>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >  drivers/vfio/vfio.c      | 13 +++++++++----
> >  include/linux/security.h |  1 +
> >  security/security.c      |  1 +
> >  3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 5e631c359ef2..fe466d6ea5d8 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/rwsem.h>
> >  #include <linux/sched.h>
> > +#include <linux/security.h>
> >  #include <linux/slab.h>
> >  #include <linux/stat.h>
> >  #include <linux/string.h>
> > @@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
> >  {
> >       if (arg != VFIO_NOIOMMU_IOMMU)
> >               return ERR_PTR(-EINVAL);
> > -     if (!capable(CAP_SYS_RAWIO))
> > +     if (!capable(CAP_SYS_RAWIO) ||
> > +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU))
> >               return ERR_PTR(-EPERM);
> >
> >       return NULL;
> > @@ -1280,7 +1282,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
> >       if (atomic_read(&group->container_users))
> >               return -EINVAL;
> >
> > -     if (group->noiommu && !capable(CAP_SYS_RAWIO))
> > +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> > +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
> >               return -EPERM;
> >
> >       f = fdget(container_fd);
> > @@ -1362,7 +1365,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> >           !group->container->iommu_driver || !vfio_group_viable(group))
> >               return -EINVAL;
> >
> > -     if (group->noiommu && !capable(CAP_SYS_RAWIO))
> > +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> > +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
> >               return -EPERM;
> >
> >       device = vfio_device_get_from_name(group, buf);
> > @@ -1490,7 +1494,8 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
> >       if (!group)
> >               return -ENODEV;
> >
> > -     if (group->noiommu && !capable(CAP_SYS_RAWIO)) {
> > +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> > +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU))) {
> >               vfio_group_put(group);
> >               return -EPERM;
> >       }
>
> In these cases where we're testing RAWIO, the idea is to raise the
> barrier of passing file descriptors to unprivileged users.  Is lockdown
> sufficiently static that we might really only need the test on open?
> The latter three cases here only make sense if the user were able to
> open a no-iommu context when lockdown is not enabled, then lockdown is
> later enabled preventing them from doing anything with that context...
> but not preventing ongoing unsafe usage that might already exist.  I
> suspect for that reason that lockdown is static and we really only need
> the test on open.  Thanks,

Note that SELinux now also implements the locked_down hook and that
implementation is not static like the Lockdown LSM's. It checks
whether the current task's SELinux domain has either integrity or
confidentiality permission granted by the policy, so for SELinux it
makes sense to have the lockdown hook called in these other places as
well.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
  2021-05-07  8:37   ` Ondrej Mosnacek
@ 2021-05-07  9:11     ` Maxime Coquelin
  2021-05-07 12:31       ` Ondrej Mosnacek
  0 siblings, 1 reply; 7+ messages in thread
From: Maxime Coquelin @ 2021-05-07  9:11 UTC (permalink / raw)
  To: Ondrej Mosnacek, Alex Williamson
  Cc: James Morris, David Howells, Linux kernel mailing list,
	Linux Security Module list, kvm, mjg59, Kees Cook, Cornelia Huck



On 5/7/21 10:37 AM, Ondrej Mosnacek wrote:
> On Thu, May 6, 2021 at 11:50 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
>> On Thu,  6 May 2021 11:18:59 +0200
>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>
>>> When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
>>> the PCI BARs via the device's sysfs, which is locked down when
>>> the kernel is locked down.
>>>
>>> Indeed, it is possible for an attacker to craft DMA requests
>>> to modify kernel's code or leak secrets stored in the kernel,
>>> since the device is not isolated by an IOMMU.
>>>
>>> This patch introduces a new integrity lockdown reason for the
>>> unsafe VFIO no-iommu mode.
>>
>> I'm hoping security folks will chime in here as I'm not familiar with
>> the standard practices for new lockdown reasons.  The vfio no-iommu
>> backend is clearly an integrity risk, which is why it's already hidden
>> behind a separate Kconfig option, requires RAWIO capabilities, and
>> taints the kernel if it's used, but I agree that preventing it during
>> lockdown seems like a good additional step.
>>
>> Is it generally advised to create specific reasons, like done here, or
>> should we aim to create a more generic reason related to unrestricted
>> userspace DMA?

I am fine with a more generic reason. I'm also not sure what is the best
thing to do in term of granularity.

>> I understand we don't want to re-use PCI_ACCESS because the vfio
>> no-iommu backend is device agnostic, it can be used for both PCI and
>> non-PCI devices.

Right, that's why I created a new reason.

>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  drivers/vfio/vfio.c      | 13 +++++++++----
>>>  include/linux/security.h |  1 +
>>>  security/security.c      |  1 +
>>>  3 files changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index 5e631c359ef2..fe466d6ea5d8 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/pci.h>
>>>  #include <linux/rwsem.h>
>>>  #include <linux/sched.h>
>>> +#include <linux/security.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/stat.h>
>>>  #include <linux/string.h>
>>> @@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
>>>  {
>>>       if (arg != VFIO_NOIOMMU_IOMMU)
>>>               return ERR_PTR(-EINVAL);
>>> -     if (!capable(CAP_SYS_RAWIO))
>>> +     if (!capable(CAP_SYS_RAWIO) ||
>>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU))
>>>               return ERR_PTR(-EPERM);
>>>
>>>       return NULL;
>>> @@ -1280,7 +1282,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
>>>       if (atomic_read(&group->container_users))
>>>               return -EINVAL;
>>>
>>> -     if (group->noiommu && !capable(CAP_SYS_RAWIO))
>>> +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
>>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
>>>               return -EPERM;
>>>
>>>       f = fdget(container_fd);
>>> @@ -1362,7 +1365,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>>>           !group->container->iommu_driver || !vfio_group_viable(group))
>>>               return -EINVAL;
>>>
>>> -     if (group->noiommu && !capable(CAP_SYS_RAWIO))
>>> +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
>>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
>>>               return -EPERM;
>>>
>>>       device = vfio_device_get_from_name(group, buf);
>>> @@ -1490,7 +1494,8 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
>>>       if (!group)
>>>               return -ENODEV;
>>>
>>> -     if (group->noiommu && !capable(CAP_SYS_RAWIO)) {
>>> +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
>>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU))) {
>>>               vfio_group_put(group);
>>>               return -EPERM;
>>>       }
>>
>> In these cases where we're testing RAWIO, the idea is to raise the
>> barrier of passing file descriptors to unprivileged users.  Is lockdown
>> sufficiently static that we might really only need the test on open?
>> The latter three cases here only make sense if the user were able to
>> open a no-iommu context when lockdown is not enabled, then lockdown is
>> later enabled preventing them from doing anything with that context...
>> but not preventing ongoing unsafe usage that might already exist.  I
>> suspect for that reason that lockdown is static and we really only need
>> the test on open.  Thanks,
> 
> Note that SELinux now also implements the locked_down hook and that
> implementation is not static like the Lockdown LSM's. It checks
> whether the current task's SELinux domain has either integrity or
> confidentiality permission granted by the policy, so for SELinux it
> makes sense to have the lockdown hook called in these other places as
> well.
> 

Thanks Ondrej for the insights, is there any plan for selinux to support
finer granularity than integrity and confidentiality? I'm not sure it
makes sense, but it might help to understand if we should choose between
a "VFIO no-iommu mode" reason and a more generic userspace DMA one.

Maxime


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
  2021-05-07  9:11     ` Maxime Coquelin
@ 2021-05-07 12:31       ` Ondrej Mosnacek
  0 siblings, 0 replies; 7+ messages in thread
From: Ondrej Mosnacek @ 2021-05-07 12:31 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: Alex Williamson, James Morris, David Howells,
	Linux kernel mailing list, Linux Security Module list, kvm,
	mjg59, Kees Cook, Cornelia Huck, Paul Moore, Stephen Smalley,
	SElinux list

(Adding the SELinux list, Paul, and Stephen to Cc)

On Fri, May 7, 2021 at 11:11 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 5/7/21 10:37 AM, Ondrej Mosnacek wrote:
> > On Thu, May 6, 2021 at 11:50 PM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> >> On Thu,  6 May 2021 11:18:59 +0200
> >> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> >>
> >>> When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
> >>> the PCI BARs via the device's sysfs, which is locked down when
> >>> the kernel is locked down.
> >>>
> >>> Indeed, it is possible for an attacker to craft DMA requests
> >>> to modify kernel's code or leak secrets stored in the kernel,
> >>> since the device is not isolated by an IOMMU.
> >>>
> >>> This patch introduces a new integrity lockdown reason for the
> >>> unsafe VFIO no-iommu mode.
> >>
> >> I'm hoping security folks will chime in here as I'm not familiar with
> >> the standard practices for new lockdown reasons.  The vfio no-iommu
> >> backend is clearly an integrity risk, which is why it's already hidden
> >> behind a separate Kconfig option, requires RAWIO capabilities, and
> >> taints the kernel if it's used, but I agree that preventing it during
> >> lockdown seems like a good additional step.
> >>
> >> Is it generally advised to create specific reasons, like done here, or
> >> should we aim to create a more generic reason related to unrestricted
> >> userspace DMA?
>
> I am fine with a more generic reason. I'm also not sure what is the best
> thing to do in term of granularity.
>
> >> I understand we don't want to re-use PCI_ACCESS because the vfio
> >> no-iommu backend is device agnostic, it can be used for both PCI and
> >> non-PCI devices.
>
> Right, that's why I created a new reason.
>
> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> ---
> >>>  drivers/vfio/vfio.c      | 13 +++++++++----
> >>>  include/linux/security.h |  1 +
> >>>  security/security.c      |  1 +
> >>>  3 files changed, 11 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >>> index 5e631c359ef2..fe466d6ea5d8 100644
> >>> --- a/drivers/vfio/vfio.c
> >>> +++ b/drivers/vfio/vfio.c
> >>> @@ -25,6 +25,7 @@
> >>>  #include <linux/pci.h>
> >>>  #include <linux/rwsem.h>
> >>>  #include <linux/sched.h>
> >>> +#include <linux/security.h>
> >>>  #include <linux/slab.h>
> >>>  #include <linux/stat.h>
> >>>  #include <linux/string.h>
> >>> @@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
> >>>  {
> >>>       if (arg != VFIO_NOIOMMU_IOMMU)
> >>>               return ERR_PTR(-EINVAL);
> >>> -     if (!capable(CAP_SYS_RAWIO))
> >>> +     if (!capable(CAP_SYS_RAWIO) ||
> >>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU))
> >>>               return ERR_PTR(-EPERM);
> >>>
> >>>       return NULL;
> >>> @@ -1280,7 +1282,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
> >>>       if (atomic_read(&group->container_users))
> >>>               return -EINVAL;
> >>>
> >>> -     if (group->noiommu && !capable(CAP_SYS_RAWIO))
> >>> +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> >>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
> >>>               return -EPERM;
> >>>
> >>>       f = fdget(container_fd);
> >>> @@ -1362,7 +1365,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> >>>           !group->container->iommu_driver || !vfio_group_viable(group))
> >>>               return -EINVAL;
> >>>
> >>> -     if (group->noiommu && !capable(CAP_SYS_RAWIO))
> >>> +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> >>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU)))
> >>>               return -EPERM;
> >>>
> >>>       device = vfio_device_get_from_name(group, buf);
> >>> @@ -1490,7 +1494,8 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
> >>>       if (!group)
> >>>               return -ENODEV;
> >>>
> >>> -     if (group->noiommu && !capable(CAP_SYS_RAWIO)) {
> >>> +     if (group->noiommu && (!capable(CAP_SYS_RAWIO) ||
> >>> +                     security_locked_down(LOCKDOWN_VFIO_NOIOMMU))) {
> >>>               vfio_group_put(group);
> >>>               return -EPERM;
> >>>       }
> >>
> >> In these cases where we're testing RAWIO, the idea is to raise the
> >> barrier of passing file descriptors to unprivileged users.  Is lockdown
> >> sufficiently static that we might really only need the test on open?
> >> The latter three cases here only make sense if the user were able to
> >> open a no-iommu context when lockdown is not enabled, then lockdown is
> >> later enabled preventing them from doing anything with that context...
> >> but not preventing ongoing unsafe usage that might already exist.  I
> >> suspect for that reason that lockdown is static and we really only need
> >> the test on open.  Thanks,
> >
> > Note that SELinux now also implements the locked_down hook and that
> > implementation is not static like the Lockdown LSM's. It checks
> > whether the current task's SELinux domain has either integrity or
> > confidentiality permission granted by the policy, so for SELinux it
> > makes sense to have the lockdown hook called in these other places as
> > well.
> >
>
> Thanks Ondrej for the insights, is there any plan for selinux to support
> finer granularity than integrity and confidentiality? I'm not sure it
> makes sense, but it might help to understand if we should choose between
> a "VFIO no-iommu mode" reason and a more generic userspace DMA one.

Looking at the ML discussion under the original patch posting [1],
having a finer granularity was preferred, but there was a concern that
the list of reasons would change too much, making the kernel <->
policy interface too unstable. Looking at the git log, the list of
lockdown reasons hasn't changed much since the original SELinux
implementation was added (there was just one addition, not counting
this patch), so it looks like switching SELinux to finer granularity
would be viable now.

I'm not sure whether the less or more generic variant would be better
here... Perhaps Stephen or Paul will have an opinion.

[1] https://lore.kernel.org/selinux/365ca063-6efd-8051-8d4b-5c8aef0d2e12@tycho.nsa.gov/T/

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
  2021-05-06  9:18 [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down Maxime Coquelin
  2021-05-06 21:50 ` Alex Williamson
@ 2021-05-11  2:58 ` Kees Cook
  2021-05-20  8:38   ` Maxime Coquelin
  1 sibling, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-05-11  2:58 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: alex.williamson, jmorris, dhowells, linux-kernel,
	linux-security-module, kvm, mjg59, cohuck

On Thu, May 06, 2021 at 11:18:59AM +0200, Maxime Coquelin wrote:
> When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
> the PCI BARs via the device's sysfs, which is locked down when
> the kernel is locked down.
> 
> Indeed, it is possible for an attacker to craft DMA requests
> to modify kernel's code or leak secrets stored in the kernel,
> since the device is not isolated by an IOMMU.
> 
> This patch introduces a new integrity lockdown reason for the
> unsafe VFIO no-iommu mode.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/vfio/vfio.c      | 13 +++++++++----
>  include/linux/security.h |  1 +
>  security/security.c      |  1 +
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 5e631c359ef2..fe466d6ea5d8 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -25,6 +25,7 @@
>  #include <linux/pci.h>
>  #include <linux/rwsem.h>
>  #include <linux/sched.h>
> +#include <linux/security.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
>  #include <linux/string.h>
> @@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
>  {
>  	if (arg != VFIO_NOIOMMU_IOMMU)
>  		return ERR_PTR(-EINVAL);
> -	if (!capable(CAP_SYS_RAWIO))
> +	if (!capable(CAP_SYS_RAWIO) ||
> +			security_locked_down(LOCKDOWN_VFIO_NOIOMMU))

The LSM hook check should come before the capable() check to avoid
setting PF_SUPERPRIV if capable() passes and the LSM doesn't.

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 06f7c50ce77f..f29388180fab 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -120,6 +120,7 @@ enum lockdown_reason {
>  	LOCKDOWN_MMIOTRACE,
>  	LOCKDOWN_DEBUGFS,
>  	LOCKDOWN_XMON_WR,
> +	LOCKDOWN_VFIO_NOIOMMU,
>  	LOCKDOWN_INTEGRITY_MAX,
>  	LOCKDOWN_KCORE,
>  	LOCKDOWN_KPROBES,

Is the security threat specific to VFIO? (i.e. could other interfaces
want a similar thing, such that naming this VFIO doesn't make sense?

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
  2021-05-11  2:58 ` Kees Cook
@ 2021-05-20  8:38   ` Maxime Coquelin
  0 siblings, 0 replies; 7+ messages in thread
From: Maxime Coquelin @ 2021-05-20  8:38 UTC (permalink / raw)
  To: Kees Cook
  Cc: alex.williamson, jmorris, dhowells, linux-kernel,
	linux-security-module, kvm, mjg59, cohuck



On 5/11/21 4:58 AM, Kees Cook wrote:
> On Thu, May 06, 2021 at 11:18:59AM +0200, Maxime Coquelin wrote:
>> When no-IOMMU mode is enabled, VFIO is as unsafe as accessing
>> the PCI BARs via the device's sysfs, which is locked down when
>> the kernel is locked down.
>>
>> Indeed, it is possible for an attacker to craft DMA requests
>> to modify kernel's code or leak secrets stored in the kernel,
>> since the device is not isolated by an IOMMU.
>>
>> This patch introduces a new integrity lockdown reason for the
>> unsafe VFIO no-iommu mode.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/vfio/vfio.c      | 13 +++++++++----
>>  include/linux/security.h |  1 +
>>  security/security.c      |  1 +
>>  3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 5e631c359ef2..fe466d6ea5d8 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/pci.h>
>>  #include <linux/rwsem.h>
>>  #include <linux/sched.h>
>> +#include <linux/security.h>
>>  #include <linux/slab.h>
>>  #include <linux/stat.h>
>>  #include <linux/string.h>
>> @@ -165,7 +166,8 @@ static void *vfio_noiommu_open(unsigned long arg)
>>  {
>>  	if (arg != VFIO_NOIOMMU_IOMMU)
>>  		return ERR_PTR(-EINVAL);
>> -	if (!capable(CAP_SYS_RAWIO))
>> +	if (!capable(CAP_SYS_RAWIO) ||
>> +			security_locked_down(LOCKDOWN_VFIO_NOIOMMU))
> 
> The LSM hook check should come before the capable() check to avoid
> setting PF_SUPERPRIV if capable() passes and the LSM doesn't.

OK, good to know, I'll swap in next revision.

BTW, it seems other places are doing the capable check before the LSM
hook check, for example in ioport [0].

>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 06f7c50ce77f..f29388180fab 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -120,6 +120,7 @@ enum lockdown_reason {
>>  	LOCKDOWN_MMIOTRACE,
>>  	LOCKDOWN_DEBUGFS,
>>  	LOCKDOWN_XMON_WR,
>> +	LOCKDOWN_VFIO_NOIOMMU,
>>  	LOCKDOWN_INTEGRITY_MAX,
>>  	LOCKDOWN_KCORE,
>>  	LOCKDOWN_KPROBES,
> 
> Is the security threat specific to VFIO? (i.e. could other interfaces
> want a similar thing, such that naming this VFIO doesn't make sense?

It could possibly in theory, maybe something like
"LOCKDOWN_UNRESTRICTED_DMA" would be a better fit?

Maxime

[0]:
https://elixir.bootlin.com/linux/v5.13-rc2/source/arch/x86/kernel/ioport.c#L73


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-05-20  8:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06  9:18 [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down Maxime Coquelin
2021-05-06 21:50 ` Alex Williamson
2021-05-07  8:37   ` Ondrej Mosnacek
2021-05-07  9:11     ` Maxime Coquelin
2021-05-07 12:31       ` Ondrej Mosnacek
2021-05-11  2:58 ` Kees Cook
2021-05-20  8:38   ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).