* [PATCH 1/4] Revert "dma: debug: detect repeated DMA sync"
2024-09-10 11:48 [PATCH 0/4] dma: debug: track DMA buffer ownership with KASAN Ahmad Fatoum
@ 2024-09-10 11:48 ` Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 2/4] KASan: implement non-warning kasan_is_poisoned_shadow Ahmad Fatoum
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2024-09-10 11:48 UTC (permalink / raw)
To: barebox; +Cc: lst, Ahmad Fatoum
It turns out this check is too restrictive. The DMA API doesn't require
a 1:1 relation between mapping and sync. It's permissible to map a big
chunk and then managing it as a number of buffers that are synced
individually.
This is currently the case with the Designware MAC driver and having
this check here leads to a copious amount of false positives.
A proper check would involve keeping account of every individual cache
line and assigning it either a device or CPU owned flag in the common
code.
We'll implement this on top of KASAN in the follow-up commit, so let's
revert commit 1ad4b32702ddc3b801e8cd35441d2a9b3a41bb19 first to have
a clean slate.
Fixes: 1ad4b32702dd ("dma: debug: detect repeated DMA sync")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/dma/debug.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/dma/debug.c b/drivers/dma/debug.c
index befe837d6cd8..7bf0be1e1ff3 100644
--- a/drivers/dma/debug.c
+++ b/drivers/dma/debug.c
@@ -13,7 +13,6 @@ struct dma_debug_entry {
dma_addr_t dev_addr;
size_t size;
int direction;
- bool dev_owned;
};
static const char *dir2name[] = {
@@ -123,7 +122,6 @@ void debug_dma_map(struct device *dev, void *addr,
entry->dev_addr = dev_addr;
entry->size = size;
entry->direction = direction;
- entry->dev_owned = true;
list_add(&entry->list, &dma_mappings);
@@ -166,17 +164,9 @@ void debug_dma_sync_single_for_cpu(struct device *dev,
struct dma_debug_entry *entry;
entry = dma_debug_entry_find(dev, dma_handle, size);
- if (!entry) {
+ if (!entry)
dma_dev_warn(dev, "sync for CPU of never-mapped %s buffer 0x%llx+0x%zx!\n",
dir2name[direction], (u64)dma_handle, size);
- return;
- }
-
- if (!entry->dev_owned)
- dma_dev_warn(dev, "unexpected sync for CPU of already CPU-mapped %s buffer 0x%llx+0x%zx!\n",
- dir2name[direction], (u64)dma_handle, size);
-
- entry->dev_owned = false;
}
void debug_dma_sync_single_for_device(struct device *dev,
@@ -192,15 +182,7 @@ void debug_dma_sync_single_for_device(struct device *dev,
* corruption
*/
entry = dma_debug_entry_find(dev, dma_handle, size);
- if (!entry) {
+ if (!entry)
dma_dev_warn(dev, "Syncing for device of never-mapped %s buffer 0x%llx+0x%zx!\n",
dir2name[direction], (u64)dma_handle, size);
- return;
- }
-
- if (entry->dev_owned)
- dma_dev_warn(dev, "unexpected sync for device of already device-mapped %s buffer 0x%llx+0x%zx!\n",
- dir2name[direction], (u64)dma_handle, size);
-
- entry->dev_owned = true;
}
--
2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 2/4] KASan: implement non-warning kasan_is_poisoned_shadow
2024-09-10 11:48 [PATCH 0/4] dma: debug: track DMA buffer ownership with KASAN Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 1/4] Revert "dma: debug: detect repeated DMA sync" Ahmad Fatoum
@ 2024-09-10 11:48 ` Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 3/4] dma: debug: poison DMA buffers with KASAN while owned by device Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 4/4] dma: debug: detect repeated DMA sync using KASAN Ahmad Fatoum
3 siblings, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2024-09-10 11:48 UTC (permalink / raw)
To: barebox; +Cc: lst, Ahmad Fatoum
We export kasan_poison_shadow and kasan_unpoison_shadow for use by
allocators, but provide no API to query whether a region is poisoned
without triggering a stack trace.
This can be useful however for more unconventional "allocators" like the
DMA API debug code, which can use it to detect double poisons/unpoisons
that should not occur if the DMA API is correctly used.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
include/linux/kasan.h | 2 ++
lib/kasan/generic.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 5fa0bebb796b..7812e0fa805b 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -69,6 +69,7 @@ extern void kasan_disable_current(void);
void kasan_poison_shadow(const void *address, size_t size, u8 value);
void kasan_unpoison_shadow(const void *address, size_t size);
+int kasan_is_poisoned_shadow(const void *address, size_t size);
bool kasan_save_enable_multi_shot(void);
void kasan_restore_multi_shot(bool enabled);
@@ -77,6 +78,7 @@ void kasan_restore_multi_shot(bool enabled);
static inline void kasan_poison_shadow(const void *address, size_t size, u8 value) {}
static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
+static inline int kasan_is_poisoned_shadow(const void *address, size_t size) { return -1; }
static inline void kasan_enable_current(void) {}
static inline void kasan_disable_current(void) {}
diff --git a/lib/kasan/generic.c b/lib/kasan/generic.c
index 3709b8da9aae..2432f9274401 100644
--- a/lib/kasan/generic.c
+++ b/lib/kasan/generic.c
@@ -177,6 +177,43 @@ static __always_inline bool check_memory_region_inline(unsigned long addr,
return !kasan_report(addr, size, write, ret_ip);
}
+int kasan_is_poisoned_shadow(const void *_addr, size_t size)
+{
+ unsigned long addr = (unsigned long)_addr;
+ unsigned long ret;
+
+ if (!kasan_initialized)
+ return -1;
+
+ if (addr < kasan_shadow_start)
+ return -1;
+
+ if (addr > kasan_shadowed_end)
+ return -1;
+
+ if (unlikely(size == 0))
+ return -1;
+
+ if (unlikely(addr + size < addr))
+ return -1;
+
+ if (addr < kasan_shadow_base)
+ return -1;
+
+ ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
+ kasan_mem_to_shadow((void *)addr + size - 1) + 1);
+
+ if (unlikely(ret)) {
+ unsigned long last_byte = addr + size - 1;
+ s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
+
+ if (unlikely(ret != (unsigned long)last_shadow ||
+ ((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
+ return 1;
+ }
+ return 0;
+}
+
void kasan_init(unsigned long membase, unsigned long memsize,
unsigned long shadow_base)
{
--
2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 3/4] dma: debug: poison DMA buffers with KASAN while owned by device
2024-09-10 11:48 [PATCH 0/4] dma: debug: track DMA buffer ownership with KASAN Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 1/4] Revert "dma: debug: detect repeated DMA sync" Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 2/4] KASan: implement non-warning kasan_is_poisoned_shadow Ahmad Fatoum
@ 2024-09-10 11:48 ` Ahmad Fatoum
2024-09-10 11:48 ` [PATCH 4/4] dma: debug: detect repeated DMA sync using KASAN Ahmad Fatoum
3 siblings, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2024-09-10 11:48 UTC (permalink / raw)
To: barebox; +Cc: lst, Ahmad Fatoum
Accessing a DMA buffer while owned by device is racy: The device
may have not written any data yet and in the case it has written data,
it may be hidden by stale cache lines caused by CPU's speculation in
the meantime. Only after sync'ing the buffer to the CPU are caches, if
any, invalidated, so the CPU sees what was written by the device.
It's therefore sensible to poison access to a buffer while owned by the
device and only unpoison it after ownership has been transferred to the
CPU.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/dma/debug.c | 60 ++++++++++++++++++++++++++++++++++++--
include/linux/kasan.h | 2 +-
lib/kasan/generic_report.c | 4 +--
3 files changed, 61 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/debug.c b/drivers/dma/debug.c
index 7bf0be1e1ff3..fd279be5be02 100644
--- a/drivers/dma/debug.c
+++ b/drivers/dma/debug.c
@@ -3,14 +3,36 @@
#include <dma.h>
#include <linux/list.h>
#include <linux/printk.h>
+#include <linux/align.h>
+#include <linux/kasan.h>
#include "debug.h"
+/**
+ * DMA_KASAN_ALIGN() - Align to KASAN granule size
+ * @x: value to be aligned
+ *
+ * kasan_unpoison_shadow will poison the bytes after it when the size
+ * is not a multiple of the granule size. We thus always align the sizes
+ * here. This allows unpoisoning a smaller buffer overlapping a bigger
+ * buffer previously unpoisoned without KASAN detecting an out-of-bounds
+ * memory access.
+ *
+ * A more accurate reflection of the hardware would be to align to
+ * DMA_ALIGNMENT. We intentionally don't do this here as choosing the lowest
+ * possible alignment allows us to catch more issues that may not pop up
+ * when testing on platforms with higher DMA_ALIGNMENT.
+ *
+ * @returns the value after alignment
+ */
+#define DMA_KASAN_ALIGN(x) ALIGN(x, 1 << KASAN_SHADOW_SCALE_SHIFT)
+
static LIST_HEAD(dma_mappings);
struct dma_debug_entry {
struct list_head list;
struct device *dev;
dma_addr_t dev_addr;
+ const void *cpu_addr;
size_t size;
int direction;
};
@@ -103,6 +125,17 @@ dma_debug_entry_find(struct device *dev, dma_addr_t dev_addr, size_t size)
return NULL;
}
+static const void *dma_debug_entry_cpu_addr(struct dma_debug_entry *entry,
+ dma_addr_t dma_handle)
+{
+ ptrdiff_t offset = dma_handle - entry->dev_addr;
+
+ if (offset < 0)
+ return NULL;
+
+ return entry->cpu_addr + offset;
+}
+
void debug_dma_map(struct device *dev, void *addr,
size_t size,
int direction, dma_addr_t dev_addr)
@@ -120,6 +153,7 @@ void debug_dma_map(struct device *dev, void *addr,
entry->dev = dev;
entry->dev_addr = dev_addr;
+ entry->cpu_addr = addr;
entry->size = size;
entry->direction = direction;
@@ -130,12 +164,15 @@ void debug_dma_map(struct device *dev, void *addr,
if (!IS_ALIGNED(dev_addr, DMA_ALIGNMENT))
dma_dev_warn(dev, "Mapping insufficiently aligned %s buffer 0x%llx+0x%zx: %u bytes required!\n",
dir2name[direction], (u64)dev_addr, size, DMA_ALIGNMENT);
+
+ kasan_poison_shadow(addr, DMA_KASAN_ALIGN(size), KASAN_DMA_DEV_MAPPED);
}
void debug_dma_unmap(struct device *dev, dma_addr_t addr,
size_t size, int direction)
{
struct dma_debug_entry *entry;
+ const void *cpu_addr;
entry = dma_debug_entry_find(dev, addr, size);
if (!entry) {
@@ -153,6 +190,9 @@ void debug_dma_unmap(struct device *dev, dma_addr_t addr,
dir2name[direction]);
dma_debug(entry, "deallocating\n");
+ cpu_addr = dma_debug_entry_cpu_addr(entry, addr);
+ if (cpu_addr)
+ kasan_unpoison_shadow(cpu_addr, DMA_KASAN_ALIGN(size));
list_del(&entry->list);
free(entry);
}
@@ -162,11 +202,19 @@ void debug_dma_sync_single_for_cpu(struct device *dev,
int direction)
{
struct dma_debug_entry *entry;
+ const void *cpu_addr;
entry = dma_debug_entry_find(dev, dma_handle, size);
- if (!entry)
+ if (!entry) {
dma_dev_warn(dev, "sync for CPU of never-mapped %s buffer 0x%llx+0x%zx!\n",
dir2name[direction], (u64)dma_handle, size);
+ return;
+ }
+
+ cpu_addr = dma_debug_entry_cpu_addr(entry, dma_handle);
+ if (cpu_addr) {
+ kasan_unpoison_shadow(cpu_addr, DMA_KASAN_ALIGN(size));
+ }
}
void debug_dma_sync_single_for_device(struct device *dev,
@@ -174,6 +222,7 @@ void debug_dma_sync_single_for_device(struct device *dev,
size_t size, int direction)
{
struct dma_debug_entry *entry;
+ const void *cpu_addr;
/*
* If dma_map_single was omitted, CPU cache may contain dirty cache lines
@@ -182,7 +231,14 @@ void debug_dma_sync_single_for_device(struct device *dev,
* corruption
*/
entry = dma_debug_entry_find(dev, dma_handle, size);
- if (!entry)
+ if (!entry) {
dma_dev_warn(dev, "Syncing for device of never-mapped %s buffer 0x%llx+0x%zx!\n",
dir2name[direction], (u64)dma_handle, size);
+ return;
+ }
+
+ cpu_addr = dma_debug_entry_cpu_addr(entry, dma_handle);
+ if (cpu_addr) {
+ kasan_poison_shadow(cpu_addr, DMA_KASAN_ALIGN(size), KASAN_DMA_DEV_MAPPED);
+ }
}
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 7812e0fa805b..2dea347c40ae 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -25,7 +25,7 @@
#define KASAN_KMALLOC_FREETRACK 0xFA /* object was freed and has free track set */
#define KASAN_GLOBAL_REDZONE 0xF9 /* redzone for global variable */
-#define KASAN_VMALLOC_INVALID 0xF8 /* unallocated space in vmapped page */
+#define KASAN_DMA_DEV_MAPPED 0xF8 /* DMA-mapped buffer currently owned by device */
/*
* Stack redzone shadow values
diff --git a/lib/kasan/generic_report.c b/lib/kasan/generic_report.c
index 1cc5829e8d7f..83d55b12c070 100644
--- a/lib/kasan/generic_report.c
+++ b/lib/kasan/generic_report.c
@@ -74,8 +74,8 @@ static const char *get_shadow_bug_type(struct kasan_access_info *info)
case KASAN_ALLOCA_RIGHT:
bug_type = "alloca-out-of-bounds";
break;
- case KASAN_VMALLOC_INVALID:
- bug_type = "vmalloc-out-of-bounds";
+ case KASAN_DMA_DEV_MAPPED:
+ bug_type = "dma-mapped-to-device";
break;
}
--
2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 4/4] dma: debug: detect repeated DMA sync using KASAN
2024-09-10 11:48 [PATCH 0/4] dma: debug: track DMA buffer ownership with KASAN Ahmad Fatoum
` (2 preceding siblings ...)
2024-09-10 11:48 ` [PATCH 3/4] dma: debug: poison DMA buffers with KASAN while owned by device Ahmad Fatoum
@ 2024-09-10 11:48 ` Ahmad Fatoum
3 siblings, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2024-09-10 11:48 UTC (permalink / raw)
To: barebox; +Cc: lst, Ahmad Fatoum
Now that we poison/unpoison DMA buffers as part of ownership transfer,
it's possible for us to detect repeated sync's that are unnecessary at a
granularity of cache lines, which was not possible with the initial
approach in commit 1ad4b32702dd ("dma: debug: detect repeated DMA
sync").
Such repeated sync's are usually a result of mapping a buffer and then
calling dma_sync_single_for_device on it. This is unnecessary and warrants
a renewed look at the DMA management scheme, which this warning hopefully
accomplishes.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/dma/debug.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/debug.c b/drivers/dma/debug.c
index fd279be5be02..01a3832ea30a 100644
--- a/drivers/dma/debug.c
+++ b/drivers/dma/debug.c
@@ -213,7 +213,14 @@ void debug_dma_sync_single_for_cpu(struct device *dev,
cpu_addr = dma_debug_entry_cpu_addr(entry, dma_handle);
if (cpu_addr) {
- kasan_unpoison_shadow(cpu_addr, DMA_KASAN_ALIGN(size));
+ size_t kasan_size = DMA_KASAN_ALIGN(size);
+
+ if (kasan_is_poisoned_shadow(cpu_addr, kasan_size) == 0) {
+ dma_dev_warn(dev, "unexpected sync for CPU of already CPU-mapped %s buffer 0x%llx+0x%zx!\n",
+ dir2name[direction], (u64)dma_handle, size);
+ } else {
+ kasan_unpoison_shadow(cpu_addr, kasan_size);
+ }
}
}
@@ -239,6 +246,15 @@ void debug_dma_sync_single_for_device(struct device *dev,
cpu_addr = dma_debug_entry_cpu_addr(entry, dma_handle);
if (cpu_addr) {
- kasan_poison_shadow(cpu_addr, DMA_KASAN_ALIGN(size), KASAN_DMA_DEV_MAPPED);
+ size_t kasan_size = DMA_KASAN_ALIGN(size);
+
+ /* explicit == 1 check as function may return negative value on error */
+ if (kasan_is_poisoned_shadow(cpu_addr, kasan_size) == 1) {
+ dma_dev_warn(dev, "unexpected sync for device of already device-mapped %s buffer 0x%llx+0x%zx!\n",
+ dir2name[direction], (u64)dma_handle, size);
+ } else {
+ kasan_poison_shadow(cpu_addr, kasan_size, KASAN_DMA_DEV_MAPPED);
+ }
+
}
}
--
2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread