| Message ID | 1624032777-7013-13-git-send-email-ross.philipson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
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@oracle.com> > --- > 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. > + > 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 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... It might be reasonable to make IOMMU_DEFAULT_PASSTHROUGH depend on !SECURE_LAUNCH for clarity though. Robin. > } > > void iommu_set_default_translated(bool cmd_line) >
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@oracle.com> >> --- >> 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. > >> + >> 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. > > 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. Thank you, Ross > > Robin. > >> } >> void iommu_set_default_translated(bool cmd_line) >>
On Mon, Jun 21, 2021 at 10:51 AM Ross Philipson <ross.philipson@oracle.com> 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@oracle.com> > >> --- > >> 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. > > > > >> + > >> 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. > I don't really like all these special cases. Generically, what you're trying to do is (AFAICT) to get the kernel to run in a mode in which it does its best not to trust attached devices. Nothing about this is specific to Secure Launch. There are plenty of scenarios in which this the case: - Virtual devices in a VM host outside the TCB, e.g. VDUSE, Xen device domains (did I get the name right), whatever tricks QEMU has, etc. - SRTM / DRTM technologies (including but not limited to Secure Launch -- plain old Secure Boot can work like this too). - Secure guest technologies, including but not limited to TDX and SEV. - Any computer with a USB-C port or other external DMA-capable port. - Regular computers in which the admin wants to enable this mode for whatever reason. Can you folks all please agree on a coordinated way for a Linux kernel to configure itself appropriately? Or to be configured via initramfs, boot option, or some other trusted source of configuration supplied at boot time? We don't need a whole bunch of if (TDX), if (SEV), if (secure launch), if (I have a USB-C port with PCIe exposed), if (running on Xen), and similar checks all over the place.
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@oracle.com> >>> --- >>> 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. >> 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.
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@oracle.com> >>>> --- >>>> 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.
On 6/21/21 5:15 PM, Andy Lutomirski wrote: > On Mon, Jun 21, 2021 at 10:51 AM Ross Philipson > <ross.philipson@oracle.com> 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@oracle.com> >>>> --- >>>> 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. >> >>> >>>> + >>>> 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. >> > > I don't really like all these special cases. Generically, what you're > trying to do is (AFAICT) to get the kernel to run in a mode in which > it does its best not to trust attached devices. Nothing about this is > specific to Secure Launch. There are plenty of scenarios in which > this the case: > > - Virtual devices in a VM host outside the TCB, e.g. VDUSE, Xen > device domains (did I get the name right), whatever tricks QEMU has, > etc. > - SRTM / DRTM technologies (including but not limited to Secure > Launch -- plain old Secure Boot can work like this too). > - Secure guest technologies, including but not limited to TDX and SEV. > - Any computer with a USB-C port or other external DMA-capable port. > - Regular computers in which the admin wants to enable this mode for > whatever reason. > > Can you folks all please agree on a coordinated way for a Linux kernel > to configure itself appropriately? Or to be configured via initramfs, > boot option, or some other trusted source of configuration supplied at > boot time? We don't need a whole bunch of if (TDX), if (SEV), if > (secure launch), if (I have a USB-C port with PCIe exposed), if > (running on Xen), and similar checks all over the place. > I replied to Robin Murphy in another thread. As far as the IOMMU is concerned, I think we need to rethink our approach. As to the other technologies you mention here, we have not considered special casing anything at this point. Thanks Ross
On 6/21/21 5:15 PM, Andy Lutomirski wrote: > On Mon, Jun 21, 2021 at 10:51 AM Ross Philipson > <ross.philipson@oracle.com> wrote: >> >> On 6/18/21 2:32 PM, Robin Murphy wrote: >>> On 2021-06-18 17:12, Ross Philipson wrote: >>>> @@ -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. >> > > I don't really like all these special cases. Generically, what you're > trying to do is (AFAICT) to get the kernel to run in a mode in which > it does its best not to trust attached devices. Nothing about this is > specific to Secure Launch. There are plenty of scenarios in which > this the case: > > - Virtual devices in a VM host outside the TCB, e.g. VDUSE, Xen > device domains (did I get the name right), whatever tricks QEMU has, > etc. > - SRTM / DRTM technologies (including but not limited to Secure > Launch -- plain old Secure Boot can work like this too). > - Secure guest technologies, including but not limited to TDX and SEV. > - Any computer with a USB-C port or other external DMA-capable port. > - Regular computers in which the admin wants to enable this mode for > whatever reason. > > Can you folks all please agree on a coordinated way for a Linux kernel > to configure itself appropriately? Or to be configured via initramfs, > boot option, or some other trusted source of configuration supplied at > boot time? We don't need a whole bunch of if (TDX), if (SEV), if > (secure launch), if (I have a USB-C port with PCIe exposed), if > (running on Xen), and similar checks all over the place. Hey Andy, On behalf of Ross and myself I wanted to follow up on the points raised here. While there is an interest to ensure a system is properly configured we should not be blocking the user from configuring the system as they desire. Instead we are taking the approach to document the SecureLaunch capability, in particular the recommend way to configure the kernel to appropriately use the capability using the already existing methods such as using kernel parameters. Hopefully that will address the concerns in the short term. Looking forward, we do have a vested interest in ensuring there is an ability to configure access control for security and safety critical solutions and would be grateful if we would be included in any discussions or working groups that might be looking into unifying how all these security technologies should be configuring the Linux kernel. V/r, Daniel P. Smith
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; + 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; } void iommu_set_default_translated(bool cmd_line)
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@oracle.com> --- drivers/iommu/intel/iommu.c | 5 +++++ drivers/iommu/iommu.c | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-)