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 = <>;
};
- 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 = < 0x100>,
- < 0x200>,
- < 0x300>,
- < 0x400>;
-
+ #iommu-cells = <1>;
+ dma-coherent;
};
ecam0: pci@8480 {
compatible = "pci-host-ecam-generic";
device_type = "pci";
- msi-parent = <>;
msi-map = <0 0 0x1>;
+ iommu-map = <0 0 0x1>;
bus-range = <0 31>;
#size-cells = <2>;
#address-cells = <3>;
- #stream-id-cells = <1>;
u-boot,dm-pre-reloc;
dma-coherent;
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On Thu, Oct 03, 2019 at 10:42:45AM +0100, Robin Murphy wrote:
> On 03/10/2019 00:58, Kees Cook wrote:
> > On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
> > > Hi Kees,
> > >
> > > On 2019-10-02 9:46 pm, Kees Cook wrote:
> > > > As we've seen from USB and other areas, we need to always do runtime
> > > > checks for DMA operating on memory regions that might be remapped. This
> > > > consolidates the (existing!) checks and makes them on by default. A
> > > > warning will be triggered for any drivers still using DMA on the stack
> > > > (as has been seen in a few recent reports).
> > > >
> > > > Suggested-by: Laura Abbott
> > > > Signed-off-by: Kees Cook
> > > > ---
> > > >include/linux/dma-debug.h | 8
> > > >include/linux/dma-mapping.h | 8 +++-
> > > >kernel/dma/debug.c | 16
> > > >3 files changed, 7 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > > > index 4208f94d93f7..2af9765d9af7 100644
> > > > --- a/include/linux/dma-debug.h
> > > > +++ b/include/linux/dma-debug.h
> > > > @@ -18,9 +18,6 @@ struct bus_type;
> > > >extern void dma_debug_add_bus(struct bus_type *bus);
> > > > -extern void debug_dma_map_single(struct device *dev, const void *addr,
> > > > -unsigned long len);
> > > > -
> > > >extern void debug_dma_map_page(struct device *dev, struct page *page,
> > > >size_t offset, size_t size,
> > > >int direction, dma_addr_t dma_addr);
> > > > @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type
> > > > *bus)
> > > >{
> > > >}
> > > > -static inline void debug_dma_map_single(struct device *dev, const void
> > > > *addr,
> > > > - unsigned long len)
> > > > -{
> > > > -}
> > > > -
> > > >static inline void debug_dma_map_page(struct device *dev, struct
> > > > page *page,
> > > > size_t offset, size_t size,
> > > > int direction, dma_addr_t
> > > > dma_addr)
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index 4a1c4fca475a..2d6b8382eab1 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -583,7 +583,13 @@ static inline unsigned long
> > > > dma_get_merge_boundary(struct device *dev)
> > > >static inline dma_addr_t dma_map_single_attrs(struct device *dev,
> > > > void *ptr,
> > > > size_t size, enum dma_data_direction dir, unsigned long
> > > > attrs)
> > > >{
> > > > - debug_dma_map_single(dev, ptr, size);
> > > > + /* DMA must never operate on stack or other remappable places.
> > > > */
> > > > + WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> > >
> > > This stands to absolutely cripple I/O performance on arm64, because every
> > > valid call will end up going off and scanning the memblock list, which is
> > > not something we want on a fastpath in non-debug configurations. We'd
> > > need a
> > > much better solution to the "pfn_valid() vs. EFI no-map" problem before
> > > this
> > > might be viable.
> >
> > Ah! Interesting. I didn't realize this was fast-path (I don't know the
> > DMA code at all). I thought it was more of a "one time setup" before
> > actual DMA activity started.
>
> That's strictly true, it's just that many workloads can involve tens of
> thousands of "one time"s per second ;)
>
> Overhead on the dma_map_* paths has shown to have a direct impact on
> throughput in such situations, hence various optimisation effort in IOVA
> allocation for IOMMU-based DMA ops, and the recent work to remove indirect
> calls entirely for the common dma-direct/SWIOTLB cases.
>
> > Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is
> > the
> > most important part of this as far as catching stack-based DMA attempts.
> > I thought virt_addr_valid() was cheap too, but I see it's much heavier on
> > arm64.
> >
> > I just went to compare what the existing USB check does, and it happens
> > immediately before its call to dma_map_single(). Both checks are simple
> > bounds checks, so it shouldn't be an issue:
> >
> > if (is_vmalloc_addr(urb->setup_packet)) {
> > WARN_ONCE(1, "setup packet is not dma
> > capable\n");
> > return -EAGAIN;
> > } else if (object_is_on_stack(urb->setup_packet)) {
> > WARN_ONCE(1, "setup packet is on stack\n");
> > return -EAGAIN;
> > }
> >
> > urb->setup_dma = dma_map_single(
> > hcd->self.sysdev,
> > urb->setup_packet,
> >
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 3/4] iommu/io-pgtable-arm: Rationalise TCR handling
On Wed, Aug 21, 2019 at 01:56:20PM +0100, Robin Murphy wrote: > On 21/08/2019 13:11, Will Deacon wrote: > >On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote: > >>On 20/08/2019 17:07, Will Deacon wrote: > >>>On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote: > On 20/08/2019 11:31, Will Deacon wrote: > >On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote: > >>Although it's conceptually nice for the io_pgtable_cfg to provide a > >>standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU > >>looks exactly like an Arm CPU, and they all have various other TCR > >>controls which io-pgtable can't be expected to understand. Thus since > >>there is an expectation that drivers will have to add to the given TCR > >>value anyway, let's strip it down to just the essentials that are > >>directly relevant to io-pgatble's inner workings - namely the address > >>sizes, walk attributes, and where appropriate, format selection. > >> > >>Signed-off-by: Robin Murphy > >>--- > >>drivers/iommu/arm-smmu-v3.c| 7 +-- > >>drivers/iommu/arm-smmu.c | 1 + > >>drivers/iommu/arm-smmu.h | 2 ++ > >>drivers/iommu/io-pgtable-arm-v7s.c | 6 ++ > >>drivers/iommu/io-pgtable-arm.c | 4 > >>drivers/iommu/qcom_iommu.c | 2 +- > >>6 files changed, 7 insertions(+), 15 deletions(-) > > > >Hmm, so I'm a bit nervous about this one since I think we really should > >be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying > >on the driver to do this worries me. See my comments on the next patch. > > The whole idea is that we already know we can't provide a *complete* TCR > value (not least because anything above bit 31 is the wild west), thus > there's really no point in io-pgtable trying to provide anything other > than > the parts it definitely controls. It makes sense to provide this partial > TCR > value "as if" for TTBR0, since that's the most common case, but ultimately > io-pgatble doesn't know (or need to) which TTBR the caller intends to > actually use for this table. Even if the caller *is* allocating it for > TTBR0, io-pgtable doesn't know that they haven't got something live in > TTBR1 > already, so it still wouldn't be in a position to make the EPD1 call > either > way. > >>> > >>>Ok, but the driver can happily rewrite/ignore what it gets back. I suppose > >>>an alternative would be scrapped the 'u64 tcr' and instead having a bunch > >>>of named bitfields for the stuff we're actually providing, although I'd > >>>still like EPDx to be in there. > >> > >>I like the bitfield idea; it would certainly emphasise the "you have to do > >>something more with this" angle that I'm pushing towards here, but still > >>leave things framed in TCR terms without having to go to some more general > >>abstraction. It really doesn't play into your EPD argument though - such a > >>config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. > >>For a driver to understand that and do the right thing with it is even more > >>involved than for the driver to just set EPD1 by itself anyway. > > > >Having considered the bitfield idea some more, I'm less attached to EPDx > >because we simply wouldn't be making a statement about them, rather than a > >(dangerous) zero value and expecting it to be ignored. So I think we're in > >agreement on that. > > Cool, I'll give bitfields a go for v2. > > >The only part I'm still stuck to is that I think io-pgtable should know > >whether it's targetting TTBR0 or TTBR1 so that it can sanitise input > >addresses correctly. Doing this in the driver code is possible, but I'd > >rather not start from that position, particularly as it would require things > >like sign-extension in the TLBI callbacks. Bumping this as is our tradition in the -rc1 time frame before we get all distracted with other stuff. It sounds like the last agreement was for a TTBR1 hint for the EDP and the sign extension in the functions. Let me know if you need any help. I've got a little time and more than a little motivation to keep slogging ahead toward a glorious arm-smmu-v2 per-context pagetable future. Jordan -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/ipmmu-vmsa: Only call platform_get_irq() when interrupt is mandatory
Quoting Geert Uytterhoeven (2019-10-01 11:06:22)
> As platform_get_irq() now prints an error when the interrupt does not
> exist, calling it gratuitously causes scary messages like:
>
> ipmmu-vmsa e674.mmu: IRQ index 0 not found
>
> Fix this by moving the call to platform_get_irq() down, where the
> existence of the interrupt is mandatory.
>
> Fixes: 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to
> platform_get_irq*()")
> Signed-off-by: Geert Uytterhoeven
> ---
Reviewed-by: Stephen Boyd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/6] ACPI / utils: add new helper for HID/UID match
On Thu, Oct 03, 2019 at 12:00:29PM +0200, Ulf Hansson wrote: > On Tue, 1 Oct 2019 at 16:27, Andy Shevchenko > wrote: > > > > There are few users outside of ACPI realm that re-introduce a custom > > solution to match ACPI device against HID/UID. Add a generic helper for > > them. > > > > The series is supposed to go via linux-pm tree. > I guess Rafael intend to pick this up for v5.5? Yes, linux-pm tree is maintained by Rafael. > In any case, for the mmc patch, feel free to add: > > Acked-by: Ulf Hansson Thanks! -- With Best Regards, Andy Shevchenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/6] ACPI / utils: add new helper for HID/UID match
On Tue, 1 Oct 2019 at 16:27, Andy Shevchenko wrote: > > There are few users outside of ACPI realm that re-introduce a custom > solution to match ACPI device against HID/UID. Add a generic helper for > them. > > The series is supposed to go via linux-pm tree. > > In v3: > - correct logic in sdhci-acpi for qcom devices (Adrian) > - add Mika's Ack > > In v2: > - add patch 2 due to latent issue in the header (lkp) > - get rid of match_hid_uid() completely in patch 6 > > Andy Shevchenko (6): > ACPI / utils: Describe function parameters in kernel-doc > ACPI / utils: Move acpi_dev_get_first_match_dev() under CONFIG_ACPI > ACPI / utils: Introduce acpi_dev_hid_uid_match() helper > ACPI / LPSS: Switch to use acpi_dev_hid_uid_match() > mmc: sdhci-acpi: Switch to use acpi_dev_hid_uid_match() > iommu/amd: Switch to use acpi_dev_hid_uid_match() > > drivers/acpi/acpi_lpss.c | 21 +++ > drivers/acpi/utils.c | 32 +++ > drivers/iommu/amd_iommu.c | 30 - > drivers/mmc/host/sdhci-acpi.c | 49 --- > include/acpi/acpi_bus.h | 8 +++--- > include/linux/acpi.h | 6 + > 6 files changed, 67 insertions(+), 79 deletions(-) > > -- > 2.23.0 > I guess Rafael intend to pick this up for v5.5? In any case, for the mmc patch, feel free to add: Acked-by: Ulf Hansson Kind regards Uffe ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On 03/10/2019 00:58, Kees Cook wrote:
On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
Hi Kees,
On 2019-10-02 9:46 pm, Kees Cook wrote:
As we've seen from USB and other areas, we need to always do runtime
checks for DMA operating on memory regions that might be remapped. This
consolidates the (existing!) checks and makes them on by default. A
warning will be triggered for any drivers still using DMA on the stack
(as has been seen in a few recent reports).
Suggested-by: Laura Abbott
Signed-off-by: Kees Cook
---
include/linux/dma-debug.h | 8
include/linux/dma-mapping.h | 8 +++-
kernel/dma/debug.c | 16
3 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 4208f94d93f7..2af9765d9af7 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -18,9 +18,6 @@ struct bus_type;
extern void dma_debug_add_bus(struct bus_type *bus);
-extern void debug_dma_map_single(struct device *dev, const void *addr,
-unsigned long len);
-
extern void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr);
@@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
{
}
-static inline void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
-}
-
static inline void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..2d6b8382eab1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct
device *dev)
static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- debug_dma_map_single(dev, ptr, size);
+ /* DMA must never operate on stack or other remappable places. */
+ WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
This stands to absolutely cripple I/O performance on arm64, because every
valid call will end up going off and scanning the memblock list, which is
not something we want on a fastpath in non-debug configurations. We'd need a
much better solution to the "pfn_valid() vs. EFI no-map" problem before this
might be viable.
Ah! Interesting. I didn't realize this was fast-path (I don't know the
DMA code at all). I thought it was more of a "one time setup" before
actual DMA activity started.
That's strictly true, it's just that many workloads can involve tens of
thousands of "one time"s per second ;)
Overhead on the dma_map_* paths has shown to have a direct impact on
throughput in such situations, hence various optimisation effort in IOVA
allocation for IOMMU-based DMA ops, and the recent work to remove
indirect calls entirely for the common dma-direct/SWIOTLB cases.
Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
most important part of this as far as catching stack-based DMA attempts.
I thought virt_addr_valid() was cheap too, but I see it's much heavier on
arm64.
I just went to compare what the existing USB check does, and it happens
immediately before its call to dma_map_single(). Both checks are simple
bounds checks, so it shouldn't be an issue:
if (is_vmalloc_addr(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is not dma
capable\n");
return -EAGAIN;
} else if (object_is_on_stack(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is on stack\n");
return -EAGAIN;
}
urb->setup_dma = dma_map_single(
hcd->self.sysdev,
urb->setup_packet,
sizeof(struct usb_ctrlrequest),
In the USB case, it'll actually refuse to do the operation. Should
dma_map_single() similarly fail? I could push these checks down into
dma_map_single(), which would be a no-change on behavior for USB and
gain the checks on all other callers...
I think it would be reasonable to pull the is_vmalloc_addr() check
inline, as that probably covers 90+% of badness (especially given
vmapped stacks), and as you say should be reliably cheap everywhere.
Callers are certainly expected to use dma_mapping_error() and handle
failure, so refusing to do a bogus mapping operation should be OK
API-wise - ultimately if a
