++ 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
