[bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list

[bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list

[Dan Carpenter]

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

Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list

[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.

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

Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list

[Christian König]

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

Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list

[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