From: "Christian König" <christian.koenig@amd.com> To: "Charan Teja Kalla" <quic_charante@quicinc.com>, "Christian König" <ckoenig.leichtzumerken@gmail.com>, cuigaosheng <cuigaosheng1@huawei.com>, "T.J. Mercier" <tjmercier@google.com> Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, sumit.semwal@linaro.org, Dan Carpenter <dan.carpenter@oracle.com>, linux-media@vger.kernel.org Subject: Re: [Linaro-mm-sig] Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export Date: Tue, 6 Dec 2022 14:08:48 +0100 [thread overview] Message-ID: <b2736dcd-c593-e26b-5f52-638b64bc8e2b@amd.com> (raw) In-Reply-To: <866708a4-9f31-b5e5-fbda-10b2a6c948bf@quicinc.com> Am 06.12.22 um 13:55 schrieb Charan Teja Kalla: > Thanks Christian/TJ for all your inputs!! > > On 11/24/2022 6:25 PM, Christian König wrote: >>>> I was already wondering why the order is this way. >>>> >>>> Why is dma_buf_stats_setup() needing the file in the first place? >>> dmabuf->file will be used in dma_buf_stats_setup(), the >>> dma_buf_stats_setup() as follows: >>> >>>> 171 int dma_buf_stats_setup(struct dma_buf *dmabuf) >>>> 172 { >>>> 173 struct dma_buf_sysfs_entry *sysfs_entry; >>>> 174 int ret; >>>> 175 >>>> 176 if (!dmabuf || !dmabuf->file) >>>> 177 return -EINVAL; >>>> 178 >>>> 179 if (!dmabuf->exp_name) { >>>> 180 pr_err("exporter name must not be empty if stats >>>> needed\n"); >>>> 181 return -EINVAL; >>>> 182 } >>>> 183 >>>> 184 sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), >>>> GFP_KERNEL); >>>> 185 if (!sysfs_entry) >>>> 186 return -ENOMEM; >>>> 187 >>>> 188 sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset; >>>> 189 sysfs_entry->dmabuf = dmabuf; >>>> 190 >>>> 191 dmabuf->sysfs_entry = sysfs_entry; >>>> 192 >>>> 193 /* create the directory for buffer stats */ >>>> 194 ret = kobject_init_and_add(&sysfs_entry->kobj, >>>> &dma_buf_ktype, NULL, >>>> 195 "%lu", >>>> file_inode(dmabuf->file)->i_ino); >> Ah, so it uses the i_ino of the file for the sysfs unique name. >> >> I'm going to take another look how to properly clean this up. >> > How about deleting the dmabuf from the db_list directly in the error > path (which is usually done by the fput()) and then continue with the > normal fput() here. No, that's not really clean either. Give me 10 Minutes, going to come up with something. Regards, Christian. > > Just compile tested the below code and If the logic make sense for you, > will send the final tested patch. > ----------------------><--------------------------------------------- > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e6f36c0..10a1727 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -87,19 +87,28 @@ static void dma_buf_release(struct dentry *dentry) > kfree(dmabuf); > } > > -static int dma_buf_file_release(struct inode *inode, struct file *file) > +static void dma_buf_db_list_remove(struct file *file) > { > struct dma_buf *dmabuf; > > - if (!is_dma_buf_file(file)) > - return -EINVAL; > - > dmabuf = file->private_data; > + if (!dmabuf) > + return; > > mutex_lock(&db_list.lock); > list_del(&dmabuf->list_node); > mutex_unlock(&db_list.lock); > > + file->private_data = NULL; > +} > + > +static int dma_buf_file_release(struct inode *inode, struct file *file) > +{ > + if (!is_dma_buf_file(file)) > + return -EINVAL; > + > + dma_buf_db_list_remove(file); > + > return 0; > } > > @@ -688,6 +697,8 @@ struct dma_buf *dma_buf_export(const struct > dma_buf_export_info *exp_info) > * early before calling the release() dma_buf op. > */ > file->f_path.dentry->d_fsdata = NULL; > + > + dma_buf_db_list_remove(file); > fput(file); > err_dmabuf: > kfree(dmabuf); > > --------------------><----------------------------- > > >> Thanks for pointing this out, >> Christian.
WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com> To: "Charan Teja Kalla" <quic_charante@quicinc.com>, "Christian König" <ckoenig.leichtzumerken@gmail.com>, cuigaosheng <cuigaosheng1@huawei.com>, "T.J. Mercier" <tjmercier@google.com> Cc: sumit.semwal@linaro.org, Dan Carpenter <dan.carpenter@oracle.com>, Pavan Kondeti <quic_pkondeti@quicinc.com>, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org Subject: Re: [Linaro-mm-sig] Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export Date: Tue, 6 Dec 2022 14:08:48 +0100 [thread overview] Message-ID: <b2736dcd-c593-e26b-5f52-638b64bc8e2b@amd.com> (raw) In-Reply-To: <866708a4-9f31-b5e5-fbda-10b2a6c948bf@quicinc.com> Am 06.12.22 um 13:55 schrieb Charan Teja Kalla: > Thanks Christian/TJ for all your inputs!! > > On 11/24/2022 6:25 PM, Christian König wrote: >>>> I was already wondering why the order is this way. >>>> >>>> Why is dma_buf_stats_setup() needing the file in the first place? >>> dmabuf->file will be used in dma_buf_stats_setup(), the >>> dma_buf_stats_setup() as follows: >>> >>>> 171 int dma_buf_stats_setup(struct dma_buf *dmabuf) >>>> 172 { >>>> 173 struct dma_buf_sysfs_entry *sysfs_entry; >>>> 174 int ret; >>>> 175 >>>> 176 if (!dmabuf || !dmabuf->file) >>>> 177 return -EINVAL; >>>> 178 >>>> 179 if (!dmabuf->exp_name) { >>>> 180 pr_err("exporter name must not be empty if stats >>>> needed\n"); >>>> 181 return -EINVAL; >>>> 182 } >>>> 183 >>>> 184 sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), >>>> GFP_KERNEL); >>>> 185 if (!sysfs_entry) >>>> 186 return -ENOMEM; >>>> 187 >>>> 188 sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset; >>>> 189 sysfs_entry->dmabuf = dmabuf; >>>> 190 >>>> 191 dmabuf->sysfs_entry = sysfs_entry; >>>> 192 >>>> 193 /* create the directory for buffer stats */ >>>> 194 ret = kobject_init_and_add(&sysfs_entry->kobj, >>>> &dma_buf_ktype, NULL, >>>> 195 "%lu", >>>> file_inode(dmabuf->file)->i_ino); >> Ah, so it uses the i_ino of the file for the sysfs unique name. >> >> I'm going to take another look how to properly clean this up. >> > How about deleting the dmabuf from the db_list directly in the error > path (which is usually done by the fput()) and then continue with the > normal fput() here. No, that's not really clean either. Give me 10 Minutes, going to come up with something. Regards, Christian. > > Just compile tested the below code and If the logic make sense for you, > will send the final tested patch. > ----------------------><--------------------------------------------- > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e6f36c0..10a1727 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -87,19 +87,28 @@ static void dma_buf_release(struct dentry *dentry) > kfree(dmabuf); > } > > -static int dma_buf_file_release(struct inode *inode, struct file *file) > +static void dma_buf_db_list_remove(struct file *file) > { > struct dma_buf *dmabuf; > > - if (!is_dma_buf_file(file)) > - return -EINVAL; > - > dmabuf = file->private_data; > + if (!dmabuf) > + return; > > mutex_lock(&db_list.lock); > list_del(&dmabuf->list_node); > mutex_unlock(&db_list.lock); > > + file->private_data = NULL; > +} > + > +static int dma_buf_file_release(struct inode *inode, struct file *file) > +{ > + if (!is_dma_buf_file(file)) > + return -EINVAL; > + > + dma_buf_db_list_remove(file); > + > return 0; > } > > @@ -688,6 +697,8 @@ struct dma_buf *dma_buf_export(const struct > dma_buf_export_info *exp_info) > * early before calling the release() dma_buf op. > */ > file->f_path.dentry->d_fsdata = NULL; > + > + dma_buf_db_list_remove(file); > fput(file); > err_dmabuf: > kfree(dmabuf); > > --------------------><----------------------------- > > >> Thanks for pointing this out, >> Christian.
next prev parent reply other threads:[~2022-12-06 13:09 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 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 [this message] 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=b2736dcd-c593-e26b-5f52-638b64bc8e2b@amd.com \ --to=christian.koenig@amd.com \ --cc=ckoenig.leichtzumerken@gmail.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.