Hello Charan Teja Reddy,
The patch ef3a6b70507a: "dma-buf: call dma_buf_stats_setup after dmabuf is in valid list" from May 10, 2022, leads to the following Smatch static checker warning:
drivers/dma-buf/dma-buf.c:569 dma_buf_export() warn: '&dmabuf->list_node' not removed from list
drivers/dma-buf/dma-buf.c 538 file = dma_buf_getfile(dmabuf, exp_info->flags); 539 if (IS_ERR(file)) { 540 ret = PTR_ERR(file); 541 goto err_dmabuf; 542 } 543 544 file->f_mode |= FMODE_LSEEK; 545 dmabuf->file = file; 546 547 mutex_init(&dmabuf->lock); 548 INIT_LIST_HEAD(&dmabuf->attachments); 549 550 mutex_lock(&db_list.lock); 551 list_add(&dmabuf->list_node, &db_list.head);
Added to the list
552 mutex_unlock(&db_list.lock); 553 554 ret = dma_buf_stats_setup(dmabuf); 555 if (ret) 556 goto err_sysfs;
Goto
557 558 return dmabuf; 559 560 err_sysfs: 561 /* 562 * Set file->f_path.dentry->d_fsdata to NULL so that when 563 * dma_buf_release() gets invoked by dentry_ops, it exits 564 * early before calling the release() dma_buf op. 565 */ 566 file->f_path.dentry->d_fsdata = NULL; 567 fput(file); 568 err_dmabuf: 569 kfree(dmabuf);
dmabuf is freed, but it's still on the list so it leads to a use after free.
570 err_module: 571 module_put(exp_info->owner); 572 return ERR_PTR(ret); 573 }
regards, dan carpenter
++ Adding Christian
On 5/16/2022 12:19 PM, Dan Carpenter wrote:
Hello Charan Teja Reddy,
The patch ef3a6b70507a: "dma-buf: call dma_buf_stats_setup after dmabuf is in valid list" from May 10, 2022, leads to the following Smatch static checker warning:
drivers/dma-buf/dma-buf.c:569 dma_buf_export() warn: '&dmabuf->list_node' not removed from list
drivers/dma-buf/dma-buf.c 538 file = dma_buf_getfile(dmabuf, exp_info->flags); 539 if (IS_ERR(file)) { 540 ret = PTR_ERR(file); 541 goto err_dmabuf; 542 } 543 544 file->f_mode |= FMODE_LSEEK; 545 dmabuf->file = file; 546 547 mutex_init(&dmabuf->lock); 548 INIT_LIST_HEAD(&dmabuf->attachments); 549 550 mutex_lock(&db_list.lock); 551 list_add(&dmabuf->list_node, &db_list.head);
Added to the list
552 mutex_unlock(&db_list.lock); 553 554 ret = dma_buf_stats_setup(dmabuf); 555 if (ret) 556 goto err_sysfs;
Goto
557 558 return dmabuf; 559 560 err_sysfs: 561 /* 562 * Set file->f_path.dentry->d_fsdata to NULL so that when 563 * dma_buf_release() gets invoked by dentry_ops, it exits 564 * early before calling the release() dma_buf op. 565 */ 566 file->f_path.dentry->d_fsdata = NULL; 567 fput(file); 568 err_dmabuf: 569 kfree(dmabuf);
dmabuf is freed, but it's still on the list so it leads to a use after free.
This seems to be a false positive. On closing the file @line no:567, it ends up calling dma_buf_file_release() which does remove dmabuf from its list.
static int dma_buf_file_release(struct inode *inode, struct file *file) { ...... mutex_lock(&db_list.lock); list_del(&dmabuf->list_node); mutex_unlock(&db_list.lock); ...... }
570 err_module: 571 module_put(exp_info->owner); 572 return ERR_PTR(ret); 573 }
regards, dan carpenter
Am 16.05.22 um 09:13 schrieb Charan Teja Kalla:
++ Adding Christian
On 5/16/2022 12:19 PM, Dan Carpenter wrote:
Hello Charan Teja Reddy,
The patch ef3a6b70507a: "dma-buf: call dma_buf_stats_setup after dmabuf is in valid list" from May 10, 2022, leads to the following Smatch static checker warning:
drivers/dma-buf/dma-buf.c:569 dma_buf_export() warn: '&dmabuf->list_node' not removed from list
drivers/dma-buf/dma-buf.c 538 file = dma_buf_getfile(dmabuf, exp_info->flags); 539 if (IS_ERR(file)) { 540 ret = PTR_ERR(file); 541 goto err_dmabuf; 542 } 543 544 file->f_mode |= FMODE_LSEEK; 545 dmabuf->file = file; 546 547 mutex_init(&dmabuf->lock); 548 INIT_LIST_HEAD(&dmabuf->attachments); 549 550 mutex_lock(&db_list.lock); 551 list_add(&dmabuf->list_node, &db_list.head);
Added to the list
552 mutex_unlock(&db_list.lock); 553 554 ret = dma_buf_stats_setup(dmabuf); 555 if (ret) 556 goto err_sysfs;Goto
557 558 return dmabuf; 559 560 err_sysfs: 561 /* 562 * Set file->f_path.dentry->d_fsdata to NULL so that when 563 * dma_buf_release() gets invoked by dentry_ops, it exits 564 * early before calling the release() dma_buf op. 565 */ 566 file->f_path.dentry->d_fsdata = NULL; 567 fput(file); 568 err_dmabuf: 569 kfree(dmabuf);dmabuf is freed, but it's still on the list so it leads to a use after free.
This seems to be a false positive. On closing the file @line no:567, it ends up calling dma_buf_file_release() which does remove dmabuf from its list.
Yeah, correct as far as I can see. The checker just can't see that the fput will cleanup the list.
Regards, Christian.
static int dma_buf_file_release(struct inode *inode, struct file *file) { ...... mutex_lock(&db_list.lock); list_del(&dmabuf->list_node); mutex_unlock(&db_list.lock); ...... }
570 err_module: 571 module_put(exp_info->owner); 572 return ERR_PTR(ret); 573 }regards, dan carpenter
On Mon, May 16, 2022 at 09:18:55AM +0200, Christian König wrote:
557 558 return dmabuf; 559 560 err_sysfs: 561 /* 562 * Set file->f_path.dentry->d_fsdata to NULL so that when 563 * dma_buf_release() gets invoked by dentry_ops, it exits 564 * early before calling the release() dma_buf op. 565 */ 566 file->f_path.dentry->d_fsdata = NULL; 567 fput(file); 568 err_dmabuf: 569 kfree(dmabuf);dmabuf is freed, but it's still on the list so it leads to a use after free.
This seems to be a false positive. On closing the file @line no:567, it ends up calling dma_buf_file_release() which does remove dmabuf from its list.
Yeah, correct as far as I can see. The checker just can't see that the fput will cleanup the list.
Yep. Thanks!
I hope that that Smatch will be better at parsing the fput() by the end of the year but right now it doesn't work at all.
regards, dan carpenter
dri-devel@lists.freedesktop.org
-
Charan Teja Kalla -
Christian König -
Dan Carpenter