On 6/22/21 7:06 AM, Robin Murphy wrote:
On 2021-06-21 18:51, Ross Philipson wrote:
On 6/18/21 2:32 PM, Robin Murphy wrote:
On 2021-06-18 17:12, Ross Philipson wrote:
The IOMMU should always be set to default translated type after
the PMRs are disabled to protect the MLE from DMA.
Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
---
drivers/iommu/intel/iommu.c | 5 +++++
drivers/iommu/iommu.c | 6 +++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index be35284..4f0256d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -41,6 +41,7 @@
#include <linux/dma-direct.h>
#include <linux/crash_dump.h>
#include <linux/numa.h>
+#include <linux/slaunch.h>
#include <asm/irq_remapping.h>
#include <asm/cacheflush.h>
#include <asm/iommu.h>
@@ -2877,6 +2878,10 @@ static bool device_is_rmrr_locked(struct device
*dev)
*/
static int device_def_domain_type(struct device *dev)
{
+ /* Do not allow identity domain when Secure Launch is
configured */
+ if (slaunch_get_flags() & SL_FLAG_ACTIVE)
+ return IOMMU_DOMAIN_DMA;
Is this specific to Intel? It seems like it could easily be done
commonly like the check for untrusted external devices.
It is currently Intel only but that will change. I will look into what
you suggest.
Yeah, it's simple and unobtrusive enough that I reckon it's worth going
straight to the common version if it's worth doing at all.
+
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 808ab70d..d49b7dd 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -23,6 +23,7 @@
#include <linux/property.h>
#include <linux/fsl/mc.h>
#include <linux/module.h>
+#include <linux/slaunch.h>
#include <trace/events/iommu.h>
static struct kset *iommu_group_kset;
@@ -2761,7 +2762,10 @@ void iommu_set_default_passthrough(bool
cmd_line)
{
if (cmd_line)
iommu_cmd_line |= IOMMU_CMD_LINE_DMA_API;
- iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
+
+ /* Do not allow identity domain when Secure Launch is
configured */
+ if (!(slaunch_get_flags() & SL_FLAG_ACTIVE))
+ iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
Quietly ignoring the setting and possibly leaving iommu_def_domain_type
uninitialised (note that 0 is not actually a usable type) doesn't seem
great. AFAICS this probably warrants similar treatment to the
Ok so I guess it would be better to set it to IOMMU_DOMAIN_DMA event
though passthrough was requested. Or perhaps something more is needed
here?
mem_encrypt_active() case - there doesn't seem a great deal of value in
trying to save users from themselves if they care about measured boot
yet explicitly pass options which may compromise measured boot. If you
really want to go down that route there's at least the sysfs interface
you'd need to nobble as well, not to mention the various ways of
completely disabling IOMMUs...
Doing a secure launch with the kernel is not a general purpose user use
case. A lot of work is done to secure the environment. Allowing
passthrough mode would leave the secure launch kernel exposed to DMA. I
think what we are trying to do here is what we intend though there may
be a better way or perhaps it is incomplete as you suggest.
On second thoughts this is overkill anyway - if you do hook
iommu_get_def_domain_type(), you're done (in terms of the kernel-managed
setting, at least); it doesn't matter what iommu_def_domain_type gets
set to if will never get used. However, since this isn't really a
per-device thing, it might be more semantically appropriate to leave
that alone and instead only massage the default type in
iommu_subsys_init(), as for memory encryption.
When you say "secure the environment", what's the actual threat model
here, i.e. who's securing what against whom? If it's a device lockdown
type thing where the system owner wants to defend against the end user
trying to mess with the software stack or gain access to parts they
shouldn't, then possibly you can trust the command line, but there are
definitely other places which need consideration. If on the other hand
it's more about giving the end user confidence that their choice of
software stack isn't being interfered with by a malicious host or
external third parties, then it probably leans towards the opposite
being true...
If the command line *is* within the threat model, consider "iommu=off"
and/or "intel_iommu=off" for example: I don't know how PMRs work, but I
can only imagine that that's liable to leave things either wide open, or
blocked to the point of no DMA working at all, neither of which seems to
be what you want. I'm guessing "intel_iommu=tboot_noforce" might have
some relevant implications too.
Thank you for you suggestions and feedback. Sorry we did not get back
sooner. After the comments from you and Andy Lutomirski we decided we
needed to re-imagine what we are trying to accomplish here and how else
we might approach it.
Ross
It might be reasonable to make IOMMU_DEFAULT_PASSTHROUGH depend on
!SECURE_LAUNCH for clarity though.
This came from a specific request to not make disabling IOMMU modes
build time dependent. This is because a secure launch enabled kernel can
also be booted as a general purpose kernel in cases where this is
desired.
Ah, thanks for clarifying - I was wondering about that aspect. FWIW,
note that that wouldn't actually change any functionality - it's a
non-default config option anyway, and users could still override it
either way in a non-secure-launch setup - but it sounds like it might be
effectively superfluous if you do need to make a more active runtime
decision anyway.
Cheers,
Robin.
