Re: [Intel-gfx] [PATCH v15 01/23] drm/shmem-helper: Fix UAF in error path when freeing SGT of imported GEM
On Sat, 2 Sep 2023 21:15:39 +0300 Dmitry Osipenko wrote: > On 8/28/23 14:16, Boris Brezillon wrote: > > On Sun, 27 Aug 2023 20:54:27 +0300 > > Dmitry Osipenko wrote: > > > >> Freeing drm-shmem GEM right after creating it using > >> drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf > >> and then dma-buf frees this SGT second time. > >> > >> The v3d_prime_import_sg_table() is example of a error code path where > >> dma-buf's SGT is freed by drm-shmem and then it's freed second time by > >> dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). > >> > >> Add drm-shmem GEM flag telling that this is imported SGT shall not be > >> treated as own SGT, fixing the use-after-free bug. > >> > >> Cc: sta...@vger.kernel.org > >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > >> Signed-off-by: Dmitry Osipenko > >> --- > >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- > >> include/drm/drm_gem_shmem_helper.h | 7 +++ > >> 2 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> index a783d2245599..78d9cf2355a5 100644 > >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object > >> *shmem) > >> > >>if (obj->import_attach) { > >>drm_prime_gem_destroy(obj, shmem->sgt); > >> - } else { > >> + } else if (!shmem->imported_sgt) { > >>dma_resv_lock(shmem->base.resv, NULL); > >> > >>drm_WARN_ON(obj->dev, shmem->vmap_use_count); > >> @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device > >> *dev, > >>return ERR_CAST(shmem); > >> > >>shmem->sgt = sgt; > >> + shmem->imported_sgt = true; > > > > > > I feel like adding more fields that can be used to do the is_imported() > > check is going to be even more confusing. Can we instead have > > > > /* drm_gem_shmem_prime_import_sg_table() can be called from a > > * driver specific ->import_sg_table() implementations that > > * have extra failable initialization steps. Assign > > * drm_gem_object::import_attach here (even though it's > > * assigned in drm_gem_prime_import_dev()), so we don't end up > > * with driver error paths calling drm_gem_shmem_free() with an > > * imported sg_table assigned to drm_gem_shmem_object::sgt and > > * drm_gem_object::import_attach left uninitialized. > > */ > > shmem->base.import_attach = attach; > > > > here? > > AFAICT, this is not going to work because obj->import_attach will be > released by drm_prime core by the time drm_gem_shmem_free() is invoked > and drm_gem_shmem_free() uses obj->import_attach as well. How can this happen? If something wrong happens in the driver-specific ->gem_prime_import_sg_table() implementation, drm_gem_shmem_free() will be called before ->gem_prime_import_sg_table() returns, and the attachment will only be released after that [1]. > I'll keep this > patch around unless there will be other suggestions. To me the flag is > good enough, I'll add a clarifying comment to the code in v16. I really think this is a bad idea, for the same reasons I gave in my reply to patch 2 (adding fields that need to be maintained when the state can be inferred from other fields is error prone). [1]https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L958
Re: [Intel-gfx] [PATCH v15 01/23] drm/shmem-helper: Fix UAF in error path when freeing SGT of imported GEM
On 8/28/23 14:16, Boris Brezillon wrote: > On Sun, 27 Aug 2023 20:54:27 +0300 > Dmitry Osipenko wrote: > >> Freeing drm-shmem GEM right after creating it using >> drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf >> and then dma-buf frees this SGT second time. >> >> The v3d_prime_import_sg_table() is example of a error code path where >> dma-buf's SGT is freed by drm-shmem and then it's freed second time by >> dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). >> >> Add drm-shmem GEM flag telling that this is imported SGT shall not be >> treated as own SGT, fixing the use-after-free bug. >> >> Cc: sta...@vger.kernel.org >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") >> Signed-off-by: Dmitry Osipenko >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- >> include/drm/drm_gem_shmem_helper.h | 7 +++ >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c >> b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index a783d2245599..78d9cf2355a5 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object >> *shmem) >> >> if (obj->import_attach) { >> drm_prime_gem_destroy(obj, shmem->sgt); >> -} else { >> +} else if (!shmem->imported_sgt) { >> dma_resv_lock(shmem->base.resv, NULL); >> >> drm_WARN_ON(obj->dev, shmem->vmap_use_count); >> @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device >> *dev, >> return ERR_CAST(shmem); >> >> shmem->sgt = sgt; >> +shmem->imported_sgt = true; > > > I feel like adding more fields that can be used to do the is_imported() > check is going to be even more confusing. Can we instead have > > /* drm_gem_shmem_prime_import_sg_table() can be called from a >* driver specific ->import_sg_table() implementations that >* have extra failable initialization steps. Assign >* drm_gem_object::import_attach here (even though it's >* assigned in drm_gem_prime_import_dev()), so we don't end up >* with driver error paths calling drm_gem_shmem_free() with an >* imported sg_table assigned to drm_gem_shmem_object::sgt and >* drm_gem_object::import_attach left uninitialized. >*/ > shmem->base.import_attach = attach; > > here? AFAICT, this is not going to work because obj->import_attach will be released by drm_prime core by the time drm_gem_shmem_free() is invoked and drm_gem_shmem_free() uses obj->import_attach as well. I'll keep this patch around unless there will be other suggestions. To me the flag is good enough, I'll add a clarifying comment to the code in v16. -- Best regards, Dmitry
Re: [Intel-gfx] [PATCH v15 01/23] drm/shmem-helper: Fix UAF in error path when freeing SGT of imported GEM
On Sun, 27 Aug 2023 20:54:27 +0300 Dmitry Osipenko wrote: > Freeing drm-shmem GEM right after creating it using > drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf > and then dma-buf frees this SGT second time. > > The v3d_prime_import_sg_table() is example of a error code path where > dma-buf's SGT is freed by drm-shmem and then it's freed second time by > dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). > > Add drm-shmem GEM flag telling that this is imported SGT shall not be > treated as own SGT, fixing the use-after-free bug. > > Cc: sta...@vger.kernel.org > Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- > include/drm/drm_gem_shmem_helper.h | 7 +++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > b/drivers/gpu/drm/drm_gem_shmem_helper.c > index a783d2245599..78d9cf2355a5 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object > *shmem) > > if (obj->import_attach) { > drm_prime_gem_destroy(obj, shmem->sgt); > - } else { > + } else if (!shmem->imported_sgt) { > dma_resv_lock(shmem->base.resv, NULL); > > drm_WARN_ON(obj->dev, shmem->vmap_use_count); > @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device > *dev, > return ERR_CAST(shmem); > > shmem->sgt = sgt; > + shmem->imported_sgt = true; I feel like adding more fields that can be used to do the is_imported() check is going to be even more confusing. Can we instead have /* drm_gem_shmem_prime_import_sg_table() can be called from a * driver specific ->import_sg_table() implementations that * have extra failable initialization steps. Assign * drm_gem_object::import_attach here (even though it's * assigned in drm_gem_prime_import_dev()), so we don't end up * with driver error paths calling drm_gem_shmem_free() with an * imported sg_table assigned to drm_gem_shmem_object::sgt and * drm_gem_object::import_attach left uninitialized. */ shmem->base.import_attach = attach; here? > > drm_dbg_prime(dev, "size = %zu\n", size); > > diff --git a/include/drm/drm_gem_shmem_helper.h > b/include/drm/drm_gem_shmem_helper.h > index bf0c31aa8fbe..ec70a98a8fe1 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -73,6 +73,13 @@ struct drm_gem_shmem_object { >*/ > unsigned int vmap_use_count; > > + /** > + * @imported_sgt: > + * > + * True if SG table belongs to imported dma-buf. > + */ > + bool imported_sgt : 1; > + > /** >* @pages_mark_dirty_on_put: >*
[Intel-gfx] [PATCH v15 01/23] drm/shmem-helper: Fix UAF in error path when freeing SGT of imported GEM
Freeing drm-shmem GEM right after creating it using drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf and then dma-buf frees this SGT second time. The v3d_prime_import_sg_table() is example of a error code path where dma-buf's SGT is freed by drm-shmem and then it's freed second time by dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). Add drm-shmem GEM flag telling that this is imported SGT shall not be treated as own SGT, fixing the use-after-free bug. Cc: sta...@vger.kernel.org Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Signed-off-by: Dmitry Osipenko --- drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- include/drm/drm_gem_shmem_helper.h | 7 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index a783d2245599..78d9cf2355a5 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) if (obj->import_attach) { drm_prime_gem_destroy(obj, shmem->sgt); - } else { + } else if (!shmem->imported_sgt) { dma_resv_lock(shmem->base.resv, NULL); drm_WARN_ON(obj->dev, shmem->vmap_use_count); @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, return ERR_CAST(shmem); shmem->sgt = sgt; + shmem->imported_sgt = true; drm_dbg_prime(dev, "size = %zu\n", size); diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index bf0c31aa8fbe..ec70a98a8fe1 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -73,6 +73,13 @@ struct drm_gem_shmem_object { */ unsigned int vmap_use_count; + /** +* @imported_sgt: +* +* True if SG table belongs to imported dma-buf. +*/ + bool imported_sgt : 1; + /** * @pages_mark_dirty_on_put: * -- 2.41.0