We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything.
Just an RFC to see whether this idea has some stickiness. default y for now to make sure intel-gfx-ci picks it up too.
I'm semi-tempted to enforce this for dynamic importers since those really have no excuse at all to break the rules.
Unfortuantely we can't store the right pointers somewhere safe to make sure we oops on something recognizable, so best is to just wrangle them a bit by flipping all the bits. At least on x86 kernel addresses have all their high bits sets and the struct page array is fairly low in the kernel mapping, so flipping all the bits gives us a very high pointer in userspace and hence excellent chances for an invalid dereference.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: David Stevens stevensd@chromium.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/dma-buf/Kconfig | 8 +++++++ drivers/dma-buf/dma-buf.c | 49 +++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..cddb549e5e59 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -50,6 +50,14 @@ config DMABUF_MOVE_NOTIFY This is marked experimental because we don't yet have a consistent execution context and memory management between drivers.
+config DMABUF_DEBUG + bool "DMA-BUF debug checks" + default y + help + This option enables additional checks for DMA-BUF importers and + exporters. Specifically it validates that importers do not peek at the + underlying struct page when they import a buffer. + config DMABUF_SELFTESTS tristate "Selftests for the dma-buf interfaces" default n diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 1c9bd51db110..6e4725f7dfde 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -666,6 +666,30 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put);
+static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction direction) +{ + struct sg_table *sg_table; + + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); + +#if CONFIG_DMABUF_DEBUG + if (sg_table) { + int i; + struct scatterlist *sg; + + /* To catch abuse of the underlying struct page by importers mix + * up the bits, but take care to preserve the low SG_ bits to + * not corrupt the sgt. The mixing is undone in __unmap_dma_buf + * before passing the sgt back to the exporter. */ + for_each_sgtable_sg(sg_table, sg, i) + sg->page_link ^= ~0xffUL; + } +#endif + + return sg_table; +} + /** * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list * @dmabuf: [in] buffer to attach device to. @@ -737,7 +761,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, goto err_unlock; }
- sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); + sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL); if (!sgt) sgt = ERR_PTR(-ENOMEM); if (IS_ERR(sgt)) { @@ -784,6 +808,23 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, } EXPORT_SYMBOL_GPL(dma_buf_attach);
+static void __unmap_dma_buf(struct dma_buf_attachment *attach, + struct sg_table *sg_table, + enum dma_data_direction direction) +{ + +#if CONFIG_DMABUF_DEBUG + if (sg_table) { + int i; + struct scatterlist *sg; + + for_each_sgtable_sg(sg_table, sg, i) + sg->page_link ^= ~0xffUL; + } +#endif + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); +} + /** * dma_buf_detach - Remove the given attachment from dmabuf's attachments list * @dmabuf: [in] buffer to detach from. @@ -802,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_lock(attach->dmabuf->resv, NULL);
- dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); + __unmap_dma_buf(attach, attach->sgt, attach->dir);
if (dma_buf_is_dynamic(attach->dmabuf)) { dma_buf_unpin(attach); @@ -924,7 +965,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } }
- sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); + sg_table = __map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM);
@@ -987,7 +1028,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_assert_held(attach->dmabuf->resv);
- attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); + __unmap_dma_buf(attach, sg_table, direction);
if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
Quoting Daniel Vetter (2021-01-13 14:06:04)
We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything.
If the exporter doesn't want to expose the struct page, why are they setting it in the exported sg_table? -Chris
On Wed, Jan 13, 2021 at 4:43 PM Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Daniel Vetter (2021-01-13 14:06:04)
We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything.
If the exporter doesn't want to expose the struct page, why are they setting it in the exported sg_table?
You need to store it somewhere, otherwise the dma-api doesn't work. Essentially this achieves clearing/resetting the struct page pointer, without additional allocations somewhere, or tons of driver changes (since presumably the driver does keep track of the struct page somewhere too).
Also as long as we have random importers looking at struct page we can't just remove it, or crashes everywhere. So it has to be some debug option you can disable. -Daniel
Quoting Daniel Vetter (2021-01-13 20:50:11)
On Wed, Jan 13, 2021 at 4:43 PM Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Daniel Vetter (2021-01-13 14:06:04)
We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything.
If the exporter doesn't want to expose the struct page, why are they setting it in the exported sg_table?
You need to store it somewhere, otherwise the dma-api doesn't work. Essentially this achieves clearing/resetting the struct page pointer, without additional allocations somewhere, or tons of driver changes (since presumably the driver does keep track of the struct page somewhere too).
Only for mapping, and that's before the export -- if there's even a struct page to begin with.
Also as long as we have random importers looking at struct page we can't just remove it, or crashes everywhere. So it has to be some debug option you can disable.
Totally agreed that nothing generic can rely on pages being transported via dma-buf, and memfd is there if you do want a suitable transport. The one I don't know about is dma-buf heap, do both parties there consent to transport pages via the dma-buf? i.e. do they have special cases for import/export between heaps? -Chris
On Wed, Jan 13, 2021 at 6:06 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:
We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything.
Just an RFC to see whether this idea has some stickiness. default y for now to make sure intel-gfx-ci picks it up too.
I'm semi-tempted to enforce this for dynamic importers since those really have no excuse at all to break the rules.
Unfortuantely we can't store the right pointers somewhere safe to make sure we oops on something recognizable, so best is to just wrangle them a bit by flipping all the bits. At least on x86 kernel addresses have all their high bits sets and the struct page array is fairly low in the kernel mapping, so flipping all the bits gives us a very high pointer in userspace and hence excellent chances for an invalid dereference.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: David Stevens stevensd@chromium.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/Kconfig | 8 +++++++ drivers/dma-buf/dma-buf.c | 49 +++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..cddb549e5e59 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -50,6 +50,14 @@ config DMABUF_MOVE_NOTIFY This is marked experimental because we don't yet have a consistent execution context and memory management between drivers.
+config DMABUF_DEBUG
bool "DMA-BUF debug checks"default yhelpThis option enables additional checks for DMA-BUF importers andexporters. Specifically it validates that importers do not peek at theunderlying struct page when they import a buffer.config DMABUF_SELFTESTS tristate "Selftests for the dma-buf interfaces" default n diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 1c9bd51db110..6e4725f7dfde 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -666,6 +666,30 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put);
+static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
enum dma_data_direction direction)+{
struct sg_table *sg_table;sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);+#if CONFIG_DMABUF_DEBUG
Hey Daniel, I just noticed a build warning in a tree I pulled this patch into. You probably want to use #ifdef here, as if its not defined we see: drivers/dma-buf/dma-buf.c:813:5: warning: "CONFIG_DMABUF_DEBUG" is not defined, evaluates to 0 [-Wundef]
thanks -john
On Tue, Feb 16, 2021 at 7:30 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Jan 13, 2021 at 6:06 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:
We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything.
Just an RFC to see whether this idea has some stickiness. default y for now to make sure intel-gfx-ci picks it up too.
I'm semi-tempted to enforce this for dynamic importers since those really have no excuse at all to break the rules.
Unfortuantely we can't store the right pointers somewhere safe to make sure we oops on something recognizable, so best is to just wrangle them a bit by flipping all the bits. At least on x86 kernel addresses have all their high bits sets and the struct page array is fairly low in the kernel mapping, so flipping all the bits gives us a very high pointer in userspace and hence excellent chances for an invalid dereference.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: David Stevens stevensd@chromium.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/Kconfig | 8 +++++++ drivers/dma-buf/dma-buf.c | 49 +++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..cddb549e5e59 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -50,6 +50,14 @@ config DMABUF_MOVE_NOTIFY This is marked experimental because we don't yet have a consistent execution context and memory management between drivers.
+config DMABUF_DEBUG
bool "DMA-BUF debug checks"default yhelpThis option enables additional checks for DMA-BUF importers andexporters. Specifically it validates that importers do not peek at theunderlying struct page when they import a buffer.config DMABUF_SELFTESTS tristate "Selftests for the dma-buf interfaces" default n diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 1c9bd51db110..6e4725f7dfde 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -666,6 +666,30 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put);
+static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
enum dma_data_direction direction)+{
struct sg_table *sg_table;sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);+#if CONFIG_DMABUF_DEBUG
Hey Daniel, I just noticed a build warning in a tree I pulled this patch into. You probably want to use #ifdef here, as if its not defined we see: drivers/dma-buf/dma-buf.c:813:5: warning: "CONFIG_DMABUF_DEBUG" is not defined, evaluates to 0 [-Wundef]
Nevermind. I see its already fixed in drm-misc-next.
thanks -john
linaro-mm-sig@lists.linaro.org
-
Chris Wilson -
Daniel Vetter -
John Stultz