Timur Tabi
2024-Feb-02 23:06 UTC
[PATCH 1/2] [v3] drm/nouveau: fix several DMA buffer leaks
Nouveau manages GSP-RM DMA buffers with nvkm_gsp_mem objects. Several of
these buffers are never dealloced. Some of them can be deallocated
right after GSP-RM is initialized, but the rest need to stay until the
driver unloads.
Also futher bullet-proof these objects by poisoning the buffer and
clearing the nvkm_gsp_mem object when it is deallocated. Poisoning
the buffer should trigger an error (or crash) from GSP-RM if it tries
to access the buffer after we've deallocated it, because we were wrong
about when it is safe to deallocate.
Finally, change the mem->size field to a size_t because that's the same
type that dma_alloc_coherent expects.
Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
GSP-RM")
Signed-off-by: Timur Tabi <ttabi at nvidia.com>
---
v3: use size_t
v2: add buffer poisoning
.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 2 +-
.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 59 ++++++++++++-------
2 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 5c6b8536e31c..3fbc57b16a05 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -9,7 +9,7 @@
#define GSP_PAGE_SIZE BIT(GSP_PAGE_SHIFT)
struct nvkm_gsp_mem {
- u32 size;
+ size_t size;
void *data;
dma_addr_t addr;
};
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index 17fc429ee50b..a9030eb83b4d 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -999,6 +999,32 @@ r535_gsp_rpc_get_gsp_static_info(struct nvkm_gsp *gsp)
return 0;
}
+static void
+nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem)
+{
+ if (mem->data) {
+ /*
+ * Poison the buffer to catch any unexpected access from
+ * GSP-RM if the buffer was prematurely freed.
+ */
+ memset(mem->data, 0xFF, mem->size);
+
+ dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data,
mem->addr);
+ memset(mem, 0, sizeof(*mem));
+ }
+}
+
+static int
+nvkm_gsp_mem_ctor(struct nvkm_gsp *gsp, size_t size, struct nvkm_gsp_mem *mem)
+{
+ mem->size = size;
+ mem->data = dma_alloc_coherent(gsp->subdev.device->dev, size,
&mem->addr, GFP_KERNEL);
+ if (WARN_ON(!mem->data))
+ return -ENOMEM;
+
+ return 0;
+}
+
static int
r535_gsp_postinit(struct nvkm_gsp *gsp)
{
@@ -1026,6 +1052,13 @@ r535_gsp_postinit(struct nvkm_gsp *gsp)
nvkm_inth_allow(&gsp->subdev.inth);
nvkm_wr32(device, 0x110004, 0x00000040);
+
+ /* Release the DMA buffers that were needed only for boot and init */
+ nvkm_gsp_mem_dtor(gsp, &gsp->boot.fw);
+ nvkm_gsp_mem_dtor(gsp, &gsp->libos);
+ nvkm_gsp_mem_dtor(gsp, &gsp->rmargs);
+ nvkm_gsp_mem_dtor(gsp, &gsp->wpr_meta);
+
return ret;
}
@@ -1760,27 +1793,6 @@ r535_gsp_msg_run_cpu_sequencer(void *priv, u32 fn, void
*repv, u32 repc)
return 0;
}
-static void
-nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem)
-{
- if (mem->data) {
- dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data,
mem->addr);
- mem->data = NULL;
- }
-}
-
-static int
-nvkm_gsp_mem_ctor(struct nvkm_gsp *gsp, u32 size, struct nvkm_gsp_mem *mem)
-{
- mem->size = size;
- mem->data = dma_alloc_coherent(gsp->subdev.device->dev, size,
&mem->addr, GFP_KERNEL);
- if (WARN_ON(!mem->data))
- return -ENOMEM;
-
- return 0;
-}
-
-
static int
r535_gsp_booter_unload(struct nvkm_gsp *gsp, u32 mbox0, u32 mbox1)
{
@@ -2378,6 +2390,11 @@ r535_gsp_dtor(struct nvkm_gsp *gsp)
mutex_destroy(&gsp->cmdq.mutex);
r535_gsp_dtor_fws(gsp);
+
+ nvkm_gsp_mem_dtor(gsp, &gsp->shm.mem);
+ nvkm_gsp_mem_dtor(gsp, &gsp->loginit);
+ nvkm_gsp_mem_dtor(gsp, &gsp->logintr);
+ nvkm_gsp_mem_dtor(gsp, &gsp->logrm);
}
int
--
2.34.1
Timur Tabi
2024-Feb-02 23:06 UTC
[PATCH 2/2] drm/nouveau: nvkm_gsp_radix3_sg() should use nvkm_gsp_mem_ctor()
Function nvkm_gsp_radix3_sg() uses nvkm_gsp_mem objects to allocate the
radix3 tables, but it unnecessarily creates those objects manually
instead of using the standard nvkm_gsp_mem_ctor() function like the
rest of the code does.
Signed-off-by: Timur Tabi <ttabi at nvidia.com>
---
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index a9030eb83b4d..d065389e3618 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -2178,20 +2178,20 @@ nvkm_gsp_radix3_dtor(struct nvkm_gsp *gsp, struct
nvkm_gsp_radix3 *rx3)
* See kgspCreateRadix3_IMPL
*/
static int
-nvkm_gsp_radix3_sg(struct nvkm_device *device, struct sg_table *sgt, u64 size,
+nvkm_gsp_radix3_sg(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size,
struct nvkm_gsp_radix3 *rx3)
{
u64 addr;
for (int i = ARRAY_SIZE(rx3->mem) - 1; i >= 0; i--) {
u64 *ptes;
- int idx;
+ size_t bufsize;
+ int ret, idx;
- rx3->mem[i].size = ALIGN((size / GSP_PAGE_SIZE) * sizeof(u64),
GSP_PAGE_SIZE);
- rx3->mem[i].data = dma_alloc_coherent(device->dev, rx3->mem[i].size,
- &rx3->mem[i].addr, GFP_KERNEL);
- if (WARN_ON(!rx3->mem[i].data))
- return -ENOMEM;
+ bufsize = ALIGN((size / GSP_PAGE_SIZE) * sizeof(u64), GSP_PAGE_SIZE);
+ ret = nvkm_gsp_mem_ctor(gsp, bufsize, &rx3->mem[i]);
+ if (ret)
+ return ret;
ptes = rx3->mem[i].data;
if (i == 2) {
@@ -2231,7 +2231,7 @@ r535_gsp_fini(struct nvkm_gsp *gsp, bool suspend)
if (ret)
return ret;
- ret = nvkm_gsp_radix3_sg(gsp->subdev.device, &gsp->sr.sgt, len,
&gsp->sr.radix3);
+ ret = nvkm_gsp_radix3_sg(gsp, &gsp->sr.sgt, len,
&gsp->sr.radix3);
if (ret)
return ret;
@@ -2439,7 +2439,7 @@ r535_gsp_oneinit(struct nvkm_gsp *gsp)
memcpy(gsp->sig.data, data, size);
/* Build radix3 page table for ELF image. */
- ret = nvkm_gsp_radix3_sg(device, &gsp->fw.mem.sgt, gsp->fw.len,
&gsp->radix3);
+ ret = nvkm_gsp_radix3_sg(gsp, &gsp->fw.mem.sgt, gsp->fw.len,
&gsp->radix3);
if (ret)
return ret;
--
2.34.1
Danilo Krummrich
2024-Feb-05 18:08 UTC
[PATCH 1/2] [v3] drm/nouveau: fix several DMA buffer leaks
On 2/3/24 00:06, Timur Tabi wrote:> Nouveau manages GSP-RM DMA buffers with nvkm_gsp_mem objects. Several of > these buffers are never dealloced. Some of them can be deallocated > right after GSP-RM is initialized, but the rest need to stay until the > driver unloads. > > Also futher bullet-proof these objects by poisoning the buffer and > clearing the nvkm_gsp_mem object when it is deallocated. Poisoning > the buffer should trigger an error (or crash) from GSP-RM if it tries > to access the buffer after we've deallocated it, because we were wrong > about when it is safe to deallocate. > > Finally, change the mem->size field to a size_t because that's the same > type that dma_alloc_coherent expects. > > Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM") > Signed-off-by: Timur Tabi <ttabi at nvidia.com>Series applied to drm-misc-fixes, thanks!> --- > v3: use size_t > v2: add buffer poisoning > > .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 2 +- > .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 59 ++++++++++++------- > 2 files changed, 39 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > index 5c6b8536e31c..3fbc57b16a05 100644 > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > @@ -9,7 +9,7 @@ > #define GSP_PAGE_SIZE BIT(GSP_PAGE_SHIFT) > > struct nvkm_gsp_mem { > - u32 size; > + size_t size; > void *data; > dma_addr_t addr; > }; > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c > index 17fc429ee50b..a9030eb83b4d 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c > @@ -999,6 +999,32 @@ r535_gsp_rpc_get_gsp_static_info(struct nvkm_gsp *gsp) > return 0; > } > > +static void > +nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem) > +{ > + if (mem->data) { > + /* > + * Poison the buffer to catch any unexpected access from > + * GSP-RM if the buffer was prematurely freed. > + */ > + memset(mem->data, 0xFF, mem->size); > + > + dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data, mem->addr); > + memset(mem, 0, sizeof(*mem)); > + } > +} > + > +static int > +nvkm_gsp_mem_ctor(struct nvkm_gsp *gsp, size_t size, struct nvkm_gsp_mem *mem) > +{ > + mem->size = size; > + mem->data = dma_alloc_coherent(gsp->subdev.device->dev, size, &mem->addr, GFP_KERNEL); > + if (WARN_ON(!mem->data)) > + return -ENOMEM; > + > + return 0; > +} > + > static int > r535_gsp_postinit(struct nvkm_gsp *gsp) > { > @@ -1026,6 +1052,13 @@ r535_gsp_postinit(struct nvkm_gsp *gsp) > > nvkm_inth_allow(&gsp->subdev.inth); > nvkm_wr32(device, 0x110004, 0x00000040); > + > + /* Release the DMA buffers that were needed only for boot and init */ > + nvkm_gsp_mem_dtor(gsp, &gsp->boot.fw); > + nvkm_gsp_mem_dtor(gsp, &gsp->libos); > + nvkm_gsp_mem_dtor(gsp, &gsp->rmargs); > + nvkm_gsp_mem_dtor(gsp, &gsp->wpr_meta); > + > return ret; > } > > @@ -1760,27 +1793,6 @@ r535_gsp_msg_run_cpu_sequencer(void *priv, u32 fn, void *repv, u32 repc) > return 0; > } > > -static void > -nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem) > -{ > - if (mem->data) { > - dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data, mem->addr); > - mem->data = NULL; > - } > -} > - > -static int > -nvkm_gsp_mem_ctor(struct nvkm_gsp *gsp, u32 size, struct nvkm_gsp_mem *mem) > -{ > - mem->size = size; > - mem->data = dma_alloc_coherent(gsp->subdev.device->dev, size, &mem->addr, GFP_KERNEL); > - if (WARN_ON(!mem->data)) > - return -ENOMEM; > - > - return 0; > -} > - > - > static int > r535_gsp_booter_unload(struct nvkm_gsp *gsp, u32 mbox0, u32 mbox1) > { > @@ -2378,6 +2390,11 @@ r535_gsp_dtor(struct nvkm_gsp *gsp) > mutex_destroy(&gsp->cmdq.mutex); > > r535_gsp_dtor_fws(gsp); > + > + nvkm_gsp_mem_dtor(gsp, &gsp->shm.mem); > + nvkm_gsp_mem_dtor(gsp, &gsp->loginit); > + nvkm_gsp_mem_dtor(gsp, &gsp->logintr); > + nvkm_gsp_mem_dtor(gsp, &gsp->logrm); > } > > int