* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-02 20:46 [PATCH] dma-mapping: Lift address space checks out of debug code Kees Cook
@ 2019-10-02 21:15 ` Robin Murphy
2019-10-02 23:58 ` Kees Cook
2019-10-02 22:37 ` kbuild test robot
2019-10-03 0:05 ` kbuild test robot
2 siblings, 1 reply; 11+ messages in thread
From: Robin Murphy @ 2019-10-02 21:15 UTC (permalink / raw)
To: Kees Cook, Christoph Hellwig
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Allison Randal
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 <labbott@redhat.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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.
Robin.
> + "%s %s: driver maps %lu bytes from %s area\n",
> + dev ? dev_driver_string(dev) : "unknown driver",
> + dev ? dev_name(dev) : "unknown device", size,
> + is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
> +
> return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
> size, dir, attrs);
> }
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 099002d84f46..aa1e6a1990b2 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -1232,22 +1232,6 @@ static void check_sg_segment(struct device *dev, struct scatterlist *sg)
> #endif
> }
>
> -void debug_dma_map_single(struct device *dev, const void *addr,
> - unsigned long len)
> -{
> - if (unlikely(dma_debug_disabled()))
> - return;
> -
> - if (!virt_addr_valid(addr))
> - err_printk(dev, NULL, "device driver maps memory from invalid area [addr=%p] [len=%lu]\n",
> - addr, len);
> -
> - if (is_vmalloc_addr(addr))
> - err_printk(dev, NULL, "device driver maps memory from vmalloc area [addr=%p] [len=%lu]\n",
> - addr, len);
> -}
> -EXPORT_SYMBOL(debug_dma_map_single);
> -
> void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
> size_t size, int direction, dma_addr_t dma_addr)
> {
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-02 21:15 ` Robin Murphy
@ 2019-10-02 23:58 ` Kees Cook
2019-10-03 0:03 ` Kees Cook
2019-10-03 9:42 ` Robin Murphy
0 siblings, 2 replies; 11+ messages in thread
From: Kees Cook @ 2019-10-02 23:58 UTC (permalink / raw)
To: Robin Murphy
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Christoph Hellwig, Allison Randal
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 <labbott@redhat.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > 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.
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...
--
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-02 23:58 ` Kees Cook
@ 2019-10-03 0:03 ` Kees Cook
2019-10-03 9:42 ` Robin Murphy
1 sibling, 0 replies; 11+ messages in thread
From: Kees Cook @ 2019-10-03 0:03 UTC (permalink / raw)
To: Robin Murphy
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Christoph Hellwig, Allison Randal
On Wed, Oct 02, 2019 at 04:58:39PM -0700, Kees Cook wrote:
> 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...
Which begs the question: are all callers actually checking the result of
dma_map_single(). Many are paired with dma_mapping_error(), but lots
more aren't...
--
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-02 23:58 ` Kees Cook
2019-10-03 0:03 ` Kees Cook
@ 2019-10-03 9:42 ` Robin Murphy
2019-10-03 21:38 ` Kees Cook
1 sibling, 1 reply; 11+ messages in thread
From: Robin Murphy @ 2019-10-03 9:42 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Christoph Hellwig, Allison Randal
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 <labbott@redhat.com>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> 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 driver goes ahead and uses DMA_MAPPING_ERROR
as an address anyway, that's not likely to be any *more* catastrophic
than if it did the same with whatever nonsense virt_to_phys() of a
vmalloc address had returned.
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-03 9:42 ` Robin Murphy
@ 2019-10-03 21:38 ` Kees Cook
2019-10-04 18:50 ` Robin Murphy
2019-10-05 8:27 ` Christoph Hellwig
0 siblings, 2 replies; 11+ messages in thread
From: Kees Cook @ 2019-10-03 21:38 UTC (permalink / raw)
To: Robin Murphy
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Christoph Hellwig, Allison Randal
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 <labbott@redhat.com>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > > > 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 driver goes
> ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to
> be any *more* catastrophic than if it did the same with whatever nonsense
> virt_to_phys() of a vmalloc address had returned.
What do you think about the object_is_on_stack() check? That does a
dereference through "current" to find the stack bounds...
--
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-03 21:38 ` Kees Cook
@ 2019-10-04 18:50 ` Robin Murphy
2019-10-04 20:25 ` Kees Cook
2019-10-05 8:27 ` Christoph Hellwig
1 sibling, 1 reply; 11+ messages in thread
From: Robin Murphy @ 2019-10-04 18:50 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Christoph Hellwig, Allison Randal
On 03/10/2019 22:38, Kees Cook wrote:
> 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 <labbott@redhat.com>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>> 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 driver goes
>> ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to
>> be any *more* catastrophic than if it did the same with whatever nonsense
>> virt_to_phys() of a vmalloc address had returned.
>
> What do you think about the object_is_on_stack() check? That does a
> dereference through "current" to find the stack bounds...
I guess it depends what the aim is - is it just to bail out of
operations which have near-zero chance of working correctly and every
chance of going catastrophically wrong, or to lay down strict argument
checking for the API in general? (for cache-coherent devices, or if the
caller is careful to ensure the appropriate alignment, DMA from a
non-virtually-mapped stack can be *technically* fine, it's just banned
in general because those necessary assumptions can be tricky to meet and
aren't at all portable).
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-04 18:50 ` Robin Murphy
@ 2019-10-04 20:25 ` Kees Cook
0 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2019-10-04 20:25 UTC (permalink / raw)
To: Robin Murphy
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Christoph Hellwig, Allison Randal
On Fri, Oct 04, 2019 at 07:50:54PM +0100, Robin Murphy wrote:
> On 03/10/2019 22:38, Kees Cook wrote:
> > What do you think about the object_is_on_stack() check? That does a
> > dereference through "current" to find the stack bounds...
>
> I guess it depends what the aim is - is it just to bail out of operations
> which have near-zero chance of working correctly and every chance of going
> catastrophically wrong, or to lay down strict argument checking for the API
> in general? (for cache-coherent devices, or if the caller is careful to
> ensure the appropriate alignment, DMA from a non-virtually-mapped stack can
> be *technically* fine, it's just banned in general because those necessary
> assumptions can be tricky to meet and aren't at all portable).
Okay, then since the vmap check is both the cheapest and the most
important to catch in the face of breaking everything, I'll move that
in and we can keep USB's other checks separately.
--
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-03 21:38 ` Kees Cook
2019-10-04 18:50 ` Robin Murphy
@ 2019-10-05 8:27 ` Christoph Hellwig
1 sibling, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2019-10-05 8:27 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, linux-kernel, Stephen Boyd, iommu,
Semmle Security Reports, Dan Carpenter, Jesper Dangaard Brouer,
Thomas Gleixner, Laura Abbott, Robin Murphy, Christoph Hellwig,
Allison Randal
On Thu, Oct 03, 2019 at 02:38:43PM -0700, Kees Cook wrote:
> > 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 driver goes
> > ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to
> > be any *more* catastrophic than if it did the same with whatever nonsense
> > virt_to_phys() of a vmalloc address had returned.
>
> What do you think about the object_is_on_stack() check? That does a
> dereference through "current" to find the stack bounds...
I can be persuaded about just the vmalloc check as people tend to get
a lot of vmalloc alloctions without knowing these days. But what I'd
really like to see is a new config option that enables relatively
cheap checks without the full dma debugging infrastructure. That way
you can turn those on at least for all development builds, and can
easily benchmark having the checks vs not.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-02 20:46 [PATCH] dma-mapping: Lift address space checks out of debug code Kees Cook
2019-10-02 21:15 ` Robin Murphy
@ 2019-10-02 22:37 ` kbuild test robot
2019-10-03 0:05 ` kbuild test robot
2 siblings, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2019-10-02 22:37 UTC (permalink / raw)
To: Kees Cook
Cc: Dan Carpenter, Greg Kroah-Hartman, linux-kernel, Stephen Boyd,
iommu, Semmle Security Reports, kbuild-all,
Jesper Dangaard Brouer, Thomas Gleixner, Laura Abbott,
Robin Murphy, Christoph Hellwig, Allison Randal
[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]
Hi Kees,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/dma-mapping-Lift-address-space-checks-out-of-debug-code/20191003-060622
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:15:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from init/do_mounts.c:2:
include/linux/dma-mapping.h: In function 'dma_map_single_attrs':
>> include/linux/dma-mapping.h:588:3: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
"%s %s: driver maps %lu bytes from %s area\n",
^
include/linux/printk.h:137:10: note: in definition of macro 'no_printk'
printk(fmt, ##__VA_ARGS__); \
^~~
include/asm-generic/bug.h:196:41: note: in expansion of macro 'WARN'
#define WARN_ONCE(condition, format...) WARN(condition, format)
^~~~
>> include/linux/dma-mapping.h:587:2: note: in expansion of macro 'WARN_ONCE'
WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
^~~~~~~~~
vim +588 include/linux/dma-mapping.h
582
583 static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
584 size_t size, enum dma_data_direction dir, unsigned long attrs)
585 {
586 /* DMA must never operate on stack or other remappable places. */
> 587 WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> 588 "%s %s: driver maps %lu bytes from %s area\n",
589 dev ? dev_driver_string(dev) : "unknown driver",
590 dev ? dev_name(dev) : "unknown device", size,
591 is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
592
593 return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
594 size, dir, attrs);
595 }
596
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7205 bytes --]
[-- Attachment #3: Type: text/plain, Size: 156 bytes --]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] dma-mapping: Lift address space checks out of debug code
2019-10-02 20:46 [PATCH] dma-mapping: Lift address space checks out of debug code Kees Cook
2019-10-02 21:15 ` Robin Murphy
2019-10-02 22:37 ` kbuild test robot
@ 2019-10-03 0:05 ` kbuild test robot
2 siblings, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2019-10-03 0:05 UTC (permalink / raw)
To: Kees Cook
Cc: Dan Carpenter, Greg Kroah-Hartman, linux-kernel, Stephen Boyd,
iommu, Semmle Security Reports, kbuild-all,
Jesper Dangaard Brouer, Thomas Gleixner, Laura Abbott,
Robin Murphy, Christoph Hellwig, Allison Randal
[-- Attachment #1: Type: text/plain, Size: 3147 bytes --]
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/dma-mapping-Lift-address-space-checks-out-of-debug-code/20191003-060622
config: i386-randconfig-c004-201939 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/bug.h:83:0,
from include/linux/bug.h:5,
from include/linux/debug_locks.h:7,
from include/linux/lockdep.h:28,
from include/linux/spinlock_types.h:18,
from include/linux/mutex.h:16,
from include/linux/kernfs.h:12,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from drivers/gpu/drm/i915/i915_drv.c:30:
include/linux/dma-mapping.h: In function 'dma_map_single_attrs':
>> include/linux/dma-mapping.h:588:3: error: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Werror=format=]
"%s %s: driver maps %lu bytes from %s area\n",
^
include/asm-generic/bug.h:92:17: note: in definition of macro '__WARN_printf'
__warn_printk(arg); \
^~~
include/asm-generic/bug.h:155:3: note: in expansion of macro 'WARN'
WARN(1, format); \
^~~~
include/linux/dma-mapping.h:587:2: note: in expansion of macro 'WARN_ONCE'
WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
^~~~~~~~~
cc1: all warnings being treated as errors
vim +588 include/linux/dma-mapping.h
582
583 static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
584 size_t size, enum dma_data_direction dir, unsigned long attrs)
585 {
586 /* DMA must never operate on stack or other remappable places. */
587 WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> 588 "%s %s: driver maps %lu bytes from %s area\n",
589 dev ? dev_driver_string(dev) : "unknown driver",
590 dev ? dev_name(dev) : "unknown device", size,
591 is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
592
593 return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
594 size, dir, attrs);
595 }
596
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39725 bytes --]
[-- Attachment #3: Type: text/plain, Size: 156 bytes --]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 11+ messages in thread