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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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@linaro.org <mailto:srinivas.kandagatla@linaro.org>> 
> 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@codeaurora.org
>     <mailto:mak.chopra@codeaurora.org>>
>     Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org
>     <mailto:jorge.ramirez-ortiz@linaro.org>>
>     Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org
>     <mailto:srinivas.kandagatla@linaro.org>>
>     ---
>       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
>