Received: by 2002:a05:6a10:a852:0:0:0:0 with SMTP id d18csp1740540pxy; Thu, 6 May 2021 14:58:04 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyaj/Z9rkjfCVWLWhQidWc8UjX4MU2IT/joE0PJ0fxPzsZMuqKq7jLUZKc6YRKLFGsAa1i3 X-Received: by 2002:a17:906:b1cc:: with SMTP id bv12mr6571422ejb.407.1620338284510; Thu, 06 May 2021 14:58:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1620338284; cv=none; d=google.com; s=arc-20160816; b=P5Djj8AT0TbFq4xyTtS6ZXuJsv8x1KVi5u7G1bPmovrGGnP8OgPmg8WlQHJ+sop0v1 ++QWiE05ucNYzgvbSdwQcj1NvY4p4i+kSsbR0zgoe08Kdb5+vXLDxCRKfUCxCjIsjcHN RU+SbZ1+9NkhMoz9eL5OBru7zfj61pG4Dah1pXxUTBCFQ5o+GS7MnC6BBzmowTCiVqSm 82NmVwuZLHeiVNDX7ab4TB9rq1X+pAGk5q4I8d57PVI2HosbkwMW2KlMDdgRwqP313tj OyFPaqqM/ZynVHG+pB6uAE2U1zTsldeoUSVyY22iZ3HeFTW8Hv8laTvyKzZbBk3gtE10 gF9w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date :dkim-signature; bh=Q9h3TdRqA3v9lLQkKiiTDr2ssav5eexul01Jy9Pakf4=; b=Ue5EWQG3h0ekVJJnmVwcA7TlBmJem25wgJKpZ6otKR2y+GadeGVk+9vy45SwXh7gqH t7+osFNZAsOqLZuoFzm4WHWJKEQEXCKOXdvmnp80DLhU9GxsEal6lADGYWThmbaW3Xp+ bVMyPytaC+16HqmHB3IPex6nYAEwaWtw1pKF8YYnMdWKE8/PKiT8cmUh9RTLfASqAT4o 5dXLeS99+5pS/wjztQW9ninaTYL0Hykle04Wl6i2XG4bbaZSeJf2Y6vgTeCms8RYPidf obOqWadjmZR8XTeHg/eEB70XxCJTqKp7YTYST9Znn64CPVEkmTPxt/DhhftguP8/Zru9 x7AA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fO4mkEN6; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id a13si3462928edx.408.2021.05.06.14.57.16; Thu, 06 May 2021 14:58:04 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fO4mkEN6; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230265AbhEFVvZ (ORCPT + 99 others); Thu, 6 May 2021 17:51:25 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:42455 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229572AbhEFVvY (ORCPT ); Thu, 6 May 2021 17:51:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620337825; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Q9h3TdRqA3v9lLQkKiiTDr2ssav5eexul01Jy9Pakf4=; b=fO4mkEN6myhHPlFhiJ24WHhKkaZy36xomXXLejScodSS9DnG9oqjb6o3d3QyBcc8kGVrPc 8KsowldXPG2NukCp1YtrEqKouK406paEOX8wZaBxrGYSTm+7QMfiyrUnItOzIMWR5wwgAE f90ih79bbLGW5mt24S+wzrOTavvQOGw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-196-eeDIo886N2awTTLzwNq-Ug-1; Thu, 06 May 2021 17:50:21 -0400 X-MC-Unique: eeDIo886N2awTTLzwNq-Ug-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5A7B7802938; Thu, 6 May 2021 21:50:20 +0000 (UTC) Received: from redhat.com (ovpn-113-225.phx2.redhat.com [10.3.113.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 48D845D9CA; Thu, 6 May 2021 21:50:05 +0000 (UTC) Date: Thu, 6 May 2021 15:50:04 -0600 From: Alex Williamson To: Maxime Coquelin 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 Message-ID: <20210506155004.7e214d8f@redhat.com> In-Reply-To: <20210506091859.6961-1-maxime.coquelin@redhat.com> References: <20210506091859.6961-1-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 6 May 2021 11:18:59 +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. 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 > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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",