From: "Christian König" <christian.koenig@amd.com> To: Charan Teja Kalla <quic_charante@quicinc.com>, Gaosheng Cui <cuigaosheng1@huawei.com>, sumit.semwal@linaro.org, tjmercier@google.com, Dan Carpenter <dan.carpenter@oracle.com>, Pavan Kondeti <quic_pkondeti@quicinc.com> Cc: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org Subject: Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export Date: Thu, 17 Nov 2022 11:16:19 +0100 [thread overview] Message-ID: <99d3aee6-ba3e-5333-6f79-ddbcfc0e8843@amd.com> (raw) In-Reply-To: <f12a5dbe-4626-f6c7-236b-30bb16be1dd6@quicinc.com> Am 17.11.22 um 08:48 schrieb Charan Teja Kalla: > Sometime back Dan also reported the same issue[1] where I do mentioned > that fput()-->dma_buf_file_release() will remove it from the list. > > But it seems that I failed to notice fput() here calls the > dma_buf_file_release() asynchronously i.e. dmabuf that is accessed in > the close path is already freed. Am I wrong here? > > Should we have the __fput_sync(file) here instead of just fput(file) > which can solve this problem? > > [1]https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220516084704.GG29930%40kadam%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C115292dd7a874278b3ed08dac8701320%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638042680960627756%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NYNIAJjt%2FSUXoc3wCz2vPvo%2Be%2FIVcABEA2JYZ8%2F2q04%3D&reserved=0 That doesn't look like the right solution to me either. Essentially we have two separate tear down methods for the dma_buf object here: 1. It's not completely initialized and we can call kfree()+module_put() to clean up. There is actually a dma_resv_fini() here. That should probably be fixed. 2. The dma_buf object is fully initialized, but creating the sysfs stats file failed. In this case we should *not* clean it up like we currently do, but rather call fput(). So the right thing to do is a) fix the missing dma_resv_fini() call and b) drop the setting d_fsdata=NULL hack and properly return after the fput(). Regards, Christian. > > Thanks, > Charan > On 11/17/2022 11:51 AM, Gaosheng Cui wrote: >> Smatch report warning as follows: >> >> drivers/dma-buf/dma-buf.c:681 dma_buf_export() warn: >> '&dmabuf->list_node' not removed from list >> >> If dma_buf_stats_setup() fails in dma_buf_export(), goto err_sysfs >> and dmabuf will be freed, but dmabuf->list_node will not be removed >> from db_list.head, then list traversal may cause UAF. >> >> Fix by removeing it from db_list.head before free(). >> >> Fixes: ef3a6b70507a ("dma-buf: call dma_buf_stats_setup after dmabuf is in valid list") >> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >> --- >> drivers/dma-buf/dma-buf.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index b809513b03fe..6848f50226d5 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -675,6 +675,9 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) >> return dmabuf; >> >> err_sysfs: >> + mutex_lock(&db_list.lock); >> + list_del(&dmabuf->list_node); >> + mutex_unlock(&db_list.lock); >> /* >> * Set file->f_path.dentry->d_fsdata to NULL so that when >> * dma_buf_release() gets invoked by dentry_ops, it exits
WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com> To: Charan Teja Kalla <quic_charante@quicinc.com>, Gaosheng Cui <cuigaosheng1@huawei.com>, sumit.semwal@linaro.org, tjmercier@google.com, Dan Carpenter <dan.carpenter@oracle.com>, Pavan Kondeti <quic_pkondeti@quicinc.com> Cc: linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org Subject: Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export Date: Thu, 17 Nov 2022 11:16:19 +0100 [thread overview] Message-ID: <99d3aee6-ba3e-5333-6f79-ddbcfc0e8843@amd.com> (raw) In-Reply-To: <f12a5dbe-4626-f6c7-236b-30bb16be1dd6@quicinc.com> Am 17.11.22 um 08:48 schrieb Charan Teja Kalla: > Sometime back Dan also reported the same issue[1] where I do mentioned > that fput()-->dma_buf_file_release() will remove it from the list. > > But it seems that I failed to notice fput() here calls the > dma_buf_file_release() asynchronously i.e. dmabuf that is accessed in > the close path is already freed. Am I wrong here? > > Should we have the __fput_sync(file) here instead of just fput(file) > which can solve this problem? > > [1]https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220516084704.GG29930%40kadam%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C115292dd7a874278b3ed08dac8701320%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638042680960627756%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NYNIAJjt%2FSUXoc3wCz2vPvo%2Be%2FIVcABEA2JYZ8%2F2q04%3D&reserved=0 That doesn't look like the right solution to me either. Essentially we have two separate tear down methods for the dma_buf object here: 1. It's not completely initialized and we can call kfree()+module_put() to clean up. There is actually a dma_resv_fini() here. That should probably be fixed. 2. The dma_buf object is fully initialized, but creating the sysfs stats file failed. In this case we should *not* clean it up like we currently do, but rather call fput(). So the right thing to do is a) fix the missing dma_resv_fini() call and b) drop the setting d_fsdata=NULL hack and properly return after the fput(). Regards, Christian. > > Thanks, > Charan > On 11/17/2022 11:51 AM, Gaosheng Cui wrote: >> Smatch report warning as follows: >> >> drivers/dma-buf/dma-buf.c:681 dma_buf_export() warn: >> '&dmabuf->list_node' not removed from list >> >> If dma_buf_stats_setup() fails in dma_buf_export(), goto err_sysfs >> and dmabuf will be freed, but dmabuf->list_node will not be removed >> from db_list.head, then list traversal may cause UAF. >> >> Fix by removeing it from db_list.head before free(). >> >> Fixes: ef3a6b70507a ("dma-buf: call dma_buf_stats_setup after dmabuf is in valid list") >> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >> --- >> drivers/dma-buf/dma-buf.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index b809513b03fe..6848f50226d5 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -675,6 +675,9 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) >> return dmabuf; >> >> err_sysfs: >> + mutex_lock(&db_list.lock); >> + list_del(&dmabuf->list_node); >> + mutex_unlock(&db_list.lock); >> /* >> * Set file->f_path.dentry->d_fsdata to NULL so that when >> * dma_buf_release() gets invoked by dentry_ops, it exits
next prev parent reply other threads:[~2022-11-17 10:16 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-11-17 6:21 [PATCH] dma-buf: Fix possible UAF in dma_buf_export Gaosheng Cui 2022-11-17 6:21 ` Gaosheng Cui 2022-11-17 7:48 ` Charan Teja Kalla 2022-11-17 7:48 ` Charan Teja Kalla 2022-11-17 10:16 ` Christian König [this message] 2022-11-17 10:16 ` Christian König 2022-11-18 2:36 ` T.J. Mercier 2022-11-18 2:36 ` T.J. Mercier 2022-11-18 8:27 ` Christian König 2022-11-18 8:27 ` Christian König 2022-11-18 17:05 ` T.J. Mercier 2022-11-19 13:30 ` Christian König 2022-11-24 5:56 ` Charan Teja Kalla 2022-11-24 5:56 ` Charan Teja Kalla 2022-11-24 11:31 ` cuigaosheng 2022-11-24 11:31 ` cuigaosheng 2022-11-24 12:05 ` cuigaosheng 2022-11-24 12:05 ` cuigaosheng 2022-11-24 12:37 ` Christian König 2022-11-24 12:37 ` Christian König 2022-11-24 12:49 ` cuigaosheng 2022-11-24 12:49 ` cuigaosheng 2022-11-24 12:55 ` [Linaro-mm-sig] " Christian König 2022-11-24 12:55 ` Christian König 2022-12-06 12:55 ` Charan Teja Kalla 2022-12-06 12:55 ` Charan Teja Kalla 2022-12-06 13:08 ` Christian König 2022-12-06 13:08 ` Christian König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=99d3aee6-ba3e-5333-6f79-ddbcfc0e8843@amd.com \ --to=christian.koenig@amd.com \ --cc=cuigaosheng1@huawei.com \ --cc=dan.carpenter@oracle.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-media@vger.kernel.org \ --cc=quic_charante@quicinc.com \ --cc=quic_pkondeti@quicinc.com \ --cc=sumit.semwal@linaro.org \ --cc=tjmercier@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.