linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: jmorris@namei.org, dhowells@redhat.com,
	linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org, kvm@vger.kernel.org,
	mjg59@srcf.ucam.org, keescook@chromium.org, cohuck@redhat.com
Subject: Re: [PATCH] vfio: Lock down no-IOMMU mode when kernel is locked down
Date: Thu, 6 May 2021 15:50:04 -0600	[thread overview]
Message-ID: <20210506155004.7e214d8f@redhat.com> (raw)
In-Reply-To: <20210506091859.6961-1-maxime.coquelin@redhat.com>

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",


  reply	other threads:[~2021-05-06 21:50 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210506155004.7e214d8f@redhat.com \
    --to=alex.williamson@redhat.com \
    --cc=cohuck@redhat.com \
    --cc=dhowells@redhat.com \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=mjg59@srcf.ucam.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).