Re: [PATCH v4 10/11] misc: fastrpc: Fix unsigned PD support

From: Dmitry Baryshkov
Date: Fri Jun 07 2024 - 07:47:14 EST

Next message: Naresh Kamboju: "Re: [PATCH 6.9 000/374] 6.9.4-rc1 review"
Previous message: Ilpo Järvinen: "Re: [PATCH v5 2/6] platform: arm64: add Lenovo Yoga C630 WOS EC driver"
In reply to: Ekansh Gupta: "[PATCH v4 10/11] misc: fastrpc: Fix unsigned PD support"
Next in thread: Ekansh Gupta: "[PATCH v4 11/11] misc: fastrpc: Restrict untrusted app to attach to privileged PD"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
> ---
> 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

Next message: Naresh Kamboju: "Re: [PATCH 6.9 000/374] 6.9.4-rc1 review"
Previous message: Ilpo Järvinen: "Re: [PATCH v5 2/6] platform: arm64: add Lenovo Yoga C630 WOS EC driver"
In reply to: Ekansh Gupta: "[PATCH v4 10/11] misc: fastrpc: Fix unsigned PD support"
Next in thread: Ekansh Gupta: "[PATCH v4 11/11] misc: fastrpc: Restrict untrusted app to attach to privileged PD"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]