On 8/28/23 14:16, Boris Brezillon wrote: > On Sun, 27 Aug 2023 20:54:27 +0300 > Dmitry Osipenko <[email protected]> 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: [email protected] >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") >> Signed-off-by: Dmitry Osipenko <[email protected]> >> --- >> 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
