Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf

[Srinivas Kandagatla]

On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote:
> can you add me as a co-author to this patch please?

No problem I can do that if you feel so!

> since I spent about a day doing the analysis, sent you a fix that 
> maintained the API used by the library and explained you how to 
> reproduce the issue I think it is just fair. > the fact that the api could be be modified and the fix be done a bit
> differently- free dma buf ioctl removed- seems just a minor 
> implementation detail to me.

No, that's not true, this is a clear fastrpc design issue.

Userspace is already doing a refcount via mmap/unmap on that dmabuf fd, 
having an additional api adds another level of refcount which is totally 
redundant and is the root cause for this leak.


--srini

> also you can add my tested-by if you want
>
> TIA
>
> On Fri, 23 Aug 2019 at 12:07, Srinivas Kandagatla 
> <srinivas.kandagatla@xxxxxxxxxx <mailto:srinivas.kandagatla@xxxxxxxxxx>> 
> wrote:
>
>     dma buf refcount has to be done by the driver which is going to use
>     the fd.
>     This driver already does refcount on the dmabuf fd if its actively
>     using it
>     but also does an additional refcounting via extra ioctl.
>     This additional refcount can lead to memory leak in cases where the
>     applications fail to call the ioctl to decrement the refcount.
>
>     So remove this extra refcount in the ioctl
>
>     More info of dma buf usage at drivers/dma-buf/dma-buf.c
>
>     Reported-by: Mayank Chopra <mak.chopra@xxxxxxxxxxxxxx
>     <mailto:mak.chopra@xxxxxxxxxxxxxx>>
>     Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx
>     <mailto:jorge.ramirez-ortiz@xxxxxxxxxx>>
>     Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx
>     <mailto:srinivas.kandagatla@xxxxxxxxxx>>
>     ---
>      Âdrivers/misc/fastrpc.c | 25 -------------------------
>      Â1 file changed, 25 deletions(-)
>
>     diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>     index 38829fa74f28..eee2bb398947 100644
>     --- a/drivers/misc/fastrpc.c
>     +++ b/drivers/misc/fastrpc.c
>     @@ -1198,26 +1198,6 @@ static int fastrpc_device_open(struct inode
>     *inode, struct file *filp)
>      Â Â Â Â return 0;
>      Â}
>
>     -static int fastrpc_dmabuf_free(struct fastrpc_user *fl, char __user
>     *argp)
>     -{
>     -Â Â Â Âstruct dma_buf *buf;
>     -Â Â Â Âint info;
>     -
>     -Â Â Â Âif (copy_from_user(&info, argp, sizeof(info)))
>     -Â Â Â Â Â Â Â Âreturn -EFAULT;
>     -
>     -Â Â Â Âbuf = dma_buf_get(info);
>     -Â Â Â Âif (IS_ERR_OR_NULL(buf))
>     -Â Â Â Â Â Â Â Âreturn -EINVAL;
>     -Â Â Â Â/*
>     -Â Â Â Â * one for the last get and other for the ALLOC_DMA_BUFF ioctl
>     -Â Â Â Â */
>     -Â Â Â Âdma_buf_put(buf);
>     -Â Â Â Âdma_buf_put(buf);
>     -
>     -Â Â Â Âreturn 0;
>     -}
>     -
>      Âstatic int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char
>     __user *argp)
>      Â{
>      Â Â Â Â struct fastrpc_alloc_dma_buf bp;
>     @@ -1253,8 +1233,6 @@ static int fastrpc_dmabuf_alloc(struct
>     fastrpc_user *fl, char __user *argp)
>      Â Â Â Â Â Â Â Â return -EFAULT;
>      Â Â Â Â }
>
>     -Â Â Â Âget_dma_buf(buf->dmabuf);
>     -
>      Â Â Â Â return 0;
>      Â}
>
>     @@ -1322,9 +1300,6 @@ static long fastrpc_device_ioctl(struct file
>     *file, unsigned int cmd,
>      Â Â Â Â case FASTRPC_IOCTL_INIT_CREATE:
>      Â Â Â Â Â Â Â Â err = fastrpc_init_create_process(fl, argp);
>      Â Â Â Â Â Â Â Â break;
>     -Â Â Â Âcase FASTRPC_IOCTL_FREE_DMA_BUFF:
>     -Â Â Â Â Â Â Â Âerr = fastrpc_dmabuf_free(fl, argp);
>     -Â Â Â Â Â Â Â Âbreak;
>      Â Â Â Â case FASTRPC_IOCTL_ALLOC_DMA_BUFF:
>      Â Â Â Â Â Â Â Â err = fastrpc_dmabuf_alloc(fl, argp);
>      Â Â Â Â Â Â Â Â break;
>     -- 
>     2.21.0