Received-SPF: pass (google.com: domain of linux-kernel+bounces-205954-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 7 Jun 2024 14:46:39 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Ekansh Gupta <quic_ekangupt@quicinc.com>
Cc: srinivas.kandagatla@linaro.org, linux-arm-msm@vger.kernel.org, 
	gregkh@linuxfoundation.org, quic_bkumar@quicinc.com, linux-kernel@vger.kernel.org, 
	quic_chennak@quicinc.com, stable <stable@kernel.org>
Subject: Re: [PATCH v4 10/11] misc: fastrpc: Fix unsigned PD support
Message-ID: <rqnxu5ga5bauwmyrsncy5fxkrjduvhavtjqcxvy6bzb565ci35@32pvlalsuij2>
References: <20240606165939.12950-1-quic_ekangupt@quicinc.com>
 <20240606165939.12950-11-quic_ekangupt@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240606165939.12950-11-quic_ekangupt@quicinc.com>

On Thu, Jun 06, 2024 at 10:29:30PM +0530, Ekansh Gupta wrote:
> Unsigned PDs are sandboxed DSP processes used to offload computation
> workloads to the DSP. Unsigned PD have less privileges in terms of
> DSP resource access as compared to Signed PD.
> 
> Unsigned PD requires more initial memory to spawn. Also most of the
> memory request are allocated from user space. Current initial memory
> size is not sufficient and mapping request for user space allocated
> buffer is not supported. This results in failure of unsigned PD offload
> request. Add changes to fix initial memory size and user space allocated
> buffer mapping support.

You can guess my comment here.

> 
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")

And here.

> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 180 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 133 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 32f2e6f625ed..5ffb6098ac38 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -40,7 +40,7 @@
>  #define FASTRPC_INIT_HANDLE	1
>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
>  #define FASTRPC_CTXID_MASK (0xFF0)
> -#define INIT_FILELEN_MAX (2 * 1024 * 1024)
> +#define INIT_FILELEN_MAX (5 * 1024 * 1024)

So, there are two things being fixed here. One is the insufficient
memory size, another one is the mmap request. Separate commits, please.

>  #define INIT_FILE_NAMELEN_MAX (128)
>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>  
> @@ -327,6 +327,7 @@ struct fastrpc_user {
>  	int tgid;
>  	int pd;
>  	bool is_secure_dev;
> +	bool is_unsigned_pd;
>  	char *servloc_name;
>  	/* Lock for lists */
>  	spinlock_t lock;
> @@ -1488,7 +1489,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  		u32 siglen;
>  	} inbuf;
>  	u32 sc;
> -	bool unsigned_module = false;
>  
>  	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>  	if (!args)
> @@ -1500,9 +1500,9 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  	}
>  
>  	if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
> -		unsigned_module = true;
> +		fl->is_unsigned_pd = true;
>  
> -	if (is_session_rejected(fl, unsigned_module)) {
> +	if (is_session_rejected(fl, fl->is_unsigned_pd)) {
>  		err = -ECONNREFUSED;
>  		goto err;
>  	}
> @@ -1985,6 +1985,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  {
>  	struct fastrpc_buf *buf = NULL, *iter, *b;
>  	struct fastrpc_req_munmap req;
> +	struct fastrpc_map *map = NULL, *iterm, *m;
>  	struct device *dev = fl->sctx->dev;
>  	int err = 0;
>  
> @@ -2031,34 +2032,75 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  		}
>  		return err;
>  	}
> -	dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
> +	spin_lock(&fl->lock);
> +	list_for_each_entry_safe(iterm, m, &fl->maps, node) {
> +		if (iterm->raddr == req.vaddrout) {
> +			map = iterm;
> +			break;
> +		}
> +	}
> +	spin_unlock(&fl->lock);
> +	if (!map) {
> +		dev_dbg(dev, "buffer not found addr 0x%09llx, len 0x%08llx\n",
>  			req.vaddrout, req.size);
> -	return -EINVAL;
> +		return -EINVAL;
> +	}
> +
> +	err = fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
> +	if (err)
> +		dev_dbg(dev, "unmmap\tpt fd = %d, 0x%09llx error\n",  map->fd, map->raddr);

Which error? The message is useless.

> +	else
> +		fastrpc_map_put(map);

Should the fl->lock be still held here? Can the map be modified
concurrently?

> +
> +	return err;
>  }
>  
> -static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> +static int fastrpc_req_map_dsp(struct fastrpc_user *fl, u64 phys,
> +			u64 size, u32 flag, uintptr_t vaddrin, u64 *raddr)
>  {
>  	struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
> -	struct fastrpc_buf *buf = NULL;
>  	struct fastrpc_mmap_req_msg req_msg;
>  	struct fastrpc_mmap_rsp_msg rsp_msg;
>  	struct fastrpc_phy_page pages;
> -	struct fastrpc_req_mmap req;
> -	struct device *dev = fl->sctx->dev;
>  	int err;
>  	u32 sc;
>  
> -	if (copy_from_user(&req, argp, sizeof(req)))
> -		return -EFAULT;
> +	req_msg.pgid = fl->tgid;
> +	req_msg.flags = flag;
> +	req_msg.vaddr = vaddrin;
> +	req_msg.num = sizeof(pages);
>  
> -	if (req.flags != ADSP_MMAP_ADD_PAGES && req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) {
> -		dev_err(dev, "flag not supported 0x%x\n", req.flags);
> +	args[0].ptr = (u64) (uintptr_t) &req_msg;
> +	args[0].length = sizeof(req_msg);
>  
> -		return -EINVAL;
> +	pages.addr = phys;
> +	pages.size = size;
> +
> +	args[1].ptr = (u64) (uintptr_t) &pages;
> +	args[1].length = sizeof(pages);
> +	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
> +	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> +					  &args[0]);
> +
> +	if (err) {
> +		dev_err(fl->sctx->dev, "mmap error (len 0x%08llx)\n", size);
> +		return err;
>  	}
> +	*raddr = rsp_msg.vaddr;
> +
> +	return err;
> +}
> +
> +static int fastrpc_req_buf_alloc(struct fastrpc_user *fl,
> +			struct fastrpc_req_mmap req, char __user *argp)
> +{
> +	struct device *dev = fl->sctx->dev;
> +	struct fastrpc_buf *buf = NULL;
> +	u64 raddr = 0;
> +	int err;
>  
>  	if (req.vaddrin) {
> -		dev_err(dev, "adding user allocated pages is not supported\n");
> +		dev_err(dev, "adding user allocated pages is not supported for signed PD\n");

Drop, less chance of users spamming the dmesg.

>  		return -EINVAL;
>  	}
>  
> @@ -2091,36 +2133,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  		}
>  	}
>  
> -	req_msg.pgid = fl->tgid;
> -	req_msg.flags = req.flags;
> -	req_msg.vaddr = req.vaddrin;
> -	req_msg.num = sizeof(pages);
> -
> -	args[0].ptr = (u64) (uintptr_t) &req_msg;
> -	args[0].length = sizeof(req_msg);
> -
> -	pages.addr = buf->phys;
> -	pages.size = buf->size;
> -
> -	args[1].ptr = (u64) (uintptr_t) &pages;
> -	args[1].length = sizeof(pages);
> -
> -	args[2].ptr = (u64) (uintptr_t) &rsp_msg;
> -	args[2].length = sizeof(rsp_msg);
> -
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> -				      &args[0]);
> -	if (err) {
> -		dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
> +	err = fastrpc_req_map_dsp(fl, buf->phys, buf->size, buf->flag,
> +				req.vaddrin, &raddr);
> +	if (err)
>  		goto err_invoke;
> -	}
>  
>  	/* update the buffer to be able to deallocate the memory on the DSP */
> -	buf->raddr = (uintptr_t) rsp_msg.vaddr;
> +	buf->raddr = (uintptr_t) raddr;
>  
>  	/* let the client know the address to use */
> -	req.vaddrout = rsp_msg.vaddr;
> +	req.vaddrout = raddr;
>  
>  	spin_lock(&fl->lock);
>  	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> @@ -2129,16 +2151,14 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  		list_add_tail(&buf->node, &fl->mmaps);
>  	spin_unlock(&fl->lock);
>  
> +	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> +		buf->raddr, buf->size);
> +
>  	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>  		err = -EFAULT;
>  		goto err_copy;
>  	}
> -
> -	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> -		buf->raddr, buf->size);
> -
>  	return 0;
> -
>  err_copy:
>  	spin_lock(&fl->lock);
>  	list_del(&buf->node);
> @@ -2146,11 +2166,77 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  	fastrpc_req_munmap_impl(fl, buf);
>  	buf = NULL;
>  err_invoke:
> -	fastrpc_buf_free(buf);
> +	if (buf)
> +		fastrpc_buf_free(buf);
> +
> +	return err;
> +}
> +
> +static int fastrpc_req_map_create(struct fastrpc_user *fl,
> +			struct fastrpc_req_mmap req, char __user *argp)
> +{
> +	struct fastrpc_map *map = NULL;
> +	struct device *dev = fl->sctx->dev;
> +	u64 raddr = 0;
> +	int err;
> +
> +	if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->is_unsigned_pd) {
> +		dev_err(dev, "secure memory allocation is not supported in unsigned PD\n");
> +		return -EINVAL;
> +	}
> +	err = fastrpc_map_create(fl, req.fd, req.size, 0, &map);
> +	if (err) {
> +		dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
> +		return err;
> +	}
> +
> +	err = fastrpc_req_map_dsp(fl, map->phys, map->size, req.flags,
> +				req.vaddrin, &raddr);
> +	if (err)
> +		goto err_invoke;
> +
> +	/* update the buffer to be able to deallocate the memory on the DSP */
> +	map->raddr = (uintptr_t) raddr;
> +
> +	/* let the client know the address to use */
> +	req.vaddrout = raddr;
> +	dev_dbg(dev, "mmap\t\tpt 0x%09llx OK [len 0x%08llx]\n",
> +		map->raddr, map->size);
> +
> +	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
> +		err = -EFAULT;
> +		goto err_copy;
> +	}
> +	return 0;
> +err_copy:
> +	fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
> +err_invoke:
> +	fastrpc_map_put(map);
>  
>  	return err;
>  }
>  
> +static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> +{
> +	struct fastrpc_req_mmap req;
> +	int err;
> +
> +	if (copy_from_user(&req, argp, sizeof(req)))
> +		return -EFAULT;
> +
> +	if ((req.flags == ADSP_MMAP_ADD_PAGES ||
> +	     req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) {
> +		err = fastrpc_req_buf_alloc(fl, req, argp);
> +		if (err)
> +			return err;
> +	} else {
> +		err = fastrpc_req_map_create(fl, req, argp);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
> +}
> +
>  static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
>  {
>  	struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry