diff mbox series

iommu/arm-smmu: Allow disabling bypass via kernel config

Message ID 20190214204433.155715-1-dianders@chromium.org (mailing list archive)
State New, archived
Headers show
Series iommu/arm-smmu: Allow disabling bypass via kernel config | expand

Commit Message

Douglas Anderson Feb. 14, 2019, 8:44 p.m. UTC
Right now the only way to disable the iommu bypass for the ARM SMMU is
with the kernel command line parameter 'arm-smmu.disable_bypass'.

In general kernel command line parameters make sense for things that
someone would like to tweak without rebuilding the kernel or for very
basic communication between the bootloader and the kernel, but are
awkward for other things.  Specifically:
* Human parsing of the kernel command line can be difficult since it's
  just a big runon space separated line of text.
* If every bit of the system was configured via the kernel command
  line the kernel command line would get very large and even more
  unwieldly.
* Typically there are not easy ways in build systems to adjust the
  kernel command line for config-like options.

Let's introduce a new config option that allows us to disable the
iommu bypass without affecting the existing default nor the existing
ability to adjust the configuration via kernel command line.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/iommu/Kconfig    | 22 ++++++++++++++++++++++
 drivers/iommu/arm-smmu.c |  3 ++-
 2 files changed, 24 insertions(+), 1 deletion(-)

Comments

Robin Murphy Feb. 14, 2019, 9:31 p.m. UTC | #1
Hi Doug,

On 2019-02-14 8:44 pm, Douglas Anderson wrote:
> Right now the only way to disable the iommu bypass for the ARM SMMU is
> with the kernel command line parameter 'arm-smmu.disable_bypass'.
> 
> In general kernel command line parameters make sense for things that
> someone would like to tweak without rebuilding the kernel or for very
> basic communication between the bootloader and the kernel, but are
> awkward for other things.  Specifically:
> * Human parsing of the kernel command line can be difficult since it's
>    just a big runon space separated line of text.
> * If every bit of the system was configured via the kernel command
>    line the kernel command line would get very large and even more
>    unwieldly.
> * Typically there are not easy ways in build systems to adjust the
>    kernel command line for config-like options.
> 
> Let's introduce a new config option that allows us to disable the
> iommu bypass without affecting the existing default nor the existing
> ability to adjust the configuration via kernel command line.

I say let's just flip the default - for a while now it's been one of 
those "oh yeah, we should probably do that" things that gets instantly 
forgotten again, so some 3rd-party demand is plenty to convince me :)

There are few reasons to allow unmatched stream bypass, and even fewer 
good ones, so I'd be happy to shift the command-line burden over to the 
esoteric cases at this point, and consider the config option in future 
if anyone from that camp pops up and screams hard enough.

Cheers,
Robin.

> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/iommu/Kconfig    | 22 ++++++++++++++++++++++
>   drivers/iommu/arm-smmu.c |  3 ++-
>   2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 46fcd75d4364..c614beab08f8 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -359,6 +359,28 @@ config ARM_SMMU
>   	  Say Y here if your SoC includes an IOMMU device implementing
>   	  the ARM SMMU architecture.
>   
> +config ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT
> +	bool "Default to disabling bypass on ARM SMMU v1 and v2"
> +	depends on ARM_SMMU
> +	default n
> +	help
> +	  Say Y here to (by default) disable bypass streams such that
> +	  incoming transactions from devices that are not attached to
> +	  an iommu domain will report an abort back to the device and
> +	  will not be allowed to pass through the SMMU.
> +
> +	  Historically the ARM SMMU v1 and v2 driver has defaulted
> +	  to allow bypass by default but it could be disabled with
> +	  the parameter 'arm-smmu.disable_bypass'.  The parameter is
> +	  still present and can be used to override this config
> +	  option, but this config option allows you to disable bypass
> +	  without bloating the kernel command line.
> +
> +	  Disabling bypass is more secure but presumably will break
> +	  old systems.
> +
> +	  Say N if unsure.
> +
>   config ARM_SMMU_V3
>   	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
>   	depends on ARM64
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 045d93884164..930c07635956 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -110,7 +110,8 @@ static int force_stage;
>   module_param(force_stage, int, S_IRUGO);
>   MODULE_PARM_DESC(force_stage,
>   	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
> -static bool disable_bypass;
> +static bool disable_bypass =
> +	IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT);
>   module_param(disable_bypass, bool, S_IRUGO);
>   MODULE_PARM_DESC(disable_bypass,
>   	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
>
Douglas Anderson Feb. 15, 2019, 12:40 a.m. UTC | #2
Hi,

On Thu, Feb 14, 2019 at 1:32 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> Hi Doug,
>
> On 2019-02-14 8:44 pm, Douglas Anderson wrote:
> > Right now the only way to disable the iommu bypass for the ARM SMMU is
> > with the kernel command line parameter 'arm-smmu.disable_bypass'.
> >
> > In general kernel command line parameters make sense for things that
> > someone would like to tweak without rebuilding the kernel or for very
> > basic communication between the bootloader and the kernel, but are
> > awkward for other things.  Specifically:
> > * Human parsing of the kernel command line can be difficult since it's
> >    just a big runon space separated line of text.
> > * If every bit of the system was configured via the kernel command
> >    line the kernel command line would get very large and even more
> >    unwieldly.
> > * Typically there are not easy ways in build systems to adjust the
> >    kernel command line for config-like options.
> >
> > Let's introduce a new config option that allows us to disable the
> > iommu bypass without affecting the existing default nor the existing
> > ability to adjust the configuration via kernel command line.
>
> I say let's just flip the default - for a while now it's been one of
> those "oh yeah, we should probably do that" things that gets instantly
> forgotten again, so some 3rd-party demand is plenty to convince me :)
>
> There are few reasons to allow unmatched stream bypass, and even fewer
> good ones, so I'd be happy to shift the command-line burden over to the
> esoteric cases at this point, and consider the config option in future
> if anyone from that camp pops up and screams hard enough.

Sure, I can submit that patch if we want.  I presume I'll get lots of
screaming but I'm used to that.  ;-)

...specifically I found that when I turned on "disably bypass" on my
board (sdm845-cheza, which is not yet upstream) that a bunch of things
that used to work broke.  That's a good thing because all the things
that broke need to be fixed properly (by adding the IOMMUs) but
presumably my board is not special in relying on the old insecure
behavior.

I'm about to head on vacation for a week so I'm not sure I'll get to
re-post before then.  If not I'll post this sometime after I get back
unless someone beats me to it.

-Doug
Rob Clark Feb. 15, 2019, 10:37 p.m. UTC | #3
On Thu, Feb 14, 2019 at 7:40 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Feb 14, 2019 at 1:32 PM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > Hi Doug,
> >
> > On 2019-02-14 8:44 pm, Douglas Anderson wrote:
> > > Right now the only way to disable the iommu bypass for the ARM SMMU is
> > > with the kernel command line parameter 'arm-smmu.disable_bypass'.
> > >
> > > In general kernel command line parameters make sense for things that
> > > someone would like to tweak without rebuilding the kernel or for very
> > > basic communication between the bootloader and the kernel, but are
> > > awkward for other things.  Specifically:
> > > * Human parsing of the kernel command line can be difficult since it's
> > >    just a big runon space separated line of text.
> > > * If every bit of the system was configured via the kernel command
> > >    line the kernel command line would get very large and even more
> > >    unwieldly.
> > > * Typically there are not easy ways in build systems to adjust the
> > >    kernel command line for config-like options.
> > >
> > > Let's introduce a new config option that allows us to disable the
> > > iommu bypass without affecting the existing default nor the existing
> > > ability to adjust the configuration via kernel command line.
> >
> > I say let's just flip the default - for a while now it's been one of
> > those "oh yeah, we should probably do that" things that gets instantly
> > forgotten again, so some 3rd-party demand is plenty to convince me :)
> >
> > There are few reasons to allow unmatched stream bypass, and even fewer
> > good ones, so I'd be happy to shift the command-line burden over to the
> > esoteric cases at this point, and consider the config option in future
> > if anyone from that camp pops up and screams hard enough.
>
> Sure, I can submit that patch if we want.  I presume I'll get lots of
> screaming but I'm used to that.  ;-)
>
> ...specifically I found that when I turned on "disably bypass" on my
> board (sdm845-cheza, which is not yet upstream) that a bunch of things
> that used to work broke.  That's a good thing because all the things
> that broke need to be fixed properly (by adding the IOMMUs) but
> presumably my board is not special in relying on the old insecure
> behavior.

So one niggly bit, beyond the drivers that aren't but should be using
iommu, is the case where bootloader lights up the display.  We
actually still have a lot of work to do for this (in that clk and
genpd drivers need to be aware of handover, in addition to just
iommu)..  But I'd rather not make a hard problem harder just yet.

(it gets worse, afaict so far on the windows-arm laptops since in that
case uefi/edk is actually taking the iommu out of bypass and things go
badly when arm-smmu disables clks after probe..)

But I might recommend making the default a kconfig option for now, so
in the distro kernel case, where display driver is a kernel module,
you aren't making a hard problem harder.  For cases where bootloader
isn't enabling display, things are much easier, and I think we could
switch the default sooner.  But I fear for cases where bootloader is
enabling display it is a much harder problem :-(

BR,
-R


>
> I'm about to head on vacation for a week so I'm not sure I'll get to
> re-post before then.  If not I'll post this sometime after I get back
> unless someone beats me to it.
>
> -Doug
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
Douglas Anderson March 1, 2019, 7:21 p.m. UTC | #4
Hi,

On Fri, Feb 15, 2019 at 2:37 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Thu, Feb 14, 2019 at 7:40 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Feb 14, 2019 at 1:32 PM Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > Hi Doug,
> > >
> > > On 2019-02-14 8:44 pm, Douglas Anderson wrote:
> > > > Right now the only way to disable the iommu bypass for the ARM SMMU is
> > > > with the kernel command line parameter 'arm-smmu.disable_bypass'.
> > > >
> > > > In general kernel command line parameters make sense for things that
> > > > someone would like to tweak without rebuilding the kernel or for very
> > > > basic communication between the bootloader and the kernel, but are
> > > > awkward for other things.  Specifically:
> > > > * Human parsing of the kernel command line can be difficult since it's
> > > >    just a big runon space separated line of text.
> > > > * If every bit of the system was configured via the kernel command
> > > >    line the kernel command line would get very large and even more
> > > >    unwieldly.
> > > > * Typically there are not easy ways in build systems to adjust the
> > > >    kernel command line for config-like options.
> > > >
> > > > Let's introduce a new config option that allows us to disable the
> > > > iommu bypass without affecting the existing default nor the existing
> > > > ability to adjust the configuration via kernel command line.
> > >
> > > I say let's just flip the default - for a while now it's been one of
> > > those "oh yeah, we should probably do that" things that gets instantly
> > > forgotten again, so some 3rd-party demand is plenty to convince me :)
> > >
> > > There are few reasons to allow unmatched stream bypass, and even fewer
> > > good ones, so I'd be happy to shift the command-line burden over to the
> > > esoteric cases at this point, and consider the config option in future
> > > if anyone from that camp pops up and screams hard enough.
> >
> > Sure, I can submit that patch if we want.  I presume I'll get lots of
> > screaming but I'm used to that.  ;-)
> >
> > ...specifically I found that when I turned on "disably bypass" on my
> > board (sdm845-cheza, which is not yet upstream) that a bunch of things
> > that used to work broke.  That's a good thing because all the things
> > that broke need to be fixed properly (by adding the IOMMUs) but
> > presumably my board is not special in relying on the old insecure
> > behavior.
>
> So one niggly bit, beyond the drivers that aren't but should be using
> iommu, is the case where bootloader lights up the display.  We
> actually still have a lot of work to do for this (in that clk and
> genpd drivers need to be aware of handover, in addition to just
> iommu)..  But I'd rather not make a hard problem harder just yet.
>
> (it gets worse, afaict so far on the windows-arm laptops since in that
> case uefi/edk is actually taking the iommu out of bypass and things go
> badly when arm-smmu disables clks after probe..)
>
> But I might recommend making the default a kconfig option for now, so
> in the distro kernel case, where display driver is a kernel module,
> you aren't making a hard problem harder.  For cases where bootloader
> isn't enabling display, things are much easier, and I think we could
> switch the default sooner.  But I fear for cases where bootloader is
> enabling display it is a much harder problem :-(

OK, so after thinking about this and playing with it a bit, here's
what I'm planning to do: I'm going to send out a v2 of my patch where
I basically just flip it to "default y".

Originally I was going to post a patch like Robin suggested that just
changed the default in the code without introducing a KConfig option.
Then I looked at all the things I needed to do on my own board to get
things working again once the IOMMU disallowed bypass and I just
couldn't bring myself to respond to everyone else I was about to break
"now figure out how to add a new kernel command line option until you
fix this more correctly".

In my mind a change that by default breaks all these insecure people
(effectively notifying them that they were insecure) but that allows
them to get back to the old state quickly seems like a good first
step.  In my commit message I'll mention that in a kernel version or
two we should probably fully take out the KConfig option since people
will have (presumably) had a chance to wean themselves off it.

One last thought on all of this is the question about the whole
"device tree as a stable ABI".  Presumably we're are supposed to make
some attempt to be able to run old device trees and presumably those
old device trees will break because the proper way to hook up the
IOMMU is by specifying it in the device tree.  Thus we shouldn't be
too quick to totally break all those old devices and we should make
sure that there is some easy path if someone comes up with an old
device tree that is "unfixable".  ;-)

Robin: hopefully it's OK that I'm co-opting a bit of your wording and
adding you as a "Suggested-by".  ;-)

-Doug
diff mbox series

Patch

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 46fcd75d4364..c614beab08f8 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -359,6 +359,28 @@  config ARM_SMMU
 	  Say Y here if your SoC includes an IOMMU device implementing
 	  the ARM SMMU architecture.
 
+config ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT
+	bool "Default to disabling bypass on ARM SMMU v1 and v2"
+	depends on ARM_SMMU
+	default n
+	help
+	  Say Y here to (by default) disable bypass streams such that
+	  incoming transactions from devices that are not attached to
+	  an iommu domain will report an abort back to the device and
+	  will not be allowed to pass through the SMMU.
+
+	  Historically the ARM SMMU v1 and v2 driver has defaulted
+	  to allow bypass by default but it could be disabled with
+	  the parameter 'arm-smmu.disable_bypass'.  The parameter is
+	  still present and can be used to override this config
+	  option, but this config option allows you to disable bypass
+	  without bloating the kernel command line.
+
+	  Disabling bypass is more secure but presumably will break
+	  old systems.
+
+	  Say N if unsure.
+
 config ARM_SMMU_V3
 	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
 	depends on ARM64
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 045d93884164..930c07635956 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -110,7 +110,8 @@  static int force_stage;
 module_param(force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
-static bool disable_bypass;
+static bool disable_bypass =
+	IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT);
 module_param(disable_bypass, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_bypass,
 	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");