Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Fri, Oct 4, 2019 at 4:27 PM Robin Murphy wrote: > > On 2019-10-04 9:37 pm, Tim Harvey wrote: > > On Fri, Oct 4, 2019 at 11:34 AM Robin Murphy wrote: > >> > >> On 04/10/2019 18:13, Tim Harvey wrote: > >> [...] > > No difference... still need 'arm-smmu.disable_bypass=n' to boot. Are > > all four iommu-map props above supposed to be the same? Seems to me > > they all point to the same thing which looks wrong. > > Hmm... :/ > > Those mappings just set Stream ID == PCI RID (strictly each one should > only need to cover the bus range assigned to that bridge, but it's not > crucial) which is the same thing the driver assumes for the mmu-masters > property, so either that's wrong and never could have worked anyway - > have you tried VFIO on this platform? - or there are other devices also > mastering through the SMMU that aren't described at all. Are you able to > capture a boot log? The SMMU faults do encode information about the > offending ID, and you can typically correlate their appearance > reasonably well with endpoint drivers probing. > > >>> > >>> Robin, > >>> > >>> VFIO is enabled in the kernel but I don't know anything about how to > >>> test/use it: > >>> $ grep VFIO .config > >>> CONFIG_KVM_VFIO=y > >>> CONFIG_VFIO_IOMMU_TYPE1=y > >>> CONFIG_VFIO_VIRQFD=y > >>> CONFIG_VFIO=y > >>> # CONFIG_VFIO_NOIOMMU is not set > >>> CONFIG_VFIO_PCI=y > >>> CONFIG_VFIO_PCI_MMAP=y > >>> CONFIG_VFIO_PCI_INTX=y > >>> # CONFIG_VFIO_PLATFORM is not set > >>> # CONFIG_VFIO_MDEV is not set > >> > >> No worries - since it's a networking-focused SoC I figured there was a > >> chance you might be using DPDK or similar userspace drivers with the NIC > >> VFs, but I was just casting around for a quick and easy baseline of > >> whether the SMMU works at all (another way would be using Qemu to run a > >> VM with one or more PCI devices assigned). > >> > >>> I do have a boot console yet I'm not seeing any smmu faults at all. > >>> Perhaps I've mis-diagnosed the issue completely. To be clear when I > >>> boot with arm-smmu.disable_bypass=y the serial console appears to not > >>> accept input in userspace and with arm-smmu.disable_bypass=n I'm fine. > >>> I'm using a buildroot initramfs rootfs for simplicity. The system > >>> isn't hung as I originally expected as the LED heartbeat trigger > >>> continues blinking... I just can't get console to accept input. > >> > >> Curiouser and curiouser... I'm inclined to suspect that the interrupt > >> configuration might also be messed up, such that the SMMU is blocking > >> traffic and jammed up due to pending faults, but you're not getting the > >> IRQ delivered to find out. Does this patch help reveal anything? > >> > >> http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=29ac3648b580920692c9b417b2fc606995826517 > >> > >> (untested, but it's a direct port of the one I've used for SMMUv3 to > >> diagnose something similar) > > > > This shows: > > Yay (ish)! > > [ and the tangential challenge would be to find out what the real global > fault interrupt is, 'cause apparently it's not SPI 68... ] > > > arm-smmu 8300.smmu0: Unexpected global fault, this could be serious > > arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, > > GFSYNR1 0x0140, GFSYNR2 0x > > If that stream ID were a PCI RID, it would be 01:08.0 > > > arm-smmu 8300.smmu0: Unexpected global fault, this could be serious > > arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, > > GFSYNR1 0x0010, GFSYNR2 0x > > And this guy would be 00:02.0 > > So it seems that either the stream ID mapping is non-trivial (and > "mmu-masters" couldn't have worked), or there are secret magic endpoints > writing to memory during boot. Either way it probably needs some input > from Cavium/Marvell to get straight. In the meantime, unless just > disabling and ignoring the SMMU altogether is a viable option, I guess > we have to resign to this being one of those "non-good" reasons for > needing global bypass :( > > Robin. > > (note to self: it would probably be useful if we dump GFAR in these logs > too. These are all writes, so it's possible they could be MSI attempts > targeting the ITS rather than DMA as such) Robin, Thanks for the help here! I opened up a support issue with Marvell but have not gotten anything from them in the 2 weeks since (horrible support since Marvell acquired them) thus I'm adding in some Marvell/Cavium folk active in other areas of kernel development as a cry for help as OcteonTX iommu is completely broken in mainline and I think this is the cause of mainline stability issues I've seen since the 4.14 kernel. Marvell/Cavium devs... can you please chime in here regarding iommu configuration issues for CN80xx/CN81XX OcteonTX? Thanks, Tim Tim ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundati
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 2019-10-04 9:37 pm, Tim Harvey wrote: On Fri, Oct 4, 2019 at 11:34 AM Robin Murphy wrote: On 04/10/2019 18:13, Tim Harvey wrote: [...] No difference... still need 'arm-smmu.disable_bypass=n' to boot. Are all four iommu-map props above supposed to be the same? Seems to me they all point to the same thing which looks wrong. Hmm... :/ Those mappings just set Stream ID == PCI RID (strictly each one should only need to cover the bus range assigned to that bridge, but it's not crucial) which is the same thing the driver assumes for the mmu-masters property, so either that's wrong and never could have worked anyway - have you tried VFIO on this platform? - or there are other devices also mastering through the SMMU that aren't described at all. Are you able to capture a boot log? The SMMU faults do encode information about the offending ID, and you can typically correlate their appearance reasonably well with endpoint drivers probing. Robin, VFIO is enabled in the kernel but I don't know anything about how to test/use it: $ grep VFIO .config CONFIG_KVM_VFIO=y CONFIG_VFIO_IOMMU_TYPE1=y CONFIG_VFIO_VIRQFD=y CONFIG_VFIO=y # CONFIG_VFIO_NOIOMMU is not set CONFIG_VFIO_PCI=y CONFIG_VFIO_PCI_MMAP=y CONFIG_VFIO_PCI_INTX=y # CONFIG_VFIO_PLATFORM is not set # CONFIG_VFIO_MDEV is not set No worries - since it's a networking-focused SoC I figured there was a chance you might be using DPDK or similar userspace drivers with the NIC VFs, but I was just casting around for a quick and easy baseline of whether the SMMU works at all (another way would be using Qemu to run a VM with one or more PCI devices assigned). I do have a boot console yet I'm not seeing any smmu faults at all. Perhaps I've mis-diagnosed the issue completely. To be clear when I boot with arm-smmu.disable_bypass=y the serial console appears to not accept input in userspace and with arm-smmu.disable_bypass=n I'm fine. I'm using a buildroot initramfs rootfs for simplicity. The system isn't hung as I originally expected as the LED heartbeat trigger continues blinking... I just can't get console to accept input. Curiouser and curiouser... I'm inclined to suspect that the interrupt configuration might also be messed up, such that the SMMU is blocking traffic and jammed up due to pending faults, but you're not getting the IRQ delivered to find out. Does this patch help reveal anything? http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=29ac3648b580920692c9b417b2fc606995826517 (untested, but it's a direct port of the one I've used for SMMUv3 to diagnose something similar) This shows: Yay (ish)! [ and the tangential challenge would be to find out what the real global fault interrupt is, 'cause apparently it's not SPI 68... ] arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0140, GFSYNR2 0x If that stream ID were a PCI RID, it would be 01:08.0 arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x And this guy would be 00:02.0 So it seems that either the stream ID mapping is non-trivial (and "mmu-masters" couldn't have worked), or there are secret magic endpoints writing to memory during boot. Either way it probably needs some input from Cavium/Marvell to get straight. In the meantime, unless just disabling and ignoring the SMMU altogether is a viable option, I guess we have to resign to this being one of those "non-good" reasons for needing global bypass :( Robin. (note to self: it would probably be useful if we dump GFAR in these logs too. These are all writes, so it's possible they could be MSI attempts targeting the ITS rather than DMA as such) arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unex
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Fri, Oct 4, 2019 at 11:34 AM Robin Murphy wrote: > > On 04/10/2019 18:13, Tim Harvey wrote: > [...] > >>> No difference... still need 'arm-smmu.disable_bypass=n' to boot. Are > >>> all four iommu-map props above supposed to be the same? Seems to me > >>> they all point to the same thing which looks wrong. > >> > >> Hmm... :/ > >> > >> Those mappings just set Stream ID == PCI RID (strictly each one should > >> only need to cover the bus range assigned to that bridge, but it's not > >> crucial) which is the same thing the driver assumes for the mmu-masters > >> property, so either that's wrong and never could have worked anyway - > >> have you tried VFIO on this platform? - or there are other devices also > >> mastering through the SMMU that aren't described at all. Are you able to > >> capture a boot log? The SMMU faults do encode information about the > >> offending ID, and you can typically correlate their appearance > >> reasonably well with endpoint drivers probing. > >> > > > > Robin, > > > > VFIO is enabled in the kernel but I don't know anything about how to > > test/use it: > > $ grep VFIO .config > > CONFIG_KVM_VFIO=y > > CONFIG_VFIO_IOMMU_TYPE1=y > > CONFIG_VFIO_VIRQFD=y > > CONFIG_VFIO=y > > # CONFIG_VFIO_NOIOMMU is not set > > CONFIG_VFIO_PCI=y > > CONFIG_VFIO_PCI_MMAP=y > > CONFIG_VFIO_PCI_INTX=y > > # CONFIG_VFIO_PLATFORM is not set > > # CONFIG_VFIO_MDEV is not set > > No worries - since it's a networking-focused SoC I figured there was a > chance you might be using DPDK or similar userspace drivers with the NIC > VFs, but I was just casting around for a quick and easy baseline of > whether the SMMU works at all (another way would be using Qemu to run a > VM with one or more PCI devices assigned). > > > I do have a boot console yet I'm not seeing any smmu faults at all. > > Perhaps I've mis-diagnosed the issue completely. To be clear when I > > boot with arm-smmu.disable_bypass=y the serial console appears to not > > accept input in userspace and with arm-smmu.disable_bypass=n I'm fine. > > I'm using a buildroot initramfs rootfs for simplicity. The system > > isn't hung as I originally expected as the LED heartbeat trigger > > continues blinking... I just can't get console to accept input. > > Curiouser and curiouser... I'm inclined to suspect that the interrupt > configuration might also be messed up, such that the SMMU is blocking > traffic and jammed up due to pending faults, but you're not getting the > IRQ delivered to find out. Does this patch help reveal anything? > > http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=29ac3648b580920692c9b417b2fc606995826517 > > (untested, but it's a direct port of the one I've used for SMMUv3 to > diagnose something similar) This shows: arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0140, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x ... arm-smmu 8300.smmu0: Unexpected global fault, this could be serious arm-smmu 8300.smmu0: GFSR 0x8002, GFSYNR0 0x0002, GFSYNR1 0x0010, GFSYNR2 0x ^^^ these two repeat over and over > > That said, it's also puzzling that no other drivers are reporting DMA > errors or timeouts either - is there any chance that some device is set > running by the firmware/
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 04/10/2019 18:13, Tim Harvey wrote: [...] No difference... still need 'arm-smmu.disable_bypass=n' to boot. Are all four iommu-map props above supposed to be the same? Seems to me they all point to the same thing which looks wrong. Hmm... :/ Those mappings just set Stream ID == PCI RID (strictly each one should only need to cover the bus range assigned to that bridge, but it's not crucial) which is the same thing the driver assumes for the mmu-masters property, so either that's wrong and never could have worked anyway - have you tried VFIO on this platform? - or there are other devices also mastering through the SMMU that aren't described at all. Are you able to capture a boot log? The SMMU faults do encode information about the offending ID, and you can typically correlate their appearance reasonably well with endpoint drivers probing. Robin, VFIO is enabled in the kernel but I don't know anything about how to test/use it: $ grep VFIO .config CONFIG_KVM_VFIO=y CONFIG_VFIO_IOMMU_TYPE1=y CONFIG_VFIO_VIRQFD=y CONFIG_VFIO=y # CONFIG_VFIO_NOIOMMU is not set CONFIG_VFIO_PCI=y CONFIG_VFIO_PCI_MMAP=y CONFIG_VFIO_PCI_INTX=y # CONFIG_VFIO_PLATFORM is not set # CONFIG_VFIO_MDEV is not set No worries - since it's a networking-focused SoC I figured there was a chance you might be using DPDK or similar userspace drivers with the NIC VFs, but I was just casting around for a quick and easy baseline of whether the SMMU works at all (another way would be using Qemu to run a VM with one or more PCI devices assigned). I do have a boot console yet I'm not seeing any smmu faults at all. Perhaps I've mis-diagnosed the issue completely. To be clear when I boot with arm-smmu.disable_bypass=y the serial console appears to not accept input in userspace and with arm-smmu.disable_bypass=n I'm fine. I'm using a buildroot initramfs rootfs for simplicity. The system isn't hung as I originally expected as the LED heartbeat trigger continues blinking... I just can't get console to accept input. Curiouser and curiouser... I'm inclined to suspect that the interrupt configuration might also be messed up, such that the SMMU is blocking traffic and jammed up due to pending faults, but you're not getting the IRQ delivered to find out. Does this patch help reveal anything? http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=29ac3648b580920692c9b417b2fc606995826517 (untested, but it's a direct port of the one I've used for SMMUv3 to diagnose something similar) That said, it's also puzzling that no other drivers are reporting DMA errors or timeouts either - is there any chance that some device is set running by the firmware/bootloader and not taken over by a kernel driver? Robin.
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Fri, Oct 4, 2019 at 9:36 AM Robin Murphy wrote:
>
> On 04/10/2019 16:23, Tim Harvey wrote:
> > On Thu, Oct 3, 2019 at 3:24 PM Robin Murphy wrote:
> >>
> >> On 2019-10-03 9:51 pm, Tim Harvey wrote:
> >>> On Thu, Oct 3, 2019 at 1:42 PM Robin Murphy wrote:
>
> Hi Tim,
>
> On 2019-10-03 7:27 pm, Tim Harvey wrote:
> > On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson
> > wrote:
> >>
> >> If you're bisecting why your peripherals stopped working, it's
> >> probably this CL. Specifically if you see this in your dmesg:
> >> Unexpected global fault, this could be serious
> >> ...then it's almost certainly this CL.
> >>
> >> Running your IOMMU-enabled peripherals with the IOMMU in bypass mode
> >> is insecure and effectively disables the protection they provide.
> >> There are few reasons to allow unmatched stream bypass, and even fewer
> >> good ones.
> >>
> >> This patch starts the transition over to make it much harder to run
> >> your system insecurely. Expected steps:
> >>
> >> 1. By default disable bypass (so anyone insecure will notice) but make
> >> it easy for someone to re-enable bypass with just a KConfig
> >> change.
> >> That's this patch.
> >>
> >> 2. After people have had a little time to come to grips with the fact
> >> that they need to set their IOMMUs properly and have had time to
> >> dig into how to do this, the KConfig will be eliminated and
> >> bypass
> >> will simply be disabled. Folks who are truly upset and still
> >> haven't fixed their system can either figure out how to add
> >> 'arm-smmu.disable_bypass=n' to their command line or revert the
> >> patch in their own private kernel. Of course these folks will be
> >> less secure.
> >>
> >> Suggested-by: Robin Murphy
> >> Signed-off-by: Douglas Anderson
> >> ---
> >
> > Hi Doug / Robin,
> >
> > I ran into this breaking things on OcteonTx boards based on CN80XX
> > CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can
> > offer some advice. The IOMMU here is cavium,smmu-v2 as defined in
> > https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi
> >
> > Booting with 'arm-smmu.disable_bypass=n' does indeed work around the
> > breakage as the commit suggests.
> >
> > Any suggestions for a proper fix?
>
> Ah, you're using the old "mmu-masters" binding (and in a way which isn't
> well-defined - it's never been specified what the stream ID argument(s)
> would mean for a PCI host bridge, and Linux just ignores them). The
> ideal thing would be to update the DT to generic "iommu-map" properties
> - it's been a long time since I last played with a ThunderX, but I
> believe the SMMU stream IDs should just be the same as the ITS device
> IDs (which is how the "mmu-masters" mapping would have played out
> anyway).
>
> The arm-smmu driver support for the old binding has always relied on
> implicit bypass - there are technical reasons why we can't realistically
> support the full functionality offered to the generic bindings, but it
> would be possible to add some degree of workaround to prevent it
> interacting quite so poorly with disable_bypass, if necessary. Do you
> have deployed systems with DTs that can't be updated, but still might
> need to run new kernels?
>
> >>>
> >>> Robin,
> >>>
> >>> Thanks for the response. I don't care too much about supporting new
> >>> kernels with the current DT - I'm good with fixing this with a DT
> >>> change. Would you be able to give me an example? I would love to see
> >>> Cavium mainline an cn81xx dts/dtsi in arch/arm64/boot/dts to be used
> >>> as a base as the only thing we have to go off of currently is the
> >>> Cavium SDK which has fairly old kernel support.
> >>
> >> No promises (it's a late-night hack from my sofa), but try giving this a
> >> go...
> >>
> >> Robin.
> >>
> >> ->8-
> >> diff --git a/cn81xx-linux.dtsi b/cn81xx-linux.dtsi
> >> index 3b759d9575fe..dabc9047c674 100644
> >> --- a/cn81xx-linux.dtsi
> >> +++ b/cn81xx-linux.dtsi
> >> @@ -234,7 +234,7 @@
> >> clocks = <&sclk>;
> >> };
> >>
> >> - smmu0@8300 {
> >> + smmu: smmu0@8300 {
> >> compatible = "cavium,smmu-v2";
> >> reg = <0x8300 0x0 0x0 0x200>;
> >> #global-interrupts = <1>;
> >> @@ -249,23 +249,18 @@
> >> <0 69 4>, <0 69 4>, <0 69 4>, <0 69
> >> 4>, <0 69 4>, <0 69 4>,
> >> <0 69 4>, <0 69 4>, <0 69 4>, <0 69
> >> 4>, <0 69 4>, <0 69 4>,
> >> <0 69 4>, <0 69 4>
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 04/10/2019 16:23, Tim Harvey wrote:
On Thu, Oct 3, 2019 at 3:24 PM Robin Murphy wrote:
On 2019-10-03 9:51 pm, Tim Harvey wrote:
On Thu, Oct 3, 2019 at 1:42 PM Robin Murphy wrote:
Hi Tim,
On 2019-10-03 7:27 pm, Tim Harvey wrote:
On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson wrote:
If you're bisecting why your peripherals stopped working, it's
probably this CL. Specifically if you see this in your dmesg:
Unexpected global fault, this could be serious
...then it's almost certainly this CL.
Running your IOMMU-enabled peripherals with the IOMMU in bypass mode
is insecure and effectively disables the protection they provide.
There are few reasons to allow unmatched stream bypass, and even fewer
good ones.
This patch starts the transition over to make it much harder to run
your system insecurely. Expected steps:
1. By default disable bypass (so anyone insecure will notice) but make
it easy for someone to re-enable bypass with just a KConfig change.
That's this patch.
2. After people have had a little time to come to grips with the fact
that they need to set their IOMMUs properly and have had time to
dig into how to do this, the KConfig will be eliminated and bypass
will simply be disabled. Folks who are truly upset and still
haven't fixed their system can either figure out how to add
'arm-smmu.disable_bypass=n' to their command line or revert the
patch in their own private kernel. Of course these folks will be
less secure.
Suggested-by: Robin Murphy
Signed-off-by: Douglas Anderson
---
Hi Doug / Robin,
I ran into this breaking things on OcteonTx boards based on CN80XX
CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can
offer some advice. The IOMMU here is cavium,smmu-v2 as defined in
https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi
Booting with 'arm-smmu.disable_bypass=n' does indeed work around the
breakage as the commit suggests.
Any suggestions for a proper fix?
Ah, you're using the old "mmu-masters" binding (and in a way which isn't
well-defined - it's never been specified what the stream ID argument(s)
would mean for a PCI host bridge, and Linux just ignores them). The
ideal thing would be to update the DT to generic "iommu-map" properties
- it's been a long time since I last played with a ThunderX, but I
believe the SMMU stream IDs should just be the same as the ITS device
IDs (which is how the "mmu-masters" mapping would have played out anyway).
The arm-smmu driver support for the old binding has always relied on
implicit bypass - there are technical reasons why we can't realistically
support the full functionality offered to the generic bindings, but it
would be possible to add some degree of workaround to prevent it
interacting quite so poorly with disable_bypass, if necessary. Do you
have deployed systems with DTs that can't be updated, but still might
need to run new kernels?
Robin,
Thanks for the response. I don't care too much about supporting new
kernels with the current DT - I'm good with fixing this with a DT
change. Would you be able to give me an example? I would love to see
Cavium mainline an cn81xx dts/dtsi in arch/arm64/boot/dts to be used
as a base as the only thing we have to go off of currently is the
Cavium SDK which has fairly old kernel support.
No promises (it's a late-night hack from my sofa), but try giving this a
go...
Robin.
->8-
diff --git a/cn81xx-linux.dtsi b/cn81xx-linux.dtsi
index 3b759d9575fe..dabc9047c674 100644
--- a/cn81xx-linux.dtsi
+++ b/cn81xx-linux.dtsi
@@ -234,7 +234,7 @@
clocks = <&sclk>;
};
- smmu0@8300 {
+ smmu: smmu0@8300 {
compatible = "cavium,smmu-v2";
reg = <0x8300 0x0 0x0 0x200>;
#global-interrupts = <1>;
@@ -249,23 +249,18 @@
<0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>, <0 69
4>, <0 69 4>,
<0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>, <0 69
4>, <0 69 4>,
<0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>, <0
69 4>;
-
- mmu-masters = <&ecam0 0x100>,
- <&pem0 0x200>,
- <&pem1 0x300>,
- <&pem2 0x400>;
-
+ #iommu-cells = <1>;
+ dma-coherent;
};
ecam0: pci@8480 {
compatible = "pci-host-ecam-generic";
device_type = "pci";
- msi-parent = <&its>;
msi-map = <0 &its 0 0x1>;
+ iommu-map = <0 &smmu 0 0x1>;
bus-range = <0 31>;
#size-cells = <2>;
#addre
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Thu, Oct 3, 2019 at 3:24 PM Robin Murphy wrote:
>
> On 2019-10-03 9:51 pm, Tim Harvey wrote:
> > On Thu, Oct 3, 2019 at 1:42 PM Robin Murphy wrote:
> >>
> >> Hi Tim,
> >>
> >> On 2019-10-03 7:27 pm, Tim Harvey wrote:
> >>> On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson
> >>> wrote:
>
> If you're bisecting why your peripherals stopped working, it's
> probably this CL. Specifically if you see this in your dmesg:
> Unexpected global fault, this could be serious
> ...then it's almost certainly this CL.
>
> Running your IOMMU-enabled peripherals with the IOMMU in bypass mode
> is insecure and effectively disables the protection they provide.
> There are few reasons to allow unmatched stream bypass, and even fewer
> good ones.
>
> This patch starts the transition over to make it much harder to run
> your system insecurely. Expected steps:
>
> 1. By default disable bypass (so anyone insecure will notice) but make
> it easy for someone to re-enable bypass with just a KConfig change.
> That's this patch.
>
> 2. After people have had a little time to come to grips with the fact
> that they need to set their IOMMUs properly and have had time to
> dig into how to do this, the KConfig will be eliminated and bypass
> will simply be disabled. Folks who are truly upset and still
> haven't fixed their system can either figure out how to add
> 'arm-smmu.disable_bypass=n' to their command line or revert the
> patch in their own private kernel. Of course these folks will be
> less secure.
>
> Suggested-by: Robin Murphy
> Signed-off-by: Douglas Anderson
> ---
> >>>
> >>> Hi Doug / Robin,
> >>>
> >>> I ran into this breaking things on OcteonTx boards based on CN80XX
> >>> CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can
> >>> offer some advice. The IOMMU here is cavium,smmu-v2 as defined in
> >>> https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi
> >>>
> >>> Booting with 'arm-smmu.disable_bypass=n' does indeed work around the
> >>> breakage as the commit suggests.
> >>>
> >>> Any suggestions for a proper fix?
> >>
> >> Ah, you're using the old "mmu-masters" binding (and in a way which isn't
> >> well-defined - it's never been specified what the stream ID argument(s)
> >> would mean for a PCI host bridge, and Linux just ignores them). The
> >> ideal thing would be to update the DT to generic "iommu-map" properties
> >> - it's been a long time since I last played with a ThunderX, but I
> >> believe the SMMU stream IDs should just be the same as the ITS device
> >> IDs (which is how the "mmu-masters" mapping would have played out anyway).
> >>
> >> The arm-smmu driver support for the old binding has always relied on
> >> implicit bypass - there are technical reasons why we can't realistically
> >> support the full functionality offered to the generic bindings, but it
> >> would be possible to add some degree of workaround to prevent it
> >> interacting quite so poorly with disable_bypass, if necessary. Do you
> >> have deployed systems with DTs that can't be updated, but still might
> >> need to run new kernels?
> >>
> >
> > Robin,
> >
> > Thanks for the response. I don't care too much about supporting new
> > kernels with the current DT - I'm good with fixing this with a DT
> > change. Would you be able to give me an example? I would love to see
> > Cavium mainline an cn81xx dts/dtsi in arch/arm64/boot/dts to be used
> > as a base as the only thing we have to go off of currently is the
> > Cavium SDK which has fairly old kernel support.
>
> No promises (it's a late-night hack from my sofa), but try giving this a
> go...
>
> Robin.
>
> ->8-
> diff --git a/cn81xx-linux.dtsi b/cn81xx-linux.dtsi
> index 3b759d9575fe..dabc9047c674 100644
> --- a/cn81xx-linux.dtsi
> +++ b/cn81xx-linux.dtsi
> @@ -234,7 +234,7 @@
> clocks = <&sclk>;
> };
>
> - smmu0@8300 {
> + smmu: smmu0@8300 {
> compatible = "cavium,smmu-v2";
> reg = <0x8300 0x0 0x0 0x200>;
> #global-interrupts = <1>;
> @@ -249,23 +249,18 @@
> <0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>,
> <0 69 4>, <0 69 4>,
> <0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>,
> <0 69 4>, <0 69 4>,
> <0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>,
> <0 69 4>;
> -
> - mmu-masters = <&ecam0 0x100>,
> - <&pem0 0x200>,
> - <&pem1 0x300>,
> - <&pem2 0x400>;
> -
> + #iommu-cells = <1>;
> + dma-coherent;
> };
>
>
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 2019-10-03 9:51 pm, Tim Harvey wrote:
On Thu, Oct 3, 2019 at 1:42 PM Robin Murphy wrote:
Hi Tim,
On 2019-10-03 7:27 pm, Tim Harvey wrote:
On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson wrote:
If you're bisecting why your peripherals stopped working, it's
probably this CL. Specifically if you see this in your dmesg:
Unexpected global fault, this could be serious
...then it's almost certainly this CL.
Running your IOMMU-enabled peripherals with the IOMMU in bypass mode
is insecure and effectively disables the protection they provide.
There are few reasons to allow unmatched stream bypass, and even fewer
good ones.
This patch starts the transition over to make it much harder to run
your system insecurely. Expected steps:
1. By default disable bypass (so anyone insecure will notice) but make
it easy for someone to re-enable bypass with just a KConfig change.
That's this patch.
2. After people have had a little time to come to grips with the fact
that they need to set their IOMMUs properly and have had time to
dig into how to do this, the KConfig will be eliminated and bypass
will simply be disabled. Folks who are truly upset and still
haven't fixed their system can either figure out how to add
'arm-smmu.disable_bypass=n' to their command line or revert the
patch in their own private kernel. Of course these folks will be
less secure.
Suggested-by: Robin Murphy
Signed-off-by: Douglas Anderson
---
Hi Doug / Robin,
I ran into this breaking things on OcteonTx boards based on CN80XX
CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can
offer some advice. The IOMMU here is cavium,smmu-v2 as defined in
https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi
Booting with 'arm-smmu.disable_bypass=n' does indeed work around the
breakage as the commit suggests.
Any suggestions for a proper fix?
Ah, you're using the old "mmu-masters" binding (and in a way which isn't
well-defined - it's never been specified what the stream ID argument(s)
would mean for a PCI host bridge, and Linux just ignores them). The
ideal thing would be to update the DT to generic "iommu-map" properties
- it's been a long time since I last played with a ThunderX, but I
believe the SMMU stream IDs should just be the same as the ITS device
IDs (which is how the "mmu-masters" mapping would have played out anyway).
The arm-smmu driver support for the old binding has always relied on
implicit bypass - there are technical reasons why we can't realistically
support the full functionality offered to the generic bindings, but it
would be possible to add some degree of workaround to prevent it
interacting quite so poorly with disable_bypass, if necessary. Do you
have deployed systems with DTs that can't be updated, but still might
need to run new kernels?
Robin,
Thanks for the response. I don't care too much about supporting new
kernels with the current DT - I'm good with fixing this with a DT
change. Would you be able to give me an example? I would love to see
Cavium mainline an cn81xx dts/dtsi in arch/arm64/boot/dts to be used
as a base as the only thing we have to go off of currently is the
Cavium SDK which has fairly old kernel support.
No promises (it's a late-night hack from my sofa), but try giving this a
go...
Robin.
->8-
diff --git a/cn81xx-linux.dtsi b/cn81xx-linux.dtsi
index 3b759d9575fe..dabc9047c674 100644
--- a/cn81xx-linux.dtsi
+++ b/cn81xx-linux.dtsi
@@ -234,7 +234,7 @@
clocks = <&sclk>;
};
- smmu0@8300 {
+ smmu: smmu0@8300 {
compatible = "cavium,smmu-v2";
reg = <0x8300 0x0 0x0 0x200>;
#global-interrupts = <1>;
@@ -249,23 +249,18 @@
<0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>, <0 69
4>, <0 69 4>,
<0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>, <0 69
4>, <0 69 4>,
<0 69 4>, <0 69 4>, <0 69 4>, <0 69 4>, <0 69
4>;
-
- mmu-masters = <&ecam0 0x100>,
- <&pem0 0x200>,
- <&pem1 0x300>,
- <&pem2 0x400>;
-
+ #iommu-cells = <1>;
+ dma-coherent;
};
ecam0: pci@8480 {
compatible = "pci-host-ecam-generic";
device_type = "pci";
- msi-parent = <&its>;
msi-map = <0 &its 0 0x1>;
+ iommu-map = <0 &smmu 0 0x1>;
bus-range = <0 31>;
#size-cells = <2>;
#address-cells = <3>;
- #stream-id-cells = <1>;
u-boot,dm-pre-reloc;
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Thu, Oct 3, 2019 at 1:42 PM Robin Murphy wrote: > > Hi Tim, > > On 2019-10-03 7:27 pm, Tim Harvey wrote: > > On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson > > wrote: > >> > >> If you're bisecting why your peripherals stopped working, it's > >> probably this CL. Specifically if you see this in your dmesg: > >>Unexpected global fault, this could be serious > >> ...then it's almost certainly this CL. > >> > >> Running your IOMMU-enabled peripherals with the IOMMU in bypass mode > >> is insecure and effectively disables the protection they provide. > >> There are few reasons to allow unmatched stream bypass, and even fewer > >> good ones. > >> > >> This patch starts the transition over to make it much harder to run > >> your system insecurely. Expected steps: > >> > >> 1. By default disable bypass (so anyone insecure will notice) but make > >> it easy for someone to re-enable bypass with just a KConfig change. > >> That's this patch. > >> > >> 2. After people have had a little time to come to grips with the fact > >> that they need to set their IOMMUs properly and have had time to > >> dig into how to do this, the KConfig will be eliminated and bypass > >> will simply be disabled. Folks who are truly upset and still > >> haven't fixed their system can either figure out how to add > >> 'arm-smmu.disable_bypass=n' to their command line or revert the > >> patch in their own private kernel. Of course these folks will be > >> less secure. > >> > >> Suggested-by: Robin Murphy > >> Signed-off-by: Douglas Anderson > >> --- > > > > Hi Doug / Robin, > > > > I ran into this breaking things on OcteonTx boards based on CN80XX > > CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can > > offer some advice. The IOMMU here is cavium,smmu-v2 as defined in > > https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi > > > > Booting with 'arm-smmu.disable_bypass=n' does indeed work around the > > breakage as the commit suggests. > > > > Any suggestions for a proper fix? > > Ah, you're using the old "mmu-masters" binding (and in a way which isn't > well-defined - it's never been specified what the stream ID argument(s) > would mean for a PCI host bridge, and Linux just ignores them). The > ideal thing would be to update the DT to generic "iommu-map" properties > - it's been a long time since I last played with a ThunderX, but I > believe the SMMU stream IDs should just be the same as the ITS device > IDs (which is how the "mmu-masters" mapping would have played out anyway). > > The arm-smmu driver support for the old binding has always relied on > implicit bypass - there are technical reasons why we can't realistically > support the full functionality offered to the generic bindings, but it > would be possible to add some degree of workaround to prevent it > interacting quite so poorly with disable_bypass, if necessary. Do you > have deployed systems with DTs that can't be updated, but still might > need to run new kernels? > Robin, Thanks for the response. I don't care too much about supporting new kernels with the current DT - I'm good with fixing this with a DT change. Would you be able to give me an example? I would love to see Cavium mainline an cn81xx dts/dtsi in arch/arm64/boot/dts to be used as a base as the only thing we have to go off of currently is the Cavium SDK which has fairly old kernel support. Thanks, Tim
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
Hi Tim, On 2019-10-03 7:27 pm, Tim Harvey wrote: On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson wrote: If you're bisecting why your peripherals stopped working, it's probably this CL. Specifically if you see this in your dmesg: Unexpected global fault, this could be serious ...then it's almost certainly this CL. Running your IOMMU-enabled peripherals with the IOMMU in bypass mode is insecure and effectively disables the protection they provide. There are few reasons to allow unmatched stream bypass, and even fewer good ones. This patch starts the transition over to make it much harder to run your system insecurely. Expected steps: 1. By default disable bypass (so anyone insecure will notice) but make it easy for someone to re-enable bypass with just a KConfig change. That's this patch. 2. After people have had a little time to come to grips with the fact that they need to set their IOMMUs properly and have had time to dig into how to do this, the KConfig will be eliminated and bypass will simply be disabled. Folks who are truly upset and still haven't fixed their system can either figure out how to add 'arm-smmu.disable_bypass=n' to their command line or revert the patch in their own private kernel. Of course these folks will be less secure. Suggested-by: Robin Murphy Signed-off-by: Douglas Anderson --- Hi Doug / Robin, I ran into this breaking things on OcteonTx boards based on CN80XX CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can offer some advice. The IOMMU here is cavium,smmu-v2 as defined in https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi Booting with 'arm-smmu.disable_bypass=n' does indeed work around the breakage as the commit suggests. Any suggestions for a proper fix? Ah, you're using the old "mmu-masters" binding (and in a way which isn't well-defined - it's never been specified what the stream ID argument(s) would mean for a PCI host bridge, and Linux just ignores them). The ideal thing would be to update the DT to generic "iommu-map" properties - it's been a long time since I last played with a ThunderX, but I believe the SMMU stream IDs should just be the same as the ITS device IDs (which is how the "mmu-masters" mapping would have played out anyway). The arm-smmu driver support for the old binding has always relied on implicit bypass - there are technical reasons why we can't realistically support the full functionality offered to the generic bindings, but it would be possible to add some degree of workaround to prevent it interacting quite so poorly with disable_bypass, if necessary. Do you have deployed systems with DTs that can't be updated, but still might need to run new kernels? Robin.
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Fri, Mar 1, 2019 at 11:21 AM Douglas Anderson wrote: > > If you're bisecting why your peripherals stopped working, it's > probably this CL. Specifically if you see this in your dmesg: > Unexpected global fault, this could be serious > ...then it's almost certainly this CL. > > Running your IOMMU-enabled peripherals with the IOMMU in bypass mode > is insecure and effectively disables the protection they provide. > There are few reasons to allow unmatched stream bypass, and even fewer > good ones. > > This patch starts the transition over to make it much harder to run > your system insecurely. Expected steps: > > 1. By default disable bypass (so anyone insecure will notice) but make >it easy for someone to re-enable bypass with just a KConfig change. >That's this patch. > > 2. After people have had a little time to come to grips with the fact >that they need to set their IOMMUs properly and have had time to >dig into how to do this, the KConfig will be eliminated and bypass >will simply be disabled. Folks who are truly upset and still >haven't fixed their system can either figure out how to add >'arm-smmu.disable_bypass=n' to their command line or revert the >patch in their own private kernel. Of course these folks will be >less secure. > > Suggested-by: Robin Murphy > Signed-off-by: Douglas Anderson > --- Hi Doug / Robin, I ran into this breaking things on OcteonTx boards based on CN80XX CPU. The IOMMU configuration is a bit beyond me and I'm hoping you can offer some advice. The IOMMU here is cavium,smmu-v2 as defined in https://github.com/Gateworks/dts-newport/blob/master/cn81xx-linux.dtsi Booting with 'arm-smmu.disable_bypass=n' does indeed work around the breakage as the commit suggests. Any suggestions for a proper fix? Best Regards, Tim
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On Fri, Mar 01, 2019 at 11:20:17AM -0800, Douglas Anderson wrote: > If you're bisecting why your peripherals stopped working, it's > probably this CL. Specifically if you see this in your dmesg: > Unexpected global fault, this could be serious > ...then it's almost certainly this CL. > > Running your IOMMU-enabled peripherals with the IOMMU in bypass mode > is insecure and effectively disables the protection they provide. > There are few reasons to allow unmatched stream bypass, and even fewer > good ones. > > This patch starts the transition over to make it much harder to run > your system insecurely. Expected steps: > > 1. By default disable bypass (so anyone insecure will notice) but make >it easy for someone to re-enable bypass with just a KConfig change. >That's this patch. > > 2. After people have had a little time to come to grips with the fact >that they need to set their IOMMUs properly and have had time to >dig into how to do this, the KConfig will be eliminated and bypass >will simply be disabled. Folks who are truly upset and still >haven't fixed their system can either figure out how to add >'arm-smmu.disable_bypass=n' to their command line or revert the >patch in their own private kernel. Of course these folks will be >less secure. > > Suggested-by: Robin Murphy > Signed-off-by: Douglas Anderson > --- > > Changes in v2: > - Flipped default to 'yes' and changed comments a lot. > > drivers/iommu/Kconfig| 25 + > drivers/iommu/arm-smmu.c | 3 ++- > 2 files changed, 27 insertions(+), 1 deletion(-) Cheers, I'll pick this one up for 5.2. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 01/03/2019 20:20, Douglas Anderson wrote: > If you're bisecting why your peripherals stopped working, it's > probably this CL. Specifically if you see this in your dmesg: > Unexpected global fault, this could be serious > ...then it's almost certainly this CL. > > Running your IOMMU-enabled peripherals with the IOMMU in bypass mode > is insecure and effectively disables the protection they provide. > There are few reasons to allow unmatched stream bypass, and even fewer > good ones. > > This patch starts the transition over to make it much harder to run > your system insecurely. Expected steps: > > 1. By default disable bypass (so anyone insecure will notice) but make >it easy for someone to re-enable bypass with just a KConfig change. >That's this patch. > > 2. After people have had a little time to come to grips with the fact >that they need to set their IOMMUs properly and have had time to >dig into how to do this, the KConfig will be eliminated and bypass >will simply be disabled. Folks who are truly upset and still >haven't fixed their system can either figure out how to add >'arm-smmu.disable_bypass=n' to their command line or revert the >patch in their own private kernel. Of course these folks will be >less secure. > > Suggested-by: Robin Murphy > Signed-off-by: Douglas Anderson > --- > > Changes in v2: > - Flipped default to 'yes' and changed comments a lot. > > drivers/iommu/Kconfig| 25 + > drivers/iommu/arm-smmu.c | 3 ++- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 1ca1fa107b21..a4210672804a 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -359,6 +359,31 @@ 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 y > + 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. > + > + Any old kernels that existed before this KConfig was > + introduced would default to _allowing_ bypass (AKA the > + equivalent of NO for this config). However the default for > + this option is YES because the old behavior is insecure. > + > + There are few reasons to allow unmatched stream bypass, and > + even fewer good ones. If saying YES here breaks your board > + you should work on fixing your board. This KConfig option > + is expected to be removed in the future and we'll simply > + hardcode the bypass disable in the code. > + > + NOTE: the kernel command line parameter > + 'arm-smmu.disable_bypass' will continue to override this > + config. > + > 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."); > OK, so my defconfig contains neither IOMMU_DEFAULT_PASSTHROUGH nor ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT. Thus my .config contains: # CONFIG_IOMMU_DEFAULT_PASSTHROUGH is not set CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y I have checked that disable_bypass is indeed set to true. And that we can override the default value on the command-line with arm-smmu.disable_bypass=0 And that PCIe and UFS still work on my board (they are "behind" the ANOC1 SMMU). Reviewed-by: Marc Gonzalez Tested-by: Marc Gonzalez Regards. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 01/03/2019 20:20, Douglas Anderson wrote: > If you're bisecting why your peripherals stopped working, it's > probably this CL. Specifically if you see this in your dmesg: > Unexpected global fault, this could be serious > ...then it's almost certainly this CL. > > Running your IOMMU-enabled peripherals with the IOMMU in bypass mode > is insecure and effectively disables the protection they provide. > There are few reasons to allow unmatched stream bypass, and even fewer > good ones. > > This patch starts the transition over to make it much harder to run > your system insecurely. Expected steps: > > 1. By default disable bypass (so anyone insecure will notice) but make >it easy for someone to re-enable bypass with just a KConfig change. >That's this patch. > > 2. After people have had a little time to come to grips with the fact >that they need to set their IOMMUs properly and have had time to >dig into how to do this, the KConfig will be eliminated and bypass >will simply be disabled. Folks who are truly upset and still >haven't fixed their system can either figure out how to add >'arm-smmu.disable_bypass=n' to their command line or revert the >patch in their own private kernel. Of course these folks will be >less secure. > > Suggested-by: Robin Murphy > Signed-off-by: Douglas Anderson > --- > > Changes in v2: > - Flipped default to 'yes' and changed comments a lot. > > drivers/iommu/Kconfig| 25 + > drivers/iommu/arm-smmu.c | 3 ++- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 1ca1fa107b21..a4210672804a 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -359,6 +359,31 @@ 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 y > + 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. > + > + Any old kernels that existed before this KConfig was > + introduced would default to _allowing_ bypass (AKA the > + equivalent of NO for this config). However the default for > + this option is YES because the old behavior is insecure. > + > + There are few reasons to allow unmatched stream bypass, and > + even fewer good ones. If saying YES here breaks your board > + you should work on fixing your board. This KConfig option > + is expected to be removed in the future and we'll simply > + hardcode the bypass disable in the code. > + > + NOTE: the kernel command line parameter > + 'arm-smmu.disable_bypass' will continue to override this > + config. > + > 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."); I'm hoping someone can clear my confusion: drivers/iommu/arm-smmu.c defines a boolean module_param: disable_bypass It is used to select the s2cr_init_val, and whether sCR0_USFCFG is set. drivers/iommu/iommu.c defines iommu_def_domain_type differently, based on CONFIG_IOMMU_DEFAULT_PASSTHROUGH How do these two similar concepts interact? (bypass vs passthrough) Regards. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 20/03/2019 15:48, Marc Gonzalez wrote: On 01/03/2019 20:20, Douglas Anderson wrote: If you're bisecting why your peripherals stopped working, it's probably this CL. Specifically if you see this in your dmesg: Unexpected global fault, this could be serious ...then it's almost certainly this CL. Running your IOMMU-enabled peripherals with the IOMMU in bypass mode is insecure and effectively disables the protection they provide. There are few reasons to allow unmatched stream bypass, and even fewer good ones. This patch starts the transition over to make it much harder to run your system insecurely. Expected steps: 1. By default disable bypass (so anyone insecure will notice) but make it easy for someone to re-enable bypass with just a KConfig change. That's this patch. 2. After people have had a little time to come to grips with the fact that they need to set their IOMMUs properly and have had time to dig into how to do this, the KConfig will be eliminated and bypass will simply be disabled. Folks who are truly upset and still haven't fixed their system can either figure out how to add 'arm-smmu.disable_bypass=n' to their command line or revert the patch in their own private kernel. Of course these folks will be less secure. Suggested-by: Robin Murphy Signed-off-by: Douglas Anderson --- Changes in v2: - Flipped default to 'yes' and changed comments a lot. drivers/iommu/Kconfig| 25 + drivers/iommu/arm-smmu.c | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 1ca1fa107b21..a4210672804a 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -359,6 +359,31 @@ 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 y + 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. + + Any old kernels that existed before this KConfig was + introduced would default to _allowing_ bypass (AKA the + equivalent of NO for this config). However the default for + this option is YES because the old behavior is insecure. + + There are few reasons to allow unmatched stream bypass, and + even fewer good ones. If saying YES here breaks your board + you should work on fixing your board. This KConfig option + is expected to be removed in the future and we'll simply + hardcode the bypass disable in the code. + + NOTE: the kernel command line parameter + 'arm-smmu.disable_bypass' will continue to override this + config. + 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."); I'm hoping someone can clear my confusion: drivers/iommu/arm-smmu.c defines a boolean module_param: disable_bypass It is used to select the s2cr_init_val, and whether sCR0_USFCFG is set. drivers/iommu/iommu.c defines iommu_def_domain_type differently, based on CONFIG_IOMMU_DEFAULT_PASSTHROUGH How do these two similar concepts interact? (bypass vs passthrough) Bypass in this case mostly refers to unmatched stream bypass, i.e. whether Stream IDs that the kernel doesn't know about are blocked or not once the SMMU is enabled. The s2cr_init_val thing is less important since default domain support got finished, as the window in which streams can be known but not assigned to anything is now pretty minimal. The only real reasons for allowing bypass are bringup situations where you'd rather focus on more important things than SMMU config
