This patch series adds the listed features that have been missing
in upstream fastRPC driver.
- Add missing bug fixes.
- Add static PD restart support for audio and sensors PD using
PDR framework.
- Redesign and improve remote heap management.
- Add fixes for unsigned PD. Unsigned PD can be enabled
using userspace API:
https://git.codelinaro.org/linaro/qcomlt/fastrpc/-/blob/master/src/fastrpc_apps_user.c?ref_type=heads#L1173
- Add check for untrusted applications and allow trusted processed to
offload to system unsigned PD.
https://git.codelinaro.org/srinivas.kandagatla/fastrpc-qcom/-/commit/dfd073681d6a02efa080c5066546ff80c609668a
Changes in v2:
- Added separate patch to add newlines in dev_err.
- Added a bug fix in fastrpc capability function.
- Added a new patch to save and restore interrupted context.
- Fixed config dependency for PDR support.
Changes in v3:
- Dropped interrupted context patch.
- Splitted few of the bug fix patches.
- Added Fixes tag wherever applicable.
- Updated proper commit message for few of the patches.
Ekansh Gupta (9):
misc: fastrpc: Add missing dev_err newlines
misc: fastrpc: Fix DSP capabilities request
misc: fastrpc: Fix memory corruption in DSP capabilities
misc: fastrpc: Add static PD restart support
misc: fastrpc: Redesign remote heap management
misc: fastrpc: Fix unsigned PD support
misc: fastrpc: Restrict untrusted app to attach to privileged PD
misc: fastrpc: Restrict untrusted app to spawn signed PD
misc: fastrpc: Add system unsigned PD support
drivers/misc/Kconfig | 2 +
drivers/misc/fastrpc.c | 635 +++++++++++++++++++++++++++++-------
include/uapi/misc/fastrpc.h | 2 +
3 files changed, 526 insertions(+), 113 deletions(-)
--
2.43.0
Few dev_err calls are missing newlines. This can result in unrelated
lines getting appended which might make logs difficult to understand.
Add trailing newlines to avoid this.
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4c67e2c5a82e..4028cb96bcf2 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -325,7 +325,7 @@ static void fastrpc_free_map(struct kref *ref)
err = qcom_scm_assign_mem(map->phys, map->size,
&src_perms, &perm, 1);
if (err) {
- dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
+ dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
map->phys, map->size, err);
return;
}
@@ -816,7 +816,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
map->attr = attr;
err = qcom_scm_assign_mem(map->phys, (u64)map->size, &src_perms, dst_perms, 2);
if (err) {
- dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
+ dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
map->phys, map->size, err);
goto map_err;
}
@@ -1222,7 +1222,7 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
* that does not support unsigned PD offload
*/
if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
- dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD");
+ dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
return true;
}
}
@@ -1285,7 +1285,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
&src_perms,
fl->cctx->vmperms, fl->cctx->vmcount);
if (err) {
- dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
+ dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
goto err_map;
}
@@ -1337,7 +1337,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
(u64)fl->cctx->remote_heap->size,
&src_perms, &dst_perms, 1);
if (err)
- dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
+ dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
}
err_map:
--
2.43.0
DSP capabilities request is sending bad size to utilities skel
call which is resulting in memory corruption. Pass proper size
to avoid the corruption.
Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 61389795f498..3e1ab58038ed 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1695,6 +1695,7 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
/* Capability filled in userspace */
dsp_attr_buf[0] = 0;
+ dsp_attr_buf_len -= 1;
args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
args[0].length = sizeof(dsp_attr_buf_len);
--
2.43.0
Incorrect remote arguments are getting passed when requesting for
capabilities from DSP. Also there is no requirement to update the
PD type as it might cause problems for any PD other than user PD.
In addition to this, the collected capability information is not
getting copied properly to user. Add changes to address these
problems and get correct DSP capabilities.
Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4028cb96bcf2..61389795f498 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
args[0].length = sizeof(dsp_attr_buf_len);
args[0].fd = -1;
args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
- args[1].length = dsp_attr_buf_len;
+ args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
args[1].fd = -1;
- fl->pd = USER_PD;
return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
FASTRPC_SCALARS(0, 1, 1), args);
@@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
if (!dsp_attributes)
return -ENOMEM;
- err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
+ err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
if (err == DSP_UNSUPPORTED_API) {
dev_info(&cctx->rpdev->dev,
"Warning: DSP capabilities not supported on domain: %d\n", domain);
@@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
if (err)
return err;
- if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
+ if (copy_to_user(argp, &cap, sizeof(cap)))
return -EFAULT;
return 0;
--
2.43.0
Current remote heap design comes with problems where all types of
buffers are getting added to interrupted list and also user unmap
request is not handling the request to unmap remote heap buffer
type. Add changes to maintain list in a way that it can be used to
to support remote heap grow/shrink request and the remote heap
buffers cleanup during static PD restart.
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 221 ++++++++++++++++++++++++++++++++---------
1 file changed, 175 insertions(+), 46 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index d115860fc356..ef9bbd8c3dd1 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -210,6 +210,7 @@ struct fastrpc_buf {
struct dma_buf *dmabuf;
struct device *dev;
void *virt;
+ u32 flag;
u64 phys;
u64 size;
/* Lock for dma buf attachments */
@@ -274,6 +275,7 @@ struct fastrpc_session_ctx {
};
struct fastrpc_static_pd {
+ struct list_head rmaps;
char *servloc_name;
char *spdname;
void *pdrhandle;
@@ -299,7 +301,6 @@ struct fastrpc_channel_ctx {
u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
struct fastrpc_device *secure_fdevice;
struct fastrpc_device *fdevice;
- struct fastrpc_buf *remote_heap;
struct list_head invoke_interrupted_mmaps;
bool secure;
bool unsigned_support;
@@ -1256,6 +1257,53 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
return false;
}
+static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
+{
+ struct fastrpc_buf *buf, *b;
+ struct fastrpc_channel_ctx *cctx;
+ int err;
+
+ if (!spd || !spd->fl)
+ return;
+
+ cctx = spd->cctx;
+
+ spin_lock(&spd->fl->lock);
+ list_for_each_entry_safe(buf, b, &spd->rmaps, node) {
+ list_del(&buf->node);
+ spin_unlock(&spd->fl->lock);
+ if (cctx->vmcount) {
+ u64 src_perms = 0;
+ struct qcom_scm_vmperm dst_perms;
+ u32 i;
+
+ for (i = 0; i < cctx->vmcount; i++)
+ src_perms |= BIT(cctx->vmperms[i].vmid);
+
+ dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+ dst_perms.perm = QCOM_SCM_PERM_RWX;
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, &dst_perms, 1);
+ if (err) {
+ pr_err("%s: Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ __func__, buf->phys, buf->size, err);
+ return;
+ }
+ }
+ fastrpc_buf_free(buf);
+ spin_lock(&spd->fl->lock);
+ }
+ spin_unlock(&spd->fl->lock);
+}
+
+static void fastrpc_mmap_remove_ssr(struct fastrpc_channel_ctx *cctx)
+{
+ int i;
+
+ for (i = 0; i < FASTRPC_MAX_SPD; i++)
+ fastrpc_mmap_remove_pdr(&cctx->spd[i]);
+}
+
static struct fastrpc_static_pd *fastrpc_get_spd_session(
struct fastrpc_user *fl)
{
@@ -1282,9 +1330,12 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
struct fastrpc_init_create_static init;
struct fastrpc_invoke_args *args;
struct fastrpc_phy_page pages[1];
+ struct fastrpc_buf *buf = NULL;
struct fastrpc_static_pd *spd = NULL;
+ u64 phys = 0, size = 0;
char *name;
int err;
+ bool scm_done = false;
struct {
int pgid;
u32 namelen;
@@ -1330,25 +1381,26 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
goto err_name;
}
fl->spd = spd;
- if (!fl->cctx->remote_heap) {
- err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
- &fl->cctx->remote_heap);
+ if (list_empty(&spd->rmaps)) {
+ err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &buf);
if (err)
goto err_name;
+ phys = buf->phys;
+ size = buf->size;
/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
if (fl->cctx->vmcount) {
u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
- err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
- (u64)fl->cctx->remote_heap->size,
+ err = qcom_scm_assign_mem(phys, (u64)size,
&src_perms,
fl->cctx->vmperms, fl->cctx->vmcount);
if (err) {
dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
- fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
+ phys, size, err);
goto err_map;
}
+ scm_done = true;
}
}
@@ -1365,8 +1417,8 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
args[1].length = inbuf.namelen;
args[1].fd = -1;
- pages[0].addr = fl->cctx->remote_heap->phys;
- pages[0].size = fl->cctx->remote_heap->size;
+ pages[0].addr = phys;
+ pages[0].size = size;
args[2].ptr = (u64)(uintptr_t) pages;
args[2].length = sizeof(*pages);
@@ -1380,10 +1432,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
goto err_invoke;
kfree(args);
+ kfree(name);
+
+ if (buf) {
+ spin_lock(&fl->lock);
+ list_add_tail(&buf->node, &spd->rmaps);
+ spin_unlock(&fl->lock);
+ }
return 0;
err_invoke:
- if (fl->cctx->vmcount) {
+ if (fl->cctx->vmcount && scm_done) {
u64 src_perms = 0;
struct qcom_scm_vmperm dst_perms;
u32 i;
@@ -1393,15 +1452,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
dst_perms.vmid = QCOM_SCM_VMID_HLOS;
dst_perms.perm = QCOM_SCM_PERM_RWX;
- err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
- (u64)fl->cctx->remote_heap->size,
+ err = qcom_scm_assign_mem(phys, (u64)size,
&src_perms, &dst_perms, 1);
if (err)
dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
- fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
+ phys, size, err);
}
err_map:
- fastrpc_buf_free(fl->cctx->remote_heap);
+ if (buf)
+ fastrpc_buf_free(buf);
err_name:
kfree(name);
err:
@@ -1866,17 +1925,16 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
return 0;
}
-static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
+static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size)
{
struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
struct fastrpc_munmap_req_msg req_msg;
- struct device *dev = fl->sctx->dev;
int err;
u32 sc;
req_msg.pgid = fl->tgid;
- req_msg.size = buf->size;
- req_msg.vaddr = buf->raddr;
+ req_msg.size = size;
+ req_msg.vaddr = raddr;
args[0].ptr = (u64) (uintptr_t) &req_msg;
args[0].length = sizeof(req_msg);
@@ -1884,11 +1942,38 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
&args[0]);
+
+ return err;
+}
+
+static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
+{
+ struct device *dev = fl->sctx->dev;
+ int err;
+
+ err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size);
if (!err) {
+ if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) {
+ if (fl->cctx->vmcount) {
+ u64 src_perms = 0;
+ struct qcom_scm_vmperm dst_perms;
+ u32 i;
+
+ for (i = 0; i < fl->cctx->vmcount; i++)
+ src_perms |= BIT(fl->cctx->vmperms[i].vmid);
+
+ dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+ dst_perms.perm = QCOM_SCM_PERM_RWX;
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, &dst_perms, 1);
+ if (err) {
+ dev_err(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ buf->phys, buf->size, err);
+ return err;
+ }
+ }
+ }
dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
- spin_lock(&fl->lock);
- list_del(&buf->node);
- spin_unlock(&fl->lock);
fastrpc_buf_free(buf);
} else {
dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
@@ -1902,6 +1987,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 device *dev = fl->sctx->dev;
+ int err = 0;
if (copy_from_user(&req, argp, sizeof(req)))
return -EFAULT;
@@ -1910,20 +1996,48 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
buf = iter;
+ list_del(&buf->node);
break;
}
}
spin_unlock(&fl->lock);
- if (!buf) {
- dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
- req.vaddrout, req.size);
- return -EINVAL;
+ if (buf) {
+ err = fastrpc_req_munmap_impl(fl, buf);
+ if (err) {
+ spin_lock(&fl->lock);
+ list_add_tail(&buf->node, &fl->mmaps);
+ spin_unlock(&fl->lock);
+ }
+ return err;
}
- return fastrpc_req_munmap_impl(fl, buf);
+ spin_lock(&fl->lock);
+ if (fl->spd) {
+ list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) {
+ if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
+ buf = iter;
+ list_del(&buf->node);
+ break;
+ }
+ }
+ }
+ spin_unlock(&fl->lock);
+ if (buf) {
+ err = fastrpc_req_munmap_impl(fl, buf);
+ if (err) {
+ spin_lock(&fl->lock);
+ list_add_tail(&buf->node, &fl->spd->rmaps);
+ spin_unlock(&fl->lock);
+ }
+ return err;
+ }
+ dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
+ req.vaddrout, req.size);
+ return -EINVAL;
}
+
static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
{
struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
@@ -1950,16 +2064,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
return -EINVAL;
}
- if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
+ if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
+ if (!fl->spd || !fl->spd->ispdup) {
+ dev_err(dev, "remote heap request supported only for active static PD\n");
+ return -EINVAL;
+ }
err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
- else
+ } else {
err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
+ }
if (err) {
dev_err(dev, "failed to allocate buffer\n");
return err;
}
+ buf->flag = req.flags;
+ /* Add memory to static PD pool, protection through hypervisor */
+ if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
+ u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
+ if (err) {
+ dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ buf->phys, buf->size, err);
+ goto err_invoke;
+ }
+ }
req_msg.pgid = fl->tgid;
req_msg.flags = req.flags;
req_msg.vaddr = req.vaddrin;
@@ -1991,26 +2123,17 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
/* let the client know the address to use */
req.vaddrout = rsp_msg.vaddr;
- /* Add memory to static PD pool, protection thru hypervisor */
- if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
- u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
-
- err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
- &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
- if (err) {
- dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
- buf->phys, buf->size, err);
- goto err_assign;
- }
- }
spin_lock(&fl->lock);
- list_add_tail(&buf->node, &fl->mmaps);
+ if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
+ list_add_tail(&buf->node, &fl->spd->rmaps);
+ else
+ list_add_tail(&buf->node, &fl->mmaps);
spin_unlock(&fl->lock);
if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
err = -EFAULT;
- goto err_assign;
+ goto err_copy;
}
dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
@@ -2018,14 +2141,19 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
return 0;
-err_assign:
+err_copy:
+ spin_lock(&fl->lock);
+ list_del(&buf->node);
+ spin_unlock(&fl->lock);
fastrpc_req_munmap_impl(fl, buf);
+ buf = NULL;
err_invoke:
fastrpc_buf_free(buf);
return err;
}
+
static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
{
struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
@@ -2249,6 +2377,7 @@ static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
spd->servloc_name,
domains[cctx->domain_id]);
spd->ispdup = false;
+ fastrpc_mmap_remove_pdr(spd);
fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
break;
case SERVREG_SERVICE_STATE_UP:
@@ -2400,6 +2529,7 @@ static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char
cctx->spd[spd_session].servloc_name = client_name;
cctx->spd[spd_session].spdname = service_path;
cctx->spd[spd_session].cctx = cctx;
+ INIT_LIST_HEAD(&cctx->spd[spd_session].rmaps);
service = pdr_add_lookup(handle, service_name, service_path);
if (IS_ERR(service)) {
err = PTR_ERR(service);
@@ -2566,9 +2696,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
list_del(&buf->node);
- if (cctx->remote_heap)
- fastrpc_buf_free(cctx->remote_heap);
-
for (i = 0; i < FASTRPC_MAX_SPD; i++) {
if (cctx->spd[i].pdrhandle)
pdr_handle_release(cctx->spd[i].pdrhandle);
@@ -2576,6 +2703,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
of_platform_depopulate(&rpdev->dev);
+ fastrpc_mmap_remove_ssr(cctx);
+
fastrpc_channel_ctx_put(cctx);
}
--
2.43.0
Untrusted application with access to only non-secure fastrpc device
node can attach to root_pd or static PDs if it can make the respective
init request. This can cause problems as the untrusted application
can send bad requests to root_pd or static PDs. Add changes to reject
attach to privileged PDs if the request is being made using non-secure
fastrpc device node.
Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index d9d9f889e39e..73fa0e536cf9 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1344,6 +1344,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
} inbuf;
u32 sc;
+ if (!fl->is_secure_dev) {
+ dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
+ return -EACCES;
+ }
+
args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
if (!args)
return -ENOMEM;
@@ -1769,6 +1774,11 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
int tgid = fl->tgid;
u32 sc;
+ if (!fl->is_secure_dev) {
+ dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
+ return -EACCES;
+ }
+
args[0].ptr = (u64)(uintptr_t) &tgid;
args[0].length = sizeof(tgid);
args[0].fd = -1;
--
2.43.0
Static PDs on the audio and sensor domains are expected to support
PD restart. The kernel resource handling for the PDs are expected
to be handled by fastrpc driver. For this, there is a requirement
of PD service locator to get the event notifications for static PD
services. Also when events are received, the driver needs to handle
based on PD states. Added changes to add service locator for audio
and sensor domain static PDs and handle the PD restart sequence.
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/Kconfig | 2 +
drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 195 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index faf983680040..e2d83cd085b5 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -280,8 +280,10 @@ config QCOM_FASTRPC
tristate "Qualcomm FastRPC"
depends on ARCH_QCOM || COMPILE_TEST
depends on RPMSG
+ depends on NET
select DMA_SHARED_BUFFER
select QCOM_SCM
+ select QCOM_PDR_HELPERS
help
Provides a communication mechanism that allows for clients to
make remote method invocations across processor boundary to
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 3e1ab58038ed..d115860fc356 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -22,6 +22,7 @@
#include <linux/firmware/qcom/qcom_scm.h>
#include <uapi/misc/fastrpc.h>
#include <linux/of_reserved_mem.h>
+#include <linux/soc/qcom/pdr.h>
#define ADSP_DOMAIN_ID (0)
#define MDSP_DOMAIN_ID (1)
@@ -29,6 +30,7 @@
#define CDSP_DOMAIN_ID (3)
#define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
#define FASTRPC_MAX_SESSIONS 14
+#define FASTRPC_MAX_SPD 4
#define FASTRPC_MAX_VMIDS 16
#define FASTRPC_ALIGN 128
#define FASTRPC_MAX_FDLIST 16
@@ -105,6 +107,18 @@
#define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
+#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
+#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
+#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
+
+#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
+#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
+#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
+
+#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
+#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
+#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
+
static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
"sdsp", "cdsp"};
struct fastrpc_phy_page {
@@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
bool valid;
};
+struct fastrpc_static_pd {
+ char *servloc_name;
+ char *spdname;
+ void *pdrhandle;
+ struct fastrpc_channel_ctx *cctx;
+ struct fastrpc_user *fl;
+ bool ispdup;
+};
+
struct fastrpc_channel_ctx {
int domain_id;
int sesscount;
@@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
struct rpmsg_device *rpdev;
struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
+ struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
spinlock_t lock;
struct idr ctx_idr;
struct list_head users;
@@ -297,10 +321,12 @@ struct fastrpc_user {
struct fastrpc_channel_ctx *cctx;
struct fastrpc_session_ctx *sctx;
struct fastrpc_buf *init_mem;
+ struct fastrpc_static_pd *spd;
int tgid;
int pd;
bool is_secure_dev;
+ char *servloc_name;
/* Lock for lists */
spinlock_t lock;
/* lock for allocations */
@@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
return false;
}
+static struct fastrpc_static_pd *fastrpc_get_spd_session(
+ struct fastrpc_user *fl)
+{
+ int i;
+ struct fastrpc_static_pd *spd = NULL;
+ struct fastrpc_channel_ctx *cctx = fl->cctx;
+
+ for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
+ if (!cctx->spd[i].servloc_name)
+ continue;
+ if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
+ spd = &cctx->spd[i];
+ spd->fl = fl;
+ break;
+ }
+ }
+
+ return spd;
+}
+
static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
char __user *argp)
{
struct fastrpc_init_create_static init;
struct fastrpc_invoke_args *args;
struct fastrpc_phy_page pages[1];
+ struct fastrpc_static_pd *spd = NULL;
char *name;
int err;
struct {
@@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
goto err_name;
}
+ fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
+
+ spd = fastrpc_get_spd_session(fl);
+ if (!spd) {
+ err = -EUSERS;
+ goto err_name;
+ }
+
+ if (!spd->ispdup) {
+ err = -ENOTCONN;
+ goto err_name;
+ }
+ fl->spd = spd;
if (!fl->cctx->remote_heap) {
err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
&fl->cctx->remote_heap);
@@ -1645,6 +1705,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
{
struct fastrpc_invoke_args args[1];
+ struct fastrpc_static_pd *spd = NULL;
int tgid = fl->tgid;
u32 sc;
@@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
fl->pd = pd;
+ if (pd == SENSORS_PD) {
+ if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
+ fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
+ else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
+ fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
+
+ spd = fastrpc_get_spd_session(fl);
+ if (!spd)
+ return -EUSERS;
+
+ if (!spd->ispdup)
+ return -ENOTCONN;
+
+ fl->spd = spd;
+ }
+
return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
sc, &args[0]);
}
@@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
return err;
}
+static void fastrpc_notify_users(struct fastrpc_user *user)
+{
+ struct fastrpc_invoke_ctx *ctx;
+
+ spin_lock(&user->lock);
+ list_for_each_entry(ctx, &user->pending, node) {
+ ctx->retval = -EPIPE;
+ complete(&ctx->work);
+ }
+ spin_unlock(&user->lock);
+}
+
+static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
+ char *servloc_name)
+{
+ struct fastrpc_user *fl;
+ unsigned long flags;
+
+ spin_lock_irqsave(&cctx->lock, flags);
+ list_for_each_entry(fl, &cctx->users, user) {
+ if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
+ fastrpc_notify_users(fl);
+ }
+ spin_unlock_irqrestore(&cctx->lock, flags);
+}
+
+static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
+{
+ struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
+ struct fastrpc_channel_ctx *cctx;
+
+ if (!spd)
+ return;
+
+ cctx = spd->cctx;
+ switch (state) {
+ case SERVREG_SERVICE_STATE_DOWN:
+ dev_info(&cctx->rpdev->dev,
+ "%s: %s (%s) is down for PDR on %s\n",
+ __func__, spd->spdname,
+ spd->servloc_name,
+ domains[cctx->domain_id]);
+ spd->ispdup = false;
+ fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
+ break;
+ case SERVREG_SERVICE_STATE_UP:
+ dev_info(&cctx->rpdev->dev,
+ "%s: %s (%s) is up for PDR on %s\n",
+ __func__, spd->spdname,
+ spd->servloc_name,
+ domains[cctx->domain_id]);
+ spd->ispdup = true;
+ break;
+ default:
+ break;
+ }
+}
+
static const struct file_operations fastrpc_fops = {
.open = fastrpc_device_open,
.release = fastrpc_device_release,
@@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
return err;
}
+static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
+ char *service_name, char *service_path, int domain, int spd_session)
+{
+ int err = 0;
+ struct pdr_handle *handle = NULL;
+ struct pdr_service *service = NULL;
+
+ /* Register the service locator's callback function */
+ handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
+ if (IS_ERR(handle)) {
+ err = PTR_ERR(handle);
+ goto bail;
+ }
+ cctx->spd[spd_session].pdrhandle = handle;
+ cctx->spd[spd_session].servloc_name = client_name;
+ cctx->spd[spd_session].spdname = service_path;
+ cctx->spd[spd_session].cctx = cctx;
+ service = pdr_add_lookup(handle, service_name, service_path);
+ if (IS_ERR(service)) {
+ err = PTR_ERR(service);
+ goto bail;
+ }
+ pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
+ __func__, service_name, client_name, service_path);
+
+bail:
+ if (err) {
+ pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
+ __func__, service_name, client_name, service_path, err);
+ }
+ return err;
+}
+
static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
{
struct device *rdev = &rpdev->dev;
@@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
goto fdev_error;
}
+ if (domain_id == ADSP_DOMAIN_ID) {
+ err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
+ AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
+ if (err)
+ goto populate_error;
+
+ err = fastrpc_setup_service_locator(data,
+ SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
+ SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
+ if (err)
+ goto populate_error;
+ } else if (domain_id == SDSP_DOMAIN_ID) {
+ err = fastrpc_setup_service_locator(data,
+ SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
+ SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
+ if (err)
+ goto populate_error;
+ }
+
kref_init(&data->refcount);
dev_set_drvdata(&rpdev->dev, data);
@@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
return err;
}
-static void fastrpc_notify_users(struct fastrpc_user *user)
-{
- struct fastrpc_invoke_ctx *ctx;
-
- spin_lock(&user->lock);
- list_for_each_entry(ctx, &user->pending, node) {
- ctx->retval = -EPIPE;
- complete(&ctx->work);
- }
- spin_unlock(&user->lock);
-}
-
static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
{
struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
struct fastrpc_buf *buf, *b;
struct fastrpc_user *user;
unsigned long flags;
+ int i;
/* No invocations past this point */
spin_lock_irqsave(&cctx->lock, flags);
@@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
if (cctx->remote_heap)
fastrpc_buf_free(cctx->remote_heap);
+ for (i = 0; i < FASTRPC_MAX_SPD; i++) {
+ if (cctx->spd[i].pdrhandle)
+ pdr_handle_release(cctx->spd[i].pdrhandle);
+ }
+
of_platform_depopulate(&rpdev->dev);
fastrpc_channel_ctx_put(cctx);
--
2.43.0
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.
Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 221 ++++++++++++++++++++++++++---------------
1 file changed, 141 insertions(+), 80 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index ef9bbd8c3dd1..d9d9f889e39e 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)
#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,10 @@ 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;
}
@@ -1986,6 +1987,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;
@@ -2032,76 +2034,49 @@ 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);
+ else
+ fastrpc_map_put(map);
-static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
+ return err;
+}
+
+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;
-
- 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);
-
- return -EINVAL;
- }
-
- if (req.vaddrin) {
- dev_err(dev, "adding user allocated pages is not supported\n");
- return -EINVAL;
- }
-
- if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
- if (!fl->spd || !fl->spd->ispdup) {
- dev_err(dev, "remote heap request supported only for active static PD\n");
- return -EINVAL;
- }
- err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
- } else {
- err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
- }
-
- if (err) {
- dev_err(dev, "failed to allocate buffer\n");
- return err;
- }
- buf->flag = req.flags;
-
- /* Add memory to static PD pool, protection through hypervisor */
- if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
- u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
-
- err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
- &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
- if (err) {
- dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
- buf->phys, buf->size, err);
- goto err_invoke;
- }
- }
req_msg.pgid = fl->tgid;
- req_msg.flags = req.flags;
- req_msg.vaddr = req.vaddrin;
+ req_msg.flags = flag;
+ req_msg.vaddr = 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;
+ pages.addr = phys;
+ pages.size = size;
args[1].ptr = (u64) (uintptr_t) &pages;
args[1].length = sizeof(pages);
@@ -2111,49 +2086,135 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
- &args[0]);
+ &args[0]);
+
if (err) {
- dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
- goto err_invoke;
+ dev_err(fl->sctx->dev, "mmap error (len 0x%08llx)\n", size);
+ return err;
}
+ *raddr = rsp_msg.vaddr;
- /* update the buffer to be able to deallocate the memory on the DSP */
- buf->raddr = (uintptr_t) rsp_msg.vaddr;
+ return err;
+}
- /* let the client know the address to use */
- req.vaddrout = rsp_msg.vaddr;
+static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
+{
+ struct fastrpc_buf *buf = NULL;
+ struct fastrpc_req_mmap req;
+ struct fastrpc_map *map = NULL;
+ struct device *dev = fl->sctx->dev;
+ u64 raddr = 0;
+ int err;
+ if (copy_from_user(&req, argp, sizeof(req)))
+ return -EFAULT;
- spin_lock(&fl->lock);
- if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
- list_add_tail(&buf->node, &fl->spd->rmaps);
- else
- list_add_tail(&buf->node, &fl->mmaps);
- spin_unlock(&fl->lock);
+ if ((req.flags == ADSP_MMAP_ADD_PAGES ||
+ req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) {
+ if (req.vaddrin) {
+ dev_err(dev, "adding user allocated pages is not supported for signed PD\n");
+ return -EINVAL;
+ }
+
+ if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
+ if (!fl->spd || !fl->spd->ispdup) {
+ dev_err(dev, "remote heap request supported only for active static PD\n");
+ return -EINVAL;
+ }
+ err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
+ } else {
+ err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
+ }
+
+ if (err) {
+ dev_err(dev, "failed to allocate buffer\n");
+ return err;
+ }
+ buf->flag = req.flags;
+
+ /* Add memory to static PD pool, protection through hypervisor */
+ if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
+ u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
+ if (err) {
+ dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ buf->phys, buf->size, err);
+ goto err_invoke;
+ }
+ }
+
+ 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) raddr;
+
+ /* let the client know the address to use */
+ req.vaddrout = raddr;
+ spin_lock(&fl->lock);
+ if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
+ list_add_tail(&buf->node, &fl->spd->rmaps);
+ else
+ 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);
+ } else {
+ 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;
}
- 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);
- spin_unlock(&fl->lock);
- fastrpc_req_munmap_impl(fl, buf);
- buf = NULL;
+ if ((req.flags != ADSP_MMAP_ADD_PAGES &&
+ req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) || fl->is_unsigned_pd) {
+ fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
+ } else {
+ spin_lock(&fl->lock);
+ list_del(&buf->node);
+ spin_unlock(&fl->lock);
+ fastrpc_req_munmap_impl(fl, buf);
+ buf = NULL;
+ }
err_invoke:
- fastrpc_buf_free(buf);
+ if (map)
+ fastrpc_map_put(map);
+ if (buf)
+ fastrpc_buf_free(buf);
return err;
}
-
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
Trusted CPU applications currently offload to signed PDs on CDSP to
gain some additional services provided by root PD. Unsigned PDs have
access to limited root PD services that may not be sufficient for
all use-cases. Signed PDs have a higher dynamic loading latency
which impacts the performance of applications. Limited root PD
services could be opened up for unsigned PDs but that should be
restricted for untrusted processes. For this requirement, System
unsigned PD is introduced which will be same as Unsigned PD for
most part but will have access to more root PD services. Add
changes to offload trusted applications to System unsigned PD
when unsigned offload is requested.
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 27 +++++++++++++++++++++------
include/uapi/misc/fastrpc.h | 2 ++
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 32615ccde7ac..fc24653d7b7e 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -119,6 +119,12 @@
#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
+enum fastrpc_userpd_type {
+ SIGNED_PD = 1,
+ UNSIGNED_PD = 2,
+ SYSTEM_UNSIGNED_PD = 3,
+};
+
static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
"sdsp", "cdsp"};
struct fastrpc_phy_page {
@@ -327,7 +333,7 @@ struct fastrpc_user {
int tgid;
int pd;
bool is_secure_dev;
- bool is_unsigned_pd;
+ enum fastrpc_userpd_type userpd_type;
bool untrusted_process;
char *servloc_name;
/* Lock for lists */
@@ -1518,14 +1524,22 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
fl->untrusted_process = true;
if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
- fl->is_unsigned_pd = true;
+ fl->userpd_type = UNSIGNED_PD;
+ /* Disregard any system unsigned PD attribute from userspace */
+ init.attrs &= (~FASTRPC_MODE_SYSTEM_UNSIGNED_PD);
- if (is_session_rejected(fl, fl->is_unsigned_pd)) {
+ if (is_session_rejected(fl, fl->userpd_type != SIGNED_PD)) {
err = -EACCES;
goto err;
}
+ /* Trusted apps will be launched as system unsigned PDs */
+ if (!fl->untrusted_process && (fl->userpd_type != SIGNED_PD)) {
+ fl->userpd_type = SYSTEM_UNSIGNED_PD;
+ init.attrs |= FASTRPC_MODE_SYSTEM_UNSIGNED_PD;
+ }
+
if (init.filelen > INIT_FILELEN_MAX) {
err = -EINVAL;
goto err;
@@ -1718,6 +1732,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
fl->tgid = current->tgid;
fl->cctx = cctx;
fl->is_secure_dev = fdevice->secure;
+ fl->userpd_type = SIGNED_PD;
fl->sctx = fastrpc_session_alloc(cctx);
if (!fl->sctx) {
@@ -2133,7 +2148,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
return -EFAULT;
if ((req.flags == ADSP_MMAP_ADD_PAGES ||
- req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) {
+ req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && (fl->userpd_type == SIGNED_PD)) {
if (req.vaddrin) {
dev_err(dev, "adding user allocated pages is not supported for signed PD\n");
return -EINVAL;
@@ -2188,7 +2203,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
buf->raddr, buf->size);
} else {
- if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->is_unsigned_pd) {
+ if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && (fl->userpd_type != SIGNED_PD)) {
dev_err(dev, "secure memory allocation is not supported in unsigned PD\n");
return -EINVAL;
}
@@ -2220,7 +2235,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
err_copy:
if ((req.flags != ADSP_MMAP_ADD_PAGES &&
- req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) || fl->is_unsigned_pd) {
+ req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) || fl->userpd_type != SIGNED_PD) {
fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
} else {
spin_lock(&fl->lock);
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index f33d914d8f46..3b3279bb2cf9 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -62,6 +62,8 @@ enum fastrpc_proc_attr {
FASTRPC_MODE_SYSTEM_PROCESS = (1 << 5),
/* Macro for Prvileged Process */
FASTRPC_MODE_PRIVILEGED = (1 << 6),
+ /* Macro for system unsigned PD */
+ FASTRPC_MODE_SYSTEM_UNSIGNED_PD = (1 << 17),
};
/* Fastrpc attribute for memory protection of buffers */
--
2.43.0
Some untrusted applications will not have access to open fastrpc
device nodes and a privileged process can open the device node on
behalf of the application. Add a check to restrict such untrusted
applications from offloading to signed PD.
Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 73fa0e536cf9..32615ccde7ac 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -328,6 +328,7 @@ struct fastrpc_user {
int pd;
bool is_secure_dev;
bool is_unsigned_pd;
+ bool untrusted_process;
char *servloc_name;
/* Lock for lists */
spinlock_t lock;
@@ -1249,13 +1250,17 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
* channel is configured as secure and block untrusted apps on channel
* that does not support unsigned PD offload
*/
- if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
- dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
- return true;
- }
+ if (!fl->cctx->unsigned_support || !unsigned_pd_request)
+ goto reject_session;
}
+ /* Check if untrusted process is trying to offload to signed PD */
+ if (fl->untrusted_process && !unsigned_pd_request)
+ goto reject_session;
return false;
+reject_session:
+ dev_dbg(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
+ return true;
}
static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
@@ -1504,12 +1509,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
goto err;
}
+ /*
+ * Third-party apps don't have permission to open the fastrpc device, so
+ * it is opened on their behalf by a priveleged process. This is detected
+ * by comparing current PID with the one stored during device open.
+ */
+ if (current->tgid != fl->tgid)
+ fl->untrusted_process = true;
+
if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
fl->is_unsigned_pd = true;
if (is_session_rejected(fl, fl->is_unsigned_pd)) {
- err = -ECONNREFUSED;
+ err = -EACCES;
goto err;
}
--
2.43.0
On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote:
> Incorrect remote arguments are getting passed when requesting for
> capabilities from DSP.
Describe why and how they are incorrect.
> Also there is no requirement to update the
> PD type as it might cause problems for any PD other than user PD.
Also... means that these are two separate issues. There should be two
separate commits.
> In addition to this, the collected capability information is not
> getting copied properly to user. Add changes to address these
> problems and get correct DSP capabilities.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 4028cb96bcf2..61389795f498 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
> args[0].length = sizeof(dsp_attr_buf_len);
> args[0].fd = -1;
> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> - args[1].length = dsp_attr_buf_len;
> + args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
As you are skipping first entry, should there be (dsp_attr_buf_len - 1)
* sizeof(uint32_t).
> args[1].fd = -1;
> - fl->pd = USER_PD;
>
> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> FASTRPC_SCALARS(0, 1, 1), args);
> @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> if (!dsp_attributes)
> return -ENOMEM;
>
> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
So it looks like the argument was correct. It was passing length, not
the number of attributes. The only thing to fix is that args[1].length
should be dsp_attr_buf_len - sizeof(*dsp_attr_buf).
> if (err == DSP_UNSUPPORTED_API) {
> dev_info(&cctx->rpdev->dev,
> "Warning: DSP capabilities not supported on domain: %d\n", domain);
> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> if (err)
> return err;
>
> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
> + if (copy_to_user(argp, &cap, sizeof(cap)))
> return -EFAULT;
>
> return 0;
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:50:21PM +0530, Ekansh Gupta wrote:
> DSP capabilities request is sending bad size to utilities skel
> call which is resulting in memory corruption. Pass proper size
> to avoid the corruption.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
Should be squashed to the previous commit.
> ---
> drivers/misc/fastrpc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 61389795f498..3e1ab58038ed 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1695,6 +1695,7 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>
> /* Capability filled in userspace */
> dsp_attr_buf[0] = 0;
> + dsp_attr_buf_len -= 1;
>
> args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
> args[0].length = sizeof(dsp_attr_buf_len);
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:50:22PM +0530, Ekansh Gupta wrote:
> Static PDs on the audio and sensor domains are expected to support
> PD restart. The kernel resource handling for the PDs are expected
> to be handled by fastrpc driver. For this, there is a requirement
> of PD service locator to get the event notifications for static PD
> services. Also when events are received, the driver needs to handle
> based on PD states. Added changes to add service locator for audio
> and sensor domain static PDs and handle the PD restart sequence.
Let me repeat a comment from v2:
Please see Documentation/process/submitting-patches.rst for a suggested
language.
Please extend the commit message with the description of why sensors and
audio are handled in a different way.
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/Kconfig | 2 +
> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 195 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index faf983680040..e2d83cd085b5 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
> tristate "Qualcomm FastRPC"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on RPMSG
> + depends on NET
> select DMA_SHARED_BUFFER
> select QCOM_SCM
> + select QCOM_PDR_HELPERS
> help
> Provides a communication mechanism that allows for clients to
> make remote method invocations across processor boundary to
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 3e1ab58038ed..d115860fc356 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -22,6 +22,7 @@
> #include <linux/firmware/qcom/qcom_scm.h>
> #include <uapi/misc/fastrpc.h>
> #include <linux/of_reserved_mem.h>
> +#include <linux/soc/qcom/pdr.h>
>
> #define ADSP_DOMAIN_ID (0)
> #define MDSP_DOMAIN_ID (1)
> @@ -29,6 +30,7 @@
> #define CDSP_DOMAIN_ID (3)
> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
> #define FASTRPC_MAX_SESSIONS 14
> +#define FASTRPC_MAX_SPD 4
> #define FASTRPC_MAX_VMIDS 16
> #define FASTRPC_ALIGN 128
> #define FASTRPC_MAX_FDLIST 16
> @@ -105,6 +107,18 @@
>
> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>
> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
> +
> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
> +
> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
> +
> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> "sdsp", "cdsp"};
> struct fastrpc_phy_page {
> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
> bool valid;
> };
>
> +struct fastrpc_static_pd {
> + char *servloc_name;
> + char *spdname;
> + void *pdrhandle;
> + struct fastrpc_channel_ctx *cctx;
> + struct fastrpc_user *fl;
> + bool ispdup;
> +};
> +
> struct fastrpc_channel_ctx {
> int domain_id;
> int sesscount;
> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
> struct rpmsg_device *rpdev;
> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
> spinlock_t lock;
> struct idr ctx_idr;
> struct list_head users;
> @@ -297,10 +321,12 @@ struct fastrpc_user {
> struct fastrpc_channel_ctx *cctx;
> struct fastrpc_session_ctx *sctx;
> struct fastrpc_buf *init_mem;
> + struct fastrpc_static_pd *spd;
>
> int tgid;
> int pd;
> bool is_secure_dev;
> + char *servloc_name;
> /* Lock for lists */
> spinlock_t lock;
> /* lock for allocations */
> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
> + struct fastrpc_user *fl)
> +{
> + int i;
> + struct fastrpc_static_pd *spd = NULL;
> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> +
> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
> + if (!cctx->spd[i].servloc_name)
> + continue;
> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
> + spd = &cctx->spd[i];
> + spd->fl = fl;
> + break;
> + }
> + }
> +
> + return spd;
> +}
> +
> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> char __user *argp)
> {
> struct fastrpc_init_create_static init;
> struct fastrpc_invoke_args *args;
> struct fastrpc_phy_page pages[1];
> + struct fastrpc_static_pd *spd = NULL;
> char *name;
> int err;
> struct {
> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_name;
> }
>
> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd) {
> + err = -EUSERS;
> + goto err_name;
> + }
> +
> + if (!spd->ispdup) {
> + err = -ENOTCONN;
> + goto err_name;
> + }
> + fl->spd = spd;
> if (!fl->cctx->remote_heap) {
> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> &fl->cctx->remote_heap);
> @@ -1645,6 +1705,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
> static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> {
> struct fastrpc_invoke_args args[1];
> + struct fastrpc_static_pd *spd = NULL;
> int tgid = fl->tgid;
> u32 sc;
>
> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> fl->pd = pd;
>
> + if (pd == SENSORS_PD) {
> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd)
> + return -EUSERS;
> +
> + if (!spd->ispdup)
> + return -ENOTCONN;
> +
> + fl->spd = spd;
> + }
> +
> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> sc, &args[0]);
> }
> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> return err;
> }
>
> +static void fastrpc_notify_users(struct fastrpc_user *user)
> +{
> + struct fastrpc_invoke_ctx *ctx;
> +
> + spin_lock(&user->lock);
> + list_for_each_entry(ctx, &user->pending, node) {
> + ctx->retval = -EPIPE;
> + complete(&ctx->work);
> + }
> + spin_unlock(&user->lock);
> +}
> +
> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
> + char *servloc_name)
> +{
> + struct fastrpc_user *fl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&cctx->lock, flags);
> + list_for_each_entry(fl, &cctx->users, user) {
> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
> + fastrpc_notify_users(fl);
> + }
> + spin_unlock_irqrestore(&cctx->lock, flags);
> +}
> +
> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> +{
> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
> + struct fastrpc_channel_ctx *cctx;
> +
> + if (!spd)
> + return;
> +
> + cctx = spd->cctx;
> + switch (state) {
> + case SERVREG_SERVICE_STATE_DOWN:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is down for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
> + spd->ispdup = false;
> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> + break;
> + case SERVREG_SERVICE_STATE_UP:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is up for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
> + spd->ispdup = true;
> + break;
> + default:
> + break;
> + }
> +}
> +
> static const struct file_operations fastrpc_fops = {
> .open = fastrpc_device_open,
> .release = fastrpc_device_release,
> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
> return err;
> }
>
> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
> + char *service_name, char *service_path, int domain, int spd_session)
> +{
> + int err = 0;
> + struct pdr_handle *handle = NULL;
> + struct pdr_service *service = NULL;
> +
> + /* Register the service locator's callback function */
> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
> + if (IS_ERR(handle)) {
> + err = PTR_ERR(handle);
> + goto bail;
> + }
> + cctx->spd[spd_session].pdrhandle = handle;
> + cctx->spd[spd_session].servloc_name = client_name;
> + cctx->spd[spd_session].spdname = service_path;
> + cctx->spd[spd_session].cctx = cctx;
> + service = pdr_add_lookup(handle, service_name, service_path);
> + if (IS_ERR(service)) {
> + err = PTR_ERR(service);
> + goto bail;
> + }
> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
> + __func__, service_name, client_name, service_path);
> +
> +bail:
> + if (err) {
> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
> + __func__, service_name, client_name, service_path, err);
> + }
> + return err;
> +}
> +
> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> {
> struct device *rdev = &rpdev->dev;
> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> goto fdev_error;
> }
>
> + if (domain_id == ADSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> +
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
> + if (err)
> + goto populate_error;
> + } else if (domain_id == SDSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> + }
> +
> kref_init(&data->refcount);
>
> dev_set_drvdata(&rpdev->dev, data);
> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> return err;
> }
>
> -static void fastrpc_notify_users(struct fastrpc_user *user)
> -{
> - struct fastrpc_invoke_ctx *ctx;
> -
> - spin_lock(&user->lock);
> - list_for_each_entry(ctx, &user->pending, node) {
> - ctx->retval = -EPIPE;
> - complete(&ctx->work);
> - }
> - spin_unlock(&user->lock);
> -}
> -
> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> {
> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> struct fastrpc_buf *buf, *b;
> struct fastrpc_user *user;
> unsigned long flags;
> + int i;
>
> /* No invocations past this point */
> spin_lock_irqsave(&cctx->lock, flags);
> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> if (cctx->remote_heap)
> fastrpc_buf_free(cctx->remote_heap);
>
> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> + if (cctx->spd[i].pdrhandle)
> + pdr_handle_release(cctx->spd[i].pdrhandle);
> + }
> +
> of_platform_depopulate(&rpdev->dev);
>
> fastrpc_channel_ctx_put(cctx);
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:50:23PM +0530, Ekansh Gupta wrote:
> Current remote heap design comes with problems where all types of
> buffers are getting added to interrupted list and also user unmap
> request is not handling the request to unmap remote heap buffer
> type. Add changes to maintain list in a way that it can be used to
> to support remote heap grow/shrink request and the remote heap
> buffers cleanup during static PD restart.
This commit seems to contain several fixes squashed into one. You have
heap management, scm_done fix, missing SCM calls on rpmsg removal and
maybe several other fixes. Please separate them so that each commit
fixes just one issue in an atomic way.
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 221 ++++++++++++++++++++++++++++++++---------
> 1 file changed, 175 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d115860fc356..ef9bbd8c3dd1 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -210,6 +210,7 @@ struct fastrpc_buf {
> struct dma_buf *dmabuf;
> struct device *dev;
> void *virt;
> + u32 flag;
> u64 phys;
> u64 size;
> /* Lock for dma buf attachments */
> @@ -274,6 +275,7 @@ struct fastrpc_session_ctx {
> };
>
> struct fastrpc_static_pd {
> + struct list_head rmaps;
> char *servloc_name;
> char *spdname;
> void *pdrhandle;
> @@ -299,7 +301,6 @@ struct fastrpc_channel_ctx {
> u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
> struct fastrpc_device *secure_fdevice;
> struct fastrpc_device *fdevice;
> - struct fastrpc_buf *remote_heap;
> struct list_head invoke_interrupted_mmaps;
> bool secure;
> bool unsigned_support;
> @@ -1256,6 +1257,53 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
> +{
> + struct fastrpc_buf *buf, *b;
> + struct fastrpc_channel_ctx *cctx;
> + int err;
> +
> + if (!spd || !spd->fl)
> + return;
> +
> + cctx = spd->cctx;
> +
> + spin_lock(&spd->fl->lock);
> + list_for_each_entry_safe(buf, b, &spd->rmaps, node) {
> + list_del(&buf->node);
> + spin_unlock(&spd->fl->lock);
> + if (cctx->vmcount) {
> + u64 src_perms = 0;
> + struct qcom_scm_vmperm dst_perms;
> + u32 i;
> +
> + for (i = 0; i < cctx->vmcount; i++)
> + src_perms |= BIT(cctx->vmperms[i].vmid);
> +
> + dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> + dst_perms.perm = QCOM_SCM_PERM_RWX;
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, &dst_perms, 1);
> + if (err) {
> + pr_err("%s: Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + __func__, buf->phys, buf->size, err);
> + return;
> + }
> + }
> + fastrpc_buf_free(buf);
> + spin_lock(&spd->fl->lock);
> + }
> + spin_unlock(&spd->fl->lock);
> +}
> +
> +static void fastrpc_mmap_remove_ssr(struct fastrpc_channel_ctx *cctx)
> +{
> + int i;
> +
> + for (i = 0; i < FASTRPC_MAX_SPD; i++)
> + fastrpc_mmap_remove_pdr(&cctx->spd[i]);
> +}
> +
> static struct fastrpc_static_pd *fastrpc_get_spd_session(
> struct fastrpc_user *fl)
> {
> @@ -1282,9 +1330,12 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> struct fastrpc_init_create_static init;
> struct fastrpc_invoke_args *args;
> struct fastrpc_phy_page pages[1];
> + struct fastrpc_buf *buf = NULL;
> struct fastrpc_static_pd *spd = NULL;
> + u64 phys = 0, size = 0;
> char *name;
> int err;
> + bool scm_done = false;
> struct {
> int pgid;
> u32 namelen;
> @@ -1330,25 +1381,26 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_name;
> }
> fl->spd = spd;
> - if (!fl->cctx->remote_heap) {
> - err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> - &fl->cctx->remote_heap);
> + if (list_empty(&spd->rmaps)) {
> + err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &buf);
> if (err)
> goto err_name;
>
> + phys = buf->phys;
> + size = buf->size;
> /* Map if we have any heap VMIDs associated with this ADSP Static Process. */
> if (fl->cctx->vmcount) {
> u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>
> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
> - (u64)fl->cctx->remote_heap->size,
> + err = qcom_scm_assign_mem(phys, (u64)size,
> &src_perms,
> fl->cctx->vmperms, fl->cctx->vmcount);
> if (err) {
> dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> + phys, size, err);
> goto err_map;
> }
> + scm_done = true;
> }
> }
>
> @@ -1365,8 +1417,8 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> args[1].length = inbuf.namelen;
> args[1].fd = -1;
>
> - pages[0].addr = fl->cctx->remote_heap->phys;
> - pages[0].size = fl->cctx->remote_heap->size;
> + pages[0].addr = phys;
> + pages[0].size = size;
>
> args[2].ptr = (u64)(uintptr_t) pages;
> args[2].length = sizeof(*pages);
> @@ -1380,10 +1432,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_invoke;
>
> kfree(args);
> + kfree(name);
> +
> + if (buf) {
> + spin_lock(&fl->lock);
> + list_add_tail(&buf->node, &spd->rmaps);
> + spin_unlock(&fl->lock);
> + }
>
> return 0;
> err_invoke:
> - if (fl->cctx->vmcount) {
> + if (fl->cctx->vmcount && scm_done) {
> u64 src_perms = 0;
> struct qcom_scm_vmperm dst_perms;
> u32 i;
> @@ -1393,15 +1452,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>
> dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> dst_perms.perm = QCOM_SCM_PERM_RWX;
> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
> - (u64)fl->cctx->remote_heap->size,
> + err = qcom_scm_assign_mem(phys, (u64)size,
> &src_perms, &dst_perms, 1);
> if (err)
> dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> + phys, size, err);
> }
> err_map:
> - fastrpc_buf_free(fl->cctx->remote_heap);
> + if (buf)
> + fastrpc_buf_free(buf);
> err_name:
> kfree(name);
> err:
> @@ -1866,17 +1925,16 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> return 0;
> }
>
> -static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
> +static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size)
> {
> struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
> struct fastrpc_munmap_req_msg req_msg;
> - struct device *dev = fl->sctx->dev;
> int err;
> u32 sc;
>
> req_msg.pgid = fl->tgid;
> - req_msg.size = buf->size;
> - req_msg.vaddr = buf->raddr;
> + req_msg.size = size;
> + req_msg.vaddr = raddr;
>
> args[0].ptr = (u64) (uintptr_t) &req_msg;
> args[0].length = sizeof(req_msg);
> @@ -1884,11 +1942,38 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
> err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> &args[0]);
> +
> + return err;
> +}
> +
> +static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
> +{
> + struct device *dev = fl->sctx->dev;
> + int err;
> +
> + err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size);
> if (!err) {
> + if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> + if (fl->cctx->vmcount) {
> + u64 src_perms = 0;
> + struct qcom_scm_vmperm dst_perms;
> + u32 i;
> +
> + for (i = 0; i < fl->cctx->vmcount; i++)
> + src_perms |= BIT(fl->cctx->vmperms[i].vmid);
> +
> + dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> + dst_perms.perm = QCOM_SCM_PERM_RWX;
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, &dst_perms, 1);
> + if (err) {
> + dev_err(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + buf->phys, buf->size, err);
> + return err;
> + }
> + }
> + }
> dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
> - spin_lock(&fl->lock);
> - list_del(&buf->node);
> - spin_unlock(&fl->lock);
> fastrpc_buf_free(buf);
> } else {
> dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
> @@ -1902,6 +1987,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 device *dev = fl->sctx->dev;
> + int err = 0;
>
> if (copy_from_user(&req, argp, sizeof(req)))
> return -EFAULT;
> @@ -1910,20 +1996,48 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
> list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
> if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
> buf = iter;
> + list_del(&buf->node);
> break;
> }
> }
> spin_unlock(&fl->lock);
>
> - if (!buf) {
> - dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
> - req.vaddrout, req.size);
> - return -EINVAL;
> + if (buf) {
> + err = fastrpc_req_munmap_impl(fl, buf);
> + if (err) {
> + spin_lock(&fl->lock);
> + list_add_tail(&buf->node, &fl->mmaps);
> + spin_unlock(&fl->lock);
> + }
> + return err;
> }
>
> - return fastrpc_req_munmap_impl(fl, buf);
> + spin_lock(&fl->lock);
> + if (fl->spd) {
> + list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) {
> + if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
> + buf = iter;
> + list_del(&buf->node);
> + break;
> + }
> + }
> + }
> + spin_unlock(&fl->lock);
> + if (buf) {
> + err = fastrpc_req_munmap_impl(fl, buf);
> + if (err) {
> + spin_lock(&fl->lock);
> + list_add_tail(&buf->node, &fl->spd->rmaps);
> + spin_unlock(&fl->lock);
> + }
> + return err;
> + }
> + dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
> + req.vaddrout, req.size);
> + return -EINVAL;
> }
>
> +
> static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> {
> struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
> @@ -1950,16 +2064,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> return -EINVAL;
> }
>
> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> + if (!fl->spd || !fl->spd->ispdup) {
> + dev_err(dev, "remote heap request supported only for active static PD\n");
> + return -EINVAL;
> + }
> err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
> - else
> + } else {
> err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
> + }
>
> if (err) {
> dev_err(dev, "failed to allocate buffer\n");
> return err;
> }
> + buf->flag = req.flags;
>
> + /* Add memory to static PD pool, protection through hypervisor */
> + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
> + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
> + if (err) {
> + dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + buf->phys, buf->size, err);
> + goto err_invoke;
> + }
> + }
> req_msg.pgid = fl->tgid;
> req_msg.flags = req.flags;
> req_msg.vaddr = req.vaddrin;
> @@ -1991,26 +2123,17 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> /* let the client know the address to use */
> req.vaddrout = rsp_msg.vaddr;
>
> - /* Add memory to static PD pool, protection thru hypervisor */
> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
> - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> -
> - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
> - if (err) {
> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
> - buf->phys, buf->size, err);
> - goto err_assign;
> - }
> - }
>
> spin_lock(&fl->lock);
> - list_add_tail(&buf->node, &fl->mmaps);
> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> + list_add_tail(&buf->node, &fl->spd->rmaps);
> + else
> + list_add_tail(&buf->node, &fl->mmaps);
> spin_unlock(&fl->lock);
>
> if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
> err = -EFAULT;
> - goto err_assign;
> + goto err_copy;
> }
>
> dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> @@ -2018,14 +2141,19 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>
> return 0;
>
> -err_assign:
> +err_copy:
> + spin_lock(&fl->lock);
> + list_del(&buf->node);
> + spin_unlock(&fl->lock);
> fastrpc_req_munmap_impl(fl, buf);
> + buf = NULL;
> err_invoke:
> fastrpc_buf_free(buf);
>
> return err;
> }
>
> +
> static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
> {
> struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
> @@ -2249,6 +2377,7 @@ static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> spd->servloc_name,
> domains[cctx->domain_id]);
> spd->ispdup = false;
> + fastrpc_mmap_remove_pdr(spd);
> fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> break;
> case SERVREG_SERVICE_STATE_UP:
> @@ -2400,6 +2529,7 @@ static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char
> cctx->spd[spd_session].servloc_name = client_name;
> cctx->spd[spd_session].spdname = service_path;
> cctx->spd[spd_session].cctx = cctx;
> + INIT_LIST_HEAD(&cctx->spd[spd_session].rmaps);
> service = pdr_add_lookup(handle, service_name, service_path);
> if (IS_ERR(service)) {
> err = PTR_ERR(service);
> @@ -2566,9 +2696,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
> list_del(&buf->node);
>
> - if (cctx->remote_heap)
> - fastrpc_buf_free(cctx->remote_heap);
> -
> for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> if (cctx->spd[i].pdrhandle)
> pdr_handle_release(cctx->spd[i].pdrhandle);
> @@ -2576,6 +2703,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>
> of_platform_depopulate(&rpdev->dev);
>
> + fastrpc_mmap_remove_ssr(cctx);
> +
> fastrpc_channel_ctx_put(cctx);
> }
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:50:24PM +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.
>
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 221 ++++++++++++++++++++++++++---------------
> 1 file changed, 141 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index ef9bbd8c3dd1..d9d9f889e39e 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)
> #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,10 @@ 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)) {
> +
Nit: extra empty line. Please drop it if you are going to send next
iteration.
> + if (is_session_rejected(fl, fl->is_unsigned_pd)) {
> err = -ECONNREFUSED;
> goto err;
> }
> @@ -1986,6 +1987,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;
>
> @@ -2032,76 +2034,49 @@ 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);
> + else
> + fastrpc_map_put(map);
>
> -static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> + return err;
> +}
> +
> +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;
> -
> - 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);
> -
> - return -EINVAL;
> - }
> -
> - if (req.vaddrin) {
> - dev_err(dev, "adding user allocated pages is not supported\n");
> - return -EINVAL;
> - }
> -
> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> - if (!fl->spd || !fl->spd->ispdup) {
> - dev_err(dev, "remote heap request supported only for active static PD\n");
> - return -EINVAL;
> - }
> - err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
> - } else {
> - err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
> - }
> -
> - if (err) {
> - dev_err(dev, "failed to allocate buffer\n");
> - return err;
> - }
> - buf->flag = req.flags;
> -
> - /* Add memory to static PD pool, protection through hypervisor */
> - if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
> - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> -
> - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
> - if (err) {
> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> - buf->phys, buf->size, err);
> - goto err_invoke;
> - }
> - }
> req_msg.pgid = fl->tgid;
> - req_msg.flags = req.flags;
> - req_msg.vaddr = req.vaddrin;
> + req_msg.flags = flag;
> + req_msg.vaddr = 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;
> + pages.addr = phys;
> + pages.size = size;
>
> args[1].ptr = (u64) (uintptr_t) &pages;
> args[1].length = sizeof(pages);
> @@ -2111,49 +2086,135 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>
> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
> err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> - &args[0]);
> + &args[0]);
> +
> if (err) {
> - dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
> - goto err_invoke;
> + dev_err(fl->sctx->dev, "mmap error (len 0x%08llx)\n", size);
> + return err;
> }
> + *raddr = rsp_msg.vaddr;
>
> - /* update the buffer to be able to deallocate the memory on the DSP */
> - buf->raddr = (uintptr_t) rsp_msg.vaddr;
> + return err;
> +}
>
> - /* let the client know the address to use */
> - req.vaddrout = rsp_msg.vaddr;
> +static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> +{
> + struct fastrpc_buf *buf = NULL;
> + struct fastrpc_req_mmap req;
> + struct fastrpc_map *map = NULL;
> + struct device *dev = fl->sctx->dev;
> + u64 raddr = 0;
> + int err;
>
> + if (copy_from_user(&req, argp, sizeof(req)))
> + return -EFAULT;
>
> - spin_lock(&fl->lock);
> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> - list_add_tail(&buf->node, &fl->spd->rmaps);
> - else
> - list_add_tail(&buf->node, &fl->mmaps);
> - spin_unlock(&fl->lock);
> + if ((req.flags == ADSP_MMAP_ADD_PAGES ||
> + req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) {
Please indent to the opening bracket rather than with the tab.
Also, looking at the funciton and at the error path, this almost looks
like two different functions. Could you please split it?
> + if (req.vaddrin) {
> + dev_err(dev, "adding user allocated pages is not supported for signed PD\n");
> + return -EINVAL;
> + }
> +
> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> + if (!fl->spd || !fl->spd->ispdup) {
> + dev_err(dev, "remote heap request supported only for active static PD\n");
> + return -EINVAL;
> + }
> + err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
> + } else {
> + err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
> + }
> +
> + if (err) {
> + dev_err(dev, "failed to allocate buffer\n");
> + return err;
> + }
> + buf->flag = req.flags;
> +
> + /* Add memory to static PD pool, protection through hypervisor */
> + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
> + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
> + if (err) {
> + dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + buf->phys, buf->size, err);
> + goto err_invoke;
> + }
> + }
> +
> + 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) raddr;
> +
> + /* let the client know the address to use */
> + req.vaddrout = raddr;
>
> + spin_lock(&fl->lock);
> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> + list_add_tail(&buf->node, &fl->spd->rmaps);
> + else
> + 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);
> + } else {
> + 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;
> }
>
> - 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);
> - spin_unlock(&fl->lock);
> - fastrpc_req_munmap_impl(fl, buf);
> - buf = NULL;
> + if ((req.flags != ADSP_MMAP_ADD_PAGES &&
> + req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) || fl->is_unsigned_pd) {
> + fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
> + } else {
> + spin_lock(&fl->lock);
> + list_del(&buf->node);
> + spin_unlock(&fl->lock);
> + fastrpc_req_munmap_impl(fl, buf);
> + buf = NULL;
> + }
> err_invoke:
> - fastrpc_buf_free(buf);
> + if (map)
> + fastrpc_map_put(map);
> + if (buf)
> + fastrpc_buf_free(buf);
>
> return err;
> }
>
> -
> 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
On Thu, May 30, 2024 at 03:50:25PM +0530, Ekansh Gupta wrote:
> Untrusted application with access to only non-secure fastrpc device
> node can attach to root_pd or static PDs if it can make the respective
> init request. This can cause problems as the untrusted application
> can send bad requests to root_pd or static PDs. Add changes to reject
> attach to privileged PDs if the request is being made using non-secure
> fastrpc device node.
>
> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:50:26PM +0530, Ekansh Gupta wrote:
> Some untrusted applications will not have access to open fastrpc
> device nodes and a privileged process can open the device node on
> behalf of the application. Add a check to restrict such untrusted
> applications from offloading to signed PD.
>
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 73fa0e536cf9..32615ccde7ac 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -328,6 +328,7 @@ struct fastrpc_user {
> int pd;
> bool is_secure_dev;
> bool is_unsigned_pd;
> + bool untrusted_process;
> char *servloc_name;
> /* Lock for lists */
> spinlock_t lock;
> @@ -1249,13 +1250,17 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> * channel is configured as secure and block untrusted apps on channel
> * that does not support unsigned PD offload
> */
> - if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
> - dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
> - return true;
> - }
> + if (!fl->cctx->unsigned_support || !unsigned_pd_request)
> + goto reject_session;
> }
> + /* Check if untrusted process is trying to offload to signed PD */
> + if (fl->untrusted_process && !unsigned_pd_request)
> + goto reject_session;
>
> return false;
> +reject_session:
> + dev_dbg(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
> + return true;
> }
>
> static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
> @@ -1504,12 +1509,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> goto err;
> }
>
> + /*
> + * Third-party apps don't have permission to open the fastrpc device, so
Permissions depend on the end-user setup. Is it going to break if the
user sets 0666 mode for fastrpc nodes?
> + * it is opened on their behalf by a priveleged process. This is detected
> + * by comparing current PID with the one stored during device open.
> + */
> + if (current->tgid != fl->tgid)
> + fl->untrusted_process = true;
If the comment talks about PIDs, when why are you comparing GIDs here?
> +
> if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
> fl->is_unsigned_pd = true;
>
>
> if (is_session_rejected(fl, fl->is_unsigned_pd)) {
> - err = -ECONNREFUSED;
> + err = -EACCES;
> goto err;
> }
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:50:27PM +0530, Ekansh Gupta wrote:
> Trusted CPU applications currently offload to signed PDs on CDSP to
> gain some additional services provided by root PD. Unsigned PDs have
> access to limited root PD services that may not be sufficient for
> all use-cases. Signed PDs have a higher dynamic loading latency
> which impacts the performance of applications. Limited root PD
> services could be opened up for unsigned PDs but that should be
> restricted for untrusted processes. For this requirement, System
> unsigned PD is introduced which will be same as Unsigned PD for
> most part but will have access to more root PD services. Add
> changes to offload trusted applications to System unsigned PD
> when unsigned offload is requested.
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 27 +++++++++++++++++++++------
> include/uapi/misc/fastrpc.h | 2 ++
> 2 files changed, 23 insertions(+), 6 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 30/05/2024 11:20, Ekansh Gupta wrote:
> Incorrect remote arguments are getting passed when requesting for
> capabilities from DSP. Also there is no requirement to update the
> PD type as it might cause problems for any PD other than user PD.
> In addition to this, the collected capability information is not
> getting copied properly to user. Add changes to address these
> problems and get correct DSP capabilities.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 4028cb96bcf2..61389795f498 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
> args[0].length = sizeof(dsp_attr_buf_len);
> args[0].fd = -1;
> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> - args[1].length = dsp_attr_buf_len;
> + args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
This does not look correct,
we have allocated buffer of size FASTRPC_MAX_DSP_ATTRIBUTES_LEN which is
already (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES)
now this patch multiplies with again sizeof(uint32_t), this is going to
send dsp incorrect size for buffer and overrun the buffer size.
> args[1].fd = -1;
> - fl->pd = USER_PD;
another patch may be.
>
> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> FASTRPC_SCALARS(0, 1, 1), args);
> @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> if (!dsp_attributes)
> return -ENOMEM;
>
> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
You change this again to send FASTRPC_MAX_DSP_ATTRIBUTES instead of
FASTRPC_MAX_DSP_ATTRIBUTES_LEN but why?
> if (err == DSP_UNSUPPORTED_API) {
> dev_info(&cctx->rpdev->dev,
> "Warning: DSP capabilities not supported on domain: %d\n", domain);
> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> if (err)
> return err;
>
> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
> + if (copy_to_user(argp, &cap, sizeof(cap)))
Why are we copying the full struct here? All that user needs is
cap.capability?
--srini
> return -EFAULT;
>
> return 0;
On 30/05/2024 11:20, Ekansh Gupta wrote:
> DSP capabilities request is sending bad size to utilities skel
What you exactly mean by this?
Curretly driver is sending 1024 bytes of buffer, why is DSP not happy
with this size?
> call which is resulting in memory corruption. Pass proper size
What does proper size mean?
> to avoid the corruption.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 61389795f498..3e1ab58038ed 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1695,6 +1695,7 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>
> /* Capability filled in userspace */
> dsp_attr_buf[0] = 0;
> + dsp_attr_buf_len -= 1;
is DSP expecting 255 *4 bytes instead of 256 *4?
--srini
>
> args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
> args[0].length = sizeof(dsp_attr_buf_len);
On 30/05/2024 11:20, Ekansh Gupta wrote:
> Untrusted application with access to only non-secure fastrpc device
> node can attach to root_pd or static PDs if it can make the respective
> init request. This can cause problems as the untrusted application
> can send bad requests to root_pd or static PDs. Add changes to reject
> attach to privileged PDs if the request is being made using non-secure
> fastrpc device node.
>
> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d9d9f889e39e..73fa0e536cf9 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1344,6 +1344,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> } inbuf;
> u32 sc;
>
> + if (!fl->is_secure_dev) {
> + dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
> + return -EACCES;
> + }
Please move these checks to fastrpc_device_ioctl which makes it clear
that these are only supported with secure device nodes.
I would also prefer this to be documented in the the uapi headers.
--srini
> +
> args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> if (!args)
> return -ENOMEM;
> @@ -1769,6 +1774,11 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> int tgid = fl->tgid;
> u32 sc;
>
> + if (!fl->is_secure_dev) {
> + dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
> + return -EACCES;
> + }
> +
> args[0].ptr = (u64)(uintptr_t) &tgid;
> args[0].length = sizeof(tgid);
> args[0].fd = -1;
On 5/30/2024 4:29 PM, Dmitry Baryshkov wrote:
> On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote:
>> Incorrect remote arguments are getting passed when requesting for
>> capabilities from DSP.
> Describe why and how they are incorrect.
Sure, I'll update this information in the next spin.
>
>> Also there is no requirement to update the
>> PD type as it might cause problems for any PD other than user PD.
> Also... means that these are two separate issues. There should be two
> separate commits.
Okay, I'll separate out the PD type change.
>
>> In addition to this, the collected capability information is not
>> getting copied properly to user. Add changes to address these
>> problems and get correct DSP capabilities.
>>
>> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
>> Cc: stable <[email protected]>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/fastrpc.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 4028cb96bcf2..61389795f498 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>> args[0].length = sizeof(dsp_attr_buf_len);
>> args[0].fd = -1;
>> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
>> - args[1].length = dsp_attr_buf_len;
>> + args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
> As you are skipping first entry, should there be (dsp_attr_buf_len - 1)
> * sizeof(uint32_t).
This was done in the next patch of the series, I'll bring it here.
>
>> args[1].fd = -1;
>> - fl->pd = USER_PD;
>>
>> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
>> FASTRPC_SCALARS(0, 1, 1), args);
>> @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
>> if (!dsp_attributes)
>> return -ENOMEM;
>>
>> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
>> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
> So it looks like the argument was correct. It was passing length, not
> the number of attributes. The only thing to fix is that args[1].length
> should be dsp_attr_buf_len - sizeof(*dsp_attr_buf).
args[0] is expected to carry the information about the total number of attributes to be copied from DSP
and not the information about the size to be copied. Passing the size information leads to a failure
suggesting bad arguments passed to DSP.
>
>> if (err == DSP_UNSUPPORTED_API) {
>> dev_info(&cctx->rpdev->dev,
>> "Warning: DSP capabilities not supported on domain: %d\n", domain);
>> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>> if (err)
>> return err;
>>
>> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
>> + if (copy_to_user(argp, &cap, sizeof(cap)))
>> return -EFAULT;
>>
>> return 0;
>> --
>> 2.43.0
>>
On 5/31/2024 3:06 PM, Srinivas Kandagatla wrote:
>
>
> On 30/05/2024 11:20, Ekansh Gupta wrote:
>> DSP capabilities request is sending bad size to utilities skel
> What you exactly mean by this?
>
> Curretly driver is sending 1024 bytes of buffer, why is DSP not happy
> with this size?
Copying the comment sent to Dmitry's queries:
args[0] is expected to carry the information about the total number of attributes to be copied from DSP
and not the information about the size to be copied. Passing the size information leads to a failure
suggesting bad arguments passed to DSP.
>
>> call which is resulting in memory corruption. Pass proper size
> What does proper size mean?
args[1] is not carrying the information for full 256 sized array. It's sending the input argument as the first
index of the array, &dsp_attr_buf[1](resulting in total 255 size req), so sending 256 length will result in
copying of information out of the array which would result in problems
>> to avoid the corruption.
>>
>> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP
>> capabilities")
>> Cc: stable <[email protected]>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/fastrpc.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 61389795f498..3e1ab58038ed 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1695,6 +1695,7 @@ static int fastrpc_get_info_from_dsp(struct
>> fastrpc_user *fl, uint32_t *dsp_attr
>> /* Capability filled in userspace */
>> dsp_attr_buf[0] = 0;
>> + dsp_attr_buf_len -= 1;
>
> is DSP expecting 255 *4 bytes instead of 256 *4?
DSP is expecting the information about the number of attributes to be
updated, i.e., 255.
>
> --srini
>
>> args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
>> args[0].length = sizeof(dsp_attr_buf_len);
On 5/30/2024 4:44 PM, Dmitry Baryshkov wrote:
> On Thu, May 30, 2024 at 03:50:23PM +0530, Ekansh Gupta wrote:
>> Current remote heap design comes with problems where all types of
>> buffers are getting added to interrupted list and also user unmap
>> request is not handling the request to unmap remote heap buffer
>> type. Add changes to maintain list in a way that it can be used to
>> to support remote heap grow/shrink request and the remote heap
>> buffers cleanup during static PD restart.
> This commit seems to contain several fixes squashed into one. You have
> heap management, scm_done fix, missing SCM calls on rpmsg removal and
> maybe several other fixes. Please separate them so that each commit
> fixes just one issue in an atomic way.
Sure, I'll split this patch in the next spin.
>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/fastrpc.c | 221 ++++++++++++++++++++++++++++++++---------
>> 1 file changed, 175 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index d115860fc356..ef9bbd8c3dd1 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -210,6 +210,7 @@ struct fastrpc_buf {
>> struct dma_buf *dmabuf;
>> struct device *dev;
>> void *virt;
>> + u32 flag;
>> u64 phys;
>> u64 size;
>> /* Lock for dma buf attachments */
>> @@ -274,6 +275,7 @@ struct fastrpc_session_ctx {
>> };
>>
>> struct fastrpc_static_pd {
>> + struct list_head rmaps;
>> char *servloc_name;
>> char *spdname;
>> void *pdrhandle;
>> @@ -299,7 +301,6 @@ struct fastrpc_channel_ctx {
>> u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>> struct fastrpc_device *secure_fdevice;
>> struct fastrpc_device *fdevice;
>> - struct fastrpc_buf *remote_heap;
>> struct list_head invoke_interrupted_mmaps;
>> bool secure;
>> bool unsigned_support;
>> @@ -1256,6 +1257,53 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>> return false;
>> }
>>
>> +static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
>> +{
>> + struct fastrpc_buf *buf, *b;
>> + struct fastrpc_channel_ctx *cctx;
>> + int err;
>> +
>> + if (!spd || !spd->fl)
>> + return;
>> +
>> + cctx = spd->cctx;
>> +
>> + spin_lock(&spd->fl->lock);
>> + list_for_each_entry_safe(buf, b, &spd->rmaps, node) {
>> + list_del(&buf->node);
>> + spin_unlock(&spd->fl->lock);
>> + if (cctx->vmcount) {
>> + u64 src_perms = 0;
>> + struct qcom_scm_vmperm dst_perms;
>> + u32 i;
>> +
>> + for (i = 0; i < cctx->vmcount; i++)
>> + src_perms |= BIT(cctx->vmperms[i].vmid);
>> +
>> + dst_perms.vmid = QCOM_SCM_VMID_HLOS;
>> + dst_perms.perm = QCOM_SCM_PERM_RWX;
>> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> + &src_perms, &dst_perms, 1);
>> + if (err) {
>> + pr_err("%s: Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
>> + __func__, buf->phys, buf->size, err);
>> + return;
>> + }
>> + }
>> + fastrpc_buf_free(buf);
>> + spin_lock(&spd->fl->lock);
>> + }
>> + spin_unlock(&spd->fl->lock);
>> +}
>> +
>> +static void fastrpc_mmap_remove_ssr(struct fastrpc_channel_ctx *cctx)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < FASTRPC_MAX_SPD; i++)
>> + fastrpc_mmap_remove_pdr(&cctx->spd[i]);
>> +}
>> +
>> static struct fastrpc_static_pd *fastrpc_get_spd_session(
>> struct fastrpc_user *fl)
>> {
>> @@ -1282,9 +1330,12 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> struct fastrpc_init_create_static init;
>> struct fastrpc_invoke_args *args;
>> struct fastrpc_phy_page pages[1];
>> + struct fastrpc_buf *buf = NULL;
>> struct fastrpc_static_pd *spd = NULL;
>> + u64 phys = 0, size = 0;
>> char *name;
>> int err;
>> + bool scm_done = false;
>> struct {
>> int pgid;
>> u32 namelen;
>> @@ -1330,25 +1381,26 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> goto err_name;
>> }
>> fl->spd = spd;
>> - if (!fl->cctx->remote_heap) {
>> - err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>> - &fl->cctx->remote_heap);
>> + if (list_empty(&spd->rmaps)) {
>> + err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &buf);
>> if (err)
>> goto err_name;
>>
>> + phys = buf->phys;
>> + size = buf->size;
>> /* Map if we have any heap VMIDs associated with this ADSP Static Process. */
>> if (fl->cctx->vmcount) {
>> u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>
>> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>> - (u64)fl->cctx->remote_heap->size,
>> + err = qcom_scm_assign_mem(phys, (u64)size,
>> &src_perms,
>> fl->cctx->vmperms, fl->cctx->vmcount);
>> if (err) {
>> dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
>> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
>> + phys, size, err);
>> goto err_map;
>> }
>> + scm_done = true;
>> }
>> }
>>
>> @@ -1365,8 +1417,8 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> args[1].length = inbuf.namelen;
>> args[1].fd = -1;
>>
>> - pages[0].addr = fl->cctx->remote_heap->phys;
>> - pages[0].size = fl->cctx->remote_heap->size;
>> + pages[0].addr = phys;
>> + pages[0].size = size;
>>
>> args[2].ptr = (u64)(uintptr_t) pages;
>> args[2].length = sizeof(*pages);
>> @@ -1380,10 +1432,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> goto err_invoke;
>>
>> kfree(args);
>> + kfree(name);
>> +
>> + if (buf) {
>> + spin_lock(&fl->lock);
>> + list_add_tail(&buf->node, &spd->rmaps);
>> + spin_unlock(&fl->lock);
>> + }
>>
>> return 0;
>> err_invoke:
>> - if (fl->cctx->vmcount) {
>> + if (fl->cctx->vmcount && scm_done) {
>> u64 src_perms = 0;
>> struct qcom_scm_vmperm dst_perms;
>> u32 i;
>> @@ -1393,15 +1452,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>
>> dst_perms.vmid = QCOM_SCM_VMID_HLOS;
>> dst_perms.perm = QCOM_SCM_PERM_RWX;
>> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>> - (u64)fl->cctx->remote_heap->size,
>> + err = qcom_scm_assign_mem(phys, (u64)size,
>> &src_perms, &dst_perms, 1);
>> if (err)
>> dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
>> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
>> + phys, size, err);
>> }
>> err_map:
>> - fastrpc_buf_free(fl->cctx->remote_heap);
>> + if (buf)
>> + fastrpc_buf_free(buf);
>> err_name:
>> kfree(name);
>> err:
>> @@ -1866,17 +1925,16 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>> return 0;
>> }
>>
>> -static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
>> +static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size)
>> {
>> struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
>> struct fastrpc_munmap_req_msg req_msg;
>> - struct device *dev = fl->sctx->dev;
>> int err;
>> u32 sc;
>>
>> req_msg.pgid = fl->tgid;
>> - req_msg.size = buf->size;
>> - req_msg.vaddr = buf->raddr;
>> + req_msg.size = size;
>> + req_msg.vaddr = raddr;
>>
>> args[0].ptr = (u64) (uintptr_t) &req_msg;
>> args[0].length = sizeof(req_msg);
>> @@ -1884,11 +1942,38 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
>> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
>> err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
>> &args[0]);
>> +
>> + return err;
>> +}
>> +
>> +static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
>> +{
>> + struct device *dev = fl->sctx->dev;
>> + int err;
>> +
>> + err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size);
>> if (!err) {
>> + if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) {
>> + if (fl->cctx->vmcount) {
>> + u64 src_perms = 0;
>> + struct qcom_scm_vmperm dst_perms;
>> + u32 i;
>> +
>> + for (i = 0; i < fl->cctx->vmcount; i++)
>> + src_perms |= BIT(fl->cctx->vmperms[i].vmid);
>> +
>> + dst_perms.vmid = QCOM_SCM_VMID_HLOS;
>> + dst_perms.perm = QCOM_SCM_PERM_RWX;
>> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> + &src_perms, &dst_perms, 1);
>> + if (err) {
>> + dev_err(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
>> + buf->phys, buf->size, err);
>> + return err;
>> + }
>> + }
>> + }
>> dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
>> - spin_lock(&fl->lock);
>> - list_del(&buf->node);
>> - spin_unlock(&fl->lock);
>> fastrpc_buf_free(buf);
>> } else {
>> dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
>> @@ -1902,6 +1987,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 device *dev = fl->sctx->dev;
>> + int err = 0;
>>
>> if (copy_from_user(&req, argp, sizeof(req)))
>> return -EFAULT;
>> @@ -1910,20 +1996,48 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>> list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
>> if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
>> buf = iter;
>> + list_del(&buf->node);
>> break;
>> }
>> }
>> spin_unlock(&fl->lock);
>>
>> - if (!buf) {
>> - dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
>> - req.vaddrout, req.size);
>> - return -EINVAL;
>> + if (buf) {
>> + err = fastrpc_req_munmap_impl(fl, buf);
>> + if (err) {
>> + spin_lock(&fl->lock);
>> + list_add_tail(&buf->node, &fl->mmaps);
>> + spin_unlock(&fl->lock);
>> + }
>> + return err;
>> }
>>
>> - return fastrpc_req_munmap_impl(fl, buf);
>> + spin_lock(&fl->lock);
>> + if (fl->spd) {
>> + list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) {
>> + if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
>> + buf = iter;
>> + list_del(&buf->node);
>> + break;
>> + }
>> + }
>> + }
>> + spin_unlock(&fl->lock);
>> + if (buf) {
>> + err = fastrpc_req_munmap_impl(fl, buf);
>> + if (err) {
>> + spin_lock(&fl->lock);
>> + list_add_tail(&buf->node, &fl->spd->rmaps);
>> + spin_unlock(&fl->lock);
>> + }
>> + return err;
>> + }
>> + dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
>> + req.vaddrout, req.size);
>> + return -EINVAL;
>> }
>>
>> +
>> static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> {
>> struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
>> @@ -1950,16 +2064,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> return -EINVAL;
>> }
>>
>> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
>> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
>> + if (!fl->spd || !fl->spd->ispdup) {
>> + dev_err(dev, "remote heap request supported only for active static PD\n");
>> + return -EINVAL;
>> + }
>> err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
>> - else
>> + } else {
>> err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
>> + }
>>
>> if (err) {
>> dev_err(dev, "failed to allocate buffer\n");
>> return err;
>> }
>> + buf->flag = req.flags;
>>
>> + /* Add memory to static PD pool, protection through hypervisor */
>> + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
>> + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>> +
>> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> + &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>> + if (err) {
>> + dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
>> + buf->phys, buf->size, err);
>> + goto err_invoke;
>> + }
>> + }
>> req_msg.pgid = fl->tgid;
>> req_msg.flags = req.flags;
>> req_msg.vaddr = req.vaddrin;
>> @@ -1991,26 +2123,17 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> /* let the client know the address to use */
>> req.vaddrout = rsp_msg.vaddr;
>>
>> - /* Add memory to static PD pool, protection thru hypervisor */
>> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
>> - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>> -
>> - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>> - if (err) {
>> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>> - buf->phys, buf->size, err);
>> - goto err_assign;
>> - }
>> - }
>>
>> spin_lock(&fl->lock);
>> - list_add_tail(&buf->node, &fl->mmaps);
>> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
>> + list_add_tail(&buf->node, &fl->spd->rmaps);
>> + else
>> + list_add_tail(&buf->node, &fl->mmaps);
>> spin_unlock(&fl->lock);
>>
>> if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>> err = -EFAULT;
>> - goto err_assign;
>> + goto err_copy;
>> }
>>
>> dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
>> @@ -2018,14 +2141,19 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>
>> return 0;
>>
>> -err_assign:
>> +err_copy:
>> + spin_lock(&fl->lock);
>> + list_del(&buf->node);
>> + spin_unlock(&fl->lock);
>> fastrpc_req_munmap_impl(fl, buf);
>> + buf = NULL;
>> err_invoke:
>> fastrpc_buf_free(buf);
>>
>> return err;
>> }
>>
>> +
>> static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
>> {
>> struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
>> @@ -2249,6 +2377,7 @@ static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
>> spd->servloc_name,
>> domains[cctx->domain_id]);
>> spd->ispdup = false;
>> + fastrpc_mmap_remove_pdr(spd);
>> fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
>> break;
>> case SERVREG_SERVICE_STATE_UP:
>> @@ -2400,6 +2529,7 @@ static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char
>> cctx->spd[spd_session].servloc_name = client_name;
>> cctx->spd[spd_session].spdname = service_path;
>> cctx->spd[spd_session].cctx = cctx;
>> + INIT_LIST_HEAD(&cctx->spd[spd_session].rmaps);
>> service = pdr_add_lookup(handle, service_name, service_path);
>> if (IS_ERR(service)) {
>> err = PTR_ERR(service);
>> @@ -2566,9 +2696,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
>> list_del(&buf->node);
>>
>> - if (cctx->remote_heap)
>> - fastrpc_buf_free(cctx->remote_heap);
>> -
>> for (i = 0; i < FASTRPC_MAX_SPD; i++) {
>> if (cctx->spd[i].pdrhandle)
>> pdr_handle_release(cctx->spd[i].pdrhandle);
>> @@ -2576,6 +2703,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>>
>> of_platform_depopulate(&rpdev->dev);
>>
>> + fastrpc_mmap_remove_ssr(cctx);
>> +
>> fastrpc_channel_ctx_put(cctx);
>> }
>>
>> --
>> 2.43.0
>>
On 5/31/2024 5:19 AM, Dmitry Baryshkov wrote:
> On Thu, May 30, 2024 at 03:50:26PM +0530, Ekansh Gupta wrote:
>> Some untrusted applications will not have access to open fastrpc
>> device nodes and a privileged process can open the device node on
>> behalf of the application. Add a check to restrict such untrusted
>> applications from offloading to signed PD.
>>
>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>> Cc: stable <[email protected]>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/fastrpc.c | 23 ++++++++++++++++++-----
>> 1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 73fa0e536cf9..32615ccde7ac 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -328,6 +328,7 @@ struct fastrpc_user {
>> int pd;
>> bool is_secure_dev;
>> bool is_unsigned_pd;
>> + bool untrusted_process;
>> char *servloc_name;
>> /* Lock for lists */
>> spinlock_t lock;
>> @@ -1249,13 +1250,17 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>> * channel is configured as secure and block untrusted apps on channel
>> * that does not support unsigned PD offload
>> */
>> - if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
>> - dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
>> - return true;
>> - }
>> + if (!fl->cctx->unsigned_support || !unsigned_pd_request)
>> + goto reject_session;
>> }
>> + /* Check if untrusted process is trying to offload to signed PD */
>> + if (fl->untrusted_process && !unsigned_pd_request)
>> + goto reject_session;
>>
>> return false;
>> +reject_session:
>> + dev_dbg(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
>> + return true;
>> }
>>
>> static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
>> @@ -1504,12 +1509,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>> goto err;
>> }
>>
>> + /*
>> + * Third-party apps don't have permission to open the fastrpc device, so
> Permissions depend on the end-user setup. Is it going to break if the
> user sets 0666 mode for fastrpc nodes?
If the root user sets 0666 for fastrpc nodes, it is expected that this check will get bypassed.
>
>> + * it is opened on their behalf by a priveleged process. This is detected
>> + * by comparing current PID with the one stored during device open.
>> + */
>> + if (current->tgid != fl->tgid)
>> + fl->untrusted_process = true;
> If the comment talks about PIDs, when why are you comparing GIDs here?
It should be GID, I'll update the comment in next spin.
>
>> +
>> if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
>> fl->is_unsigned_pd = true;
>>
>>
>> if (is_session_rejected(fl, fl->is_unsigned_pd)) {
>> - err = -ECONNREFUSED;
>> + err = -EACCES;
>> goto err;
>> }
>>
>> --
>> 2.43.0
>>
On 5/31/2024 3:03 PM, Srinivas Kandagatla wrote:
>
>
> On 30/05/2024 11:20, Ekansh Gupta wrote:
>> Incorrect remote arguments are getting passed when requesting for
>> capabilities from DSP. Also there is no requirement to update the
>> PD type as it might cause problems for any PD other than user PD.
>> In addition to this, the collected capability information is not
>> getting copied properly to user. Add changes to address these
>> problems and get correct DSP capabilities.
>>
>> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
>> Cc: stable <[email protected]>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/fastrpc.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 4028cb96bcf2..61389795f498 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>> args[0].length = sizeof(dsp_attr_buf_len);
>> args[0].fd = -1;
>> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
>> - args[1].length = dsp_attr_buf_len;
>> + args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
> This does not look correct,
>
> we have allocated buffer of size FASTRPC_MAX_DSP_ATTRIBUTES_LEN which is
> already (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES)
>
> now this patch multiplies with again sizeof(uint32_t), this is going to send dsp incorrect size for buffer and overrun the buffer size.
As the argument passed to this function is number of attributes instead of length, this won't cause another multiplication with (uint32_t).
>
>
>
>> args[1].fd = -1;
>> - fl->pd = USER_PD;
> another patch may be.
Sure.
>
>> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
>> FASTRPC_SCALARS(0, 1, 1), args);
>> @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
>> if (!dsp_attributes)
>> return -ENOMEM;
>> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
>> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
>
> You change this again to send FASTRPC_MAX_DSP_ATTRIBUTES instead of FASTRPC_MAX_DSP_ATTRIBUTES_LEN but why?
Copying the comment sent to Dmitry's queries:
args[0] is expected to carry the information about the total number of attributes to be copied from DSP
and not the information about the size to be copied. Passing the size information leads to a failure
suggesting bad arguments passed to DSP.
>
>
>> if (err == DSP_UNSUPPORTED_API) {
>> dev_info(&cctx->rpdev->dev,
>> "Warning: DSP capabilities not supported on domain: %d\n", domain);
>> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>> if (err)
>> return err;
>> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
>> + if (copy_to_user(argp, &cap, sizeof(cap)))
>
> Why are we copying the full struct here? All that user needs is cap.capability?
as argp sent from user during ioctl is the capability structure, the same argp is copied to a local fastrpc_ioctl_capability structure(cap) to get the domain and attribute_id information. Copying just the capability member to argp will cause problem when the user tries to read the capability. While testing the capability, I was observing this failure and it is resolved once we copy the information properly.
>
>
>
> --srini
>
>
>> return -EFAULT;
>> return 0;
>
On Mon, Jun 03, 2024 at 12:35:40PM +0530, Ekansh Gupta wrote:
>
>
> On 5/31/2024 3:03 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 30/05/2024 11:20, Ekansh Gupta wrote:
> >> Incorrect remote arguments are getting passed when requesting for
> >> capabilities from DSP. Also there is no requirement to update the
> >> PD type as it might cause problems for any PD other than user PD.
> >> In addition to this, the collected capability information is not
> >> getting copied properly to user. Add changes to address these
> >> problems and get correct DSP capabilities.
> >>
> >> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> >> Cc: stable <[email protected]>
> >> Signed-off-by: Ekansh Gupta <[email protected]>
> >> ---
> >> ? drivers/misc/fastrpc.c | 7 +++----
> >> ? 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> >
> >> ????? if (err == DSP_UNSUPPORTED_API) {
> >> ????????? dev_info(&cctx->rpdev->dev,
> >> ?????????????? "Warning: DSP capabilities not supported on domain: %d\n", domain);
> >> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> >> ????? if (err)
> >> ????????? return err;
> >> ? -??? if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
> >> +??? if (copy_to_user(argp, &cap, sizeof(cap)))
> >
> > Why are we copying the full struct here? All that user needs is cap.capability?
> as argp sent from user during ioctl is the capability structure, the
> same argp is copied to a local fastrpc_ioctl_capability structure(cap)
> to get the domain and attribute_id information. Copying just the
> capability member to argp will cause problem when the user tries to
> read the capability. While testing the capability, I was observing
> this failure and it is resolved once we copy the information properly.
What kind of failure? Which problems? Why do we need to get all the
details from you by asking for more and more details. All this
information must be explained in the commit message.
[please wrap your lines in a some sensible way, I had to do that for you]
> >
> >
> >
> > --srini
> >
> >
> >> ????????? return -EFAULT;
> >> ? ????? return 0;
> >
>
--
With best wishes
Dmitry
On Mon, Jun 03, 2024 at 11:45:26AM +0530, Ekansh Gupta wrote:
>
> On 5/30/2024 4:29 PM, Dmitry Baryshkov wrote:
> > On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote:
> > > Incorrect remote arguments are getting passed when requesting for
> > > capabilities from DSP.
> > Describe why and how they are incorrect.
>
> Sure, I'll update this information in the next spin.
>
> >
> > > Also there is no requirement to update the
> > > PD type as it might cause problems for any PD other than user PD.
> > Also... means that these are two separate issues. There should be two
> > separate commits.
>
> Okay, I'll separate out the PD type change.
>
> >
> > > In addition to this, the collected capability information is not
> > > getting copied properly to user. Add changes to address these
> > > problems and get correct DSP capabilities.
> > >
> > > Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> > > Cc: stable <[email protected]>
> > > Signed-off-by: Ekansh Gupta <[email protected]>
> > > ---
> > > drivers/misc/fastrpc.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index 4028cb96bcf2..61389795f498 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
> > > args[0].length = sizeof(dsp_attr_buf_len);
> > > args[0].fd = -1;
> > > args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> > > - args[1].length = dsp_attr_buf_len;
> > > + args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
> > As you are skipping first entry, should there be (dsp_attr_buf_len - 1)
> > * sizeof(uint32_t).
>
> This was done in the next patch of the series, I'll bring it here.
>
> >
> > > args[1].fd = -1;
> > > - fl->pd = USER_PD;
> > > return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> > > FASTRPC_SCALARS(0, 1, 1), args);
> > > @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> > > if (!dsp_attributes)
> > > return -ENOMEM;
> > > - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
> > > + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
> > So it looks like the argument was correct. It was passing length, not
> > the number of attributes. The only thing to fix is that args[1].length
> > should be dsp_attr_buf_len - sizeof(*dsp_attr_buf).
>
> args[0] is expected to carry the information about the total number of attributes to be copied from DSP
> and not the information about the size to be copied. Passing the size information leads to a failure
> suggesting bad arguments passed to DSP.
AH, so it gets passed twice. As a pointer to u64 (for the number ofh
attributes) and as a size for those attributes (via args[1].length).
Please explain this in the commit message.
>
> >
> > > if (err == DSP_UNSUPPORTED_API) {
> > > dev_info(&cctx->rpdev->dev,
> > > "Warning: DSP capabilities not supported on domain: %d\n", domain);
> > > @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> > > if (err)
> > > return err;
> > > - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
> > > + if (copy_to_user(argp, &cap, sizeof(cap)))
> > > return -EFAULT;
> > > return 0;
> > > --
> > > 2.43.0
> > >
--
With best wishes
Dmitry
On Mon, Jun 03, 2024 at 11:57:52AM +0530, Ekansh Gupta wrote:
>
> On 5/31/2024 5:19 AM, Dmitry Baryshkov wrote:
> > On Thu, May 30, 2024 at 03:50:26PM +0530, Ekansh Gupta wrote:
> > > Some untrusted applications will not have access to open fastrpc
> > > device nodes and a privileged process can open the device node on
> > > behalf of the application. Add a check to restrict such untrusted
> > > applications from offloading to signed PD.
> > >
> > > Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> > > Cc: stable <[email protected]>
> > > Signed-off-by: Ekansh Gupta <[email protected]>
> > > ---
> > > drivers/misc/fastrpc.c | 23 ++++++++++++++++++-----
> > > 1 file changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index 73fa0e536cf9..32615ccde7ac 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -328,6 +328,7 @@ struct fastrpc_user {
> > > int pd;
> > > bool is_secure_dev;
> > > bool is_unsigned_pd;
> > > + bool untrusted_process;
> > > char *servloc_name;
> > > /* Lock for lists */
> > > spinlock_t lock;
> > > @@ -1249,13 +1250,17 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> > > * channel is configured as secure and block untrusted apps on channel
> > > * that does not support unsigned PD offload
> > > */
> > > - if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
> > > - dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
> > > - return true;
> > > - }
> > > + if (!fl->cctx->unsigned_support || !unsigned_pd_request)
> > > + goto reject_session;
> > > }
> > > + /* Check if untrusted process is trying to offload to signed PD */
> > > + if (fl->untrusted_process && !unsigned_pd_request)
> > > + goto reject_session;
> > > return false;
> > > +reject_session:
> > > + dev_dbg(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
> > > + return true;
> > > }
> > > static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
> > > @@ -1504,12 +1509,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> > > goto err;
> > > }
> > > + /*
> > > + * Third-party apps don't have permission to open the fastrpc device, so
> > Permissions depend on the end-user setup. Is it going to break if the
> > user sets 0666 mode for fastrpc nodes?
>
> If the root user sets 0666 for fastrpc nodes, it is expected that this check will get bypassed.
So, any process will be trusted? This looks so Android-centric. Please come
with a better way to define 'trusted'.
On a typical UNIX system a used has multiple supplementary GIDs (which
can be used to allow access to the devices) which have no relationship
to the process effective GID. On a multi-user machine it might be
logical that fastrpc nodes have separate group-id and group's read/write
permissions. But then each of the users has their own unique 'effective'
GID. Which of those should be using for computing the 'trusted' status?
>
> >
> > > + * it is opened on their behalf by a priveleged process. This is detected
> > > + * by comparing current PID with the one stored during device open.
> > > + */
> > > + if (current->tgid != fl->tgid)
> > > + fl->untrusted_process = true;
> > If the comment talks about PIDs, when why are you comparing GIDs here?
>
> It should be GID, I'll update the comment in next spin.
>
> >
> > > +
> > > if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
> > > fl->is_unsigned_pd = true;
> > > if (is_session_rejected(fl, fl->is_unsigned_pd)) {
> > > - err = -ECONNREFUSED;
> > > + err = -EACCES;
> > > goto err;
> > > }
> > > --
> > > 2.43.0
> > >
--
With best wishes
Dmitry
Hi Ekansh,
On 30/05/2024 12:20, Ekansh Gupta wrote:
> Static PDs on the audio and sensor domains are expected to support
> PD restart. The kernel resource handling for the PDs are expected
> to be handled by fastrpc driver. For this, there is a requirement
> of PD service locator to get the event notifications for static PD
> services. Also when events are received, the driver needs to handle
> based on PD states. Added changes to add service locator for audio
> and sensor domain static PDs and handle the PD restart sequence.
Thanks for the patch, I wanted to bring up a larger issue which I've
been noticing with the fastrpc driver.
This file is now >2.5k LOC, and to my knowledge it features:
* Two different memory mapping/unmapping interfaces to the DSP (three if
you count dmabufs!)
* Different branching codepaths for static vs dynamic processes (with
totally different lifetimes).
* Over 8 structs just for state management, with the largest being >1500
bytes
* Zero context for which code paths are relevant for which contexts
Your series is a combination of bug fixes and cleanup which span most of
the file -- suggesting that the state management is also spread all ove
rthe file. Even for someone with some familiarity with the driver these
patches are really non-trivial to review (patch 5 being a good example
of this), while in addition you introduce new features like this in
patch. This doesn't make for a coherent story for reviewers.
On top of that, there is very minimal, if any comments in this hugely
complex code, and seemingly zero consideration for how to model state or
the lifetime of objects.
Finally, there is next-to-no public documentation or easily obtained
(and used) userspace for this driver. Certainly nothing that I could use
to offer a Tested-by on your patches.
I am not the maintainer, so while it's not my place, I want to offer
some suggestions here:
1. Split the fastrpc driver up into more bite-size chunks which can
actually be reasoned about (I would start by splitting the
implementation details out from the business logic, and then further
abstracting out the static PD support, PDR, etc. Just the "invoke"
mechanism should be in it's own file with it's own state...).
2. Add detailed documentation explaining fastrpc from at least a high
level (ideally with lower level and driver details too), and code
comments (where relevant).
3. Release (and maintain) a reference tool(s) that can be used to
validate the driver and better understand proposed new features (you
could take inspiration from https://gitlab.com/flamingradian/sensh/).
A mechanism to test at least parts of this driver without needing a DSP
on the other end would likely help a lot here, but that's obviously a
much larger scope.
Please let me know your thoughts.
Kind Regards,
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/Kconfig | 2 +
> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 195 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index faf983680040..e2d83cd085b5 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
> tristate "Qualcomm FastRPC"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on RPMSG
> + depends on NET
> select DMA_SHARED_BUFFER
> select QCOM_SCM
> + select QCOM_PDR_HELPERS
> help
> Provides a communication mechanism that allows for clients to
> make remote method invocations across processor boundary to
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 3e1ab58038ed..d115860fc356 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -22,6 +22,7 @@
> #include <linux/firmware/qcom/qcom_scm.h>
> #include <uapi/misc/fastrpc.h>
> #include <linux/of_reserved_mem.h>
> +#include <linux/soc/qcom/pdr.h>
>
> #define ADSP_DOMAIN_ID (0)
> #define MDSP_DOMAIN_ID (1)
> @@ -29,6 +30,7 @@
> #define CDSP_DOMAIN_ID (3)
> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
> #define FASTRPC_MAX_SESSIONS 14
> +#define FASTRPC_MAX_SPD 4
> #define FASTRPC_MAX_VMIDS 16
> #define FASTRPC_ALIGN 128
> #define FASTRPC_MAX_FDLIST 16
> @@ -105,6 +107,18 @@
>
> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>
> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
> +
> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
> +
> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
> +
> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> "sdsp", "cdsp"};
> struct fastrpc_phy_page {
> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
> bool valid;
> };
>
> +struct fastrpc_static_pd {
> + char *servloc_name;
> + char *spdname;
> + void *pdrhandle;
> + struct fastrpc_channel_ctx *cctx;
> + struct fastrpc_user *fl;
> + bool ispdup;
> +};
> +
> struct fastrpc_channel_ctx {
> int domain_id;
> int sesscount;
> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
> struct rpmsg_device *rpdev;
> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
> spinlock_t lock;
> struct idr ctx_idr;
> struct list_head users;
> @@ -297,10 +321,12 @@ struct fastrpc_user {
> struct fastrpc_channel_ctx *cctx;
> struct fastrpc_session_ctx *sctx;
> struct fastrpc_buf *init_mem;
> + struct fastrpc_static_pd *spd;
>
> int tgid;
> int pd;
> bool is_secure_dev;
> + char *servloc_name;
> /* Lock for lists */
> spinlock_t lock;
> /* lock for allocations */
> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
> + struct fastrpc_user *fl)
> +{
> + int i;
> + struct fastrpc_static_pd *spd = NULL;
> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> +
> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
> + if (!cctx->spd[i].servloc_name)
> + continue;
> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
> + spd = &cctx->spd[i];
> + spd->fl = fl;
> + break;
> + }
> + }
> +
> + return spd;
> +}
> +
> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> char __user *argp)
> {
> struct fastrpc_init_create_static init;
> struct fastrpc_invoke_args *args;
> struct fastrpc_phy_page pages[1];
> + struct fastrpc_static_pd *spd = NULL;
> char *name;
> int err;
> struct {
> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_name;
> }
>
> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd) {
> + err = -EUSERS;
> + goto err_name;
> + }
> +
> + if (!spd->ispdup) {
> + err = -ENOTCONN;
> + goto err_name;
> + }
> + fl->spd = spd;
> if (!fl->cctx->remote_heap) {
> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> &fl->cctx->remote_heap);
> @@ -1645,6 +1705,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
> static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> {
> struct fastrpc_invoke_args args[1];
> + struct fastrpc_static_pd *spd = NULL;
> int tgid = fl->tgid;
> u32 sc;
>
> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> fl->pd = pd;
>
> + if (pd == SENSORS_PD) {
> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd)
> + return -EUSERS;
> +
> + if (!spd->ispdup)
> + return -ENOTCONN;
> +
> + fl->spd = spd;
> + }
> +
> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> sc, &args[0]);
> }
> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> return err;
> }
>
> +static void fastrpc_notify_users(struct fastrpc_user *user)
> +{
> + struct fastrpc_invoke_ctx *ctx;
> +
> + spin_lock(&user->lock);
> + list_for_each_entry(ctx, &user->pending, node) {
> + ctx->retval = -EPIPE;
> + complete(&ctx->work);
> + }
> + spin_unlock(&user->lock);
> +}
> +
> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
> + char *servloc_name)
> +{
> + struct fastrpc_user *fl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&cctx->lock, flags);
> + list_for_each_entry(fl, &cctx->users, user) {
> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
> + fastrpc_notify_users(fl);
> + }
> + spin_unlock_irqrestore(&cctx->lock, flags);
> +}
> +
> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> +{
> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
> + struct fastrpc_channel_ctx *cctx;
> +
> + if (!spd)
> + return;
> +
> + cctx = spd->cctx;
> + switch (state) {
> + case SERVREG_SERVICE_STATE_DOWN:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is down for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
> + spd->ispdup = false;
> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> + break;
> + case SERVREG_SERVICE_STATE_UP:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is up for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
> + spd->ispdup = true;
> + break;
> + default:
> + break;
> + }
> +}
> +
> static const struct file_operations fastrpc_fops = {
> .open = fastrpc_device_open,
> .release = fastrpc_device_release,
> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
> return err;
> }
>
> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
> + char *service_name, char *service_path, int domain, int spd_session)
> +{
> + int err = 0;
> + struct pdr_handle *handle = NULL;
> + struct pdr_service *service = NULL;
> +
> + /* Register the service locator's callback function */
> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
> + if (IS_ERR(handle)) {
> + err = PTR_ERR(handle);
> + goto bail;
> + }
> + cctx->spd[spd_session].pdrhandle = handle;
> + cctx->spd[spd_session].servloc_name = client_name;
> + cctx->spd[spd_session].spdname = service_path;
> + cctx->spd[spd_session].cctx = cctx;
> + service = pdr_add_lookup(handle, service_name, service_path);
> + if (IS_ERR(service)) {
> + err = PTR_ERR(service);
> + goto bail;
> + }
> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
> + __func__, service_name, client_name, service_path);
> +
> +bail:
> + if (err) {
> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
> + __func__, service_name, client_name, service_path, err);
> + }
> + return err;
> +}
> +
> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> {
> struct device *rdev = &rpdev->dev;
> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> goto fdev_error;
> }
>
> + if (domain_id == ADSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> +
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
> + if (err)
> + goto populate_error;
> + } else if (domain_id == SDSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> + }
> +
> kref_init(&data->refcount);
>
> dev_set_drvdata(&rpdev->dev, data);
> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> return err;
> }
>
> -static void fastrpc_notify_users(struct fastrpc_user *user)
> -{
> - struct fastrpc_invoke_ctx *ctx;
> -
> - spin_lock(&user->lock);
> - list_for_each_entry(ctx, &user->pending, node) {
> - ctx->retval = -EPIPE;
> - complete(&ctx->work);
> - }
> - spin_unlock(&user->lock);
> -}
> -
> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> {
> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> struct fastrpc_buf *buf, *b;
> struct fastrpc_user *user;
> unsigned long flags;
> + int i;
>
> /* No invocations past this point */
> spin_lock_irqsave(&cctx->lock, flags);
> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> if (cctx->remote_heap)
> fastrpc_buf_free(cctx->remote_heap);
>
> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> + if (cctx->spd[i].pdrhandle)
> + pdr_handle_release(cctx->spd[i].pdrhandle);
> + }
> +
> of_platform_depopulate(&rpdev->dev);
>
> fastrpc_channel_ctx_put(cctx);
--
// Caleb (they/them)
On 6/3/2024 4:23 PM, Caleb Connolly wrote:
> Hi Ekansh,
>
> On 30/05/2024 12:20, Ekansh Gupta wrote:
>> Static PDs on the audio and sensor domains are expected to support
>> PD restart. The kernel resource handling for the PDs are expected
>> to be handled by fastrpc driver. For this, there is a requirement
>> of PD service locator to get the event notifications for static PD
>> services. Also when events are received, the driver needs to handle
>> based on PD states. Added changes to add service locator for audio
>> and sensor domain static PDs and handle the PD restart sequence.
>
> Thanks for the patch, I wanted to bring up a larger issue which I've
> been noticing with the fastrpc driver.
>
> This file is now >2.5k LOC, and to my knowledge it features:
>
> * Two different memory mapping/unmapping interfaces to the DSP (three if
> you count dmabufs!)
> * Different branching codepaths for static vs dynamic processes (with
> totally different lifetimes).
> * Over 8 structs just for state management, with the largest being >1500
> bytes
> * Zero context for which code paths are relevant for which contexts
>
> Your series is a combination of bug fixes and cleanup which span most of
> the file -- suggesting that the state management is also spread all ove
> rthe file. Even for someone with some familiarity with the driver these
> patches are really non-trivial to review (patch 5 being a good example
> of this), while in addition you introduce new features like this in
> patch. This doesn't make for a coherent story for reviewers.
>
> On top of that, there is very minimal, if any comments in this hugely
> complex code, and seemingly zero consideration for how to model state or
> the lifetime of objects.
>
> Finally, there is next-to-no public documentation or easily obtained
> (and used) userspace for this driver. Certainly nothing that I could use
> to offer a Tested-by on your patches.
>
> I am not the maintainer, so while it's not my place, I want to offer
> some suggestions here:
>
> 1. Split the fastrpc driver up into more bite-size chunks which can
> actually be reasoned about (I would start by splitting the
> implementation details out from the business logic, and then further
> abstracting out the static PD support, PDR, etc. Just the "invoke"
> mechanism should be in it's own file with it's own state...).
>
> 2. Add detailed documentation explaining fastrpc from at least a high
> level (ideally with lower level and driver details too), and code
> comments (where relevant).
>
> 3. Release (and maintain) a reference tool(s) that can be used to
> validate the driver and better understand proposed new features (you
> could take inspiration from https://gitlab.com/flamingradian/sensh/).
>
> A mechanism to test at least parts of this driver without needing a DSP
> on the other end would likely help a lot here, but that's obviously a
> much larger scope.
>
> Please let me know your thoughts.
>
> Kind Regards,
Thank you for reviewing the driver and the patches. Your feedback is
greatly appreciated.
As you mentioned, our current driver is a large file containing various
APIs to support multiple features. We are actively discussing
modularizing the driver to enhance clarity and improve understanding.
Additionally, we are in the process of creating basic documentation, as
well as feature-specific documentation for better clarity.
Our latest user space code is available on GitHub at
github.com/quic/fastrpc. Feel free to explore it, and please let us know
if you have any feedback or need further assistance. We’re committed to
continuous improvement.
For testing purposes, we’ve the Hexagon SDK by Qualcomm to the public.
You can download it from the Qualcomm developer network. The SDK
includes examples covering all the features of fastRPC, along with
comprehensive documentation. If you encounter any issues accessing the
Hexagon SDK or have suggestions for improving the documentation, feel
free to reach out.
Best Regards,
Bharath
>>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/Kconfig | 2 +
>> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 195 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index faf983680040..e2d83cd085b5 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
>> tristate "Qualcomm FastRPC"
>> depends on ARCH_QCOM || COMPILE_TEST
>> depends on RPMSG
>> + depends on NET
>> select DMA_SHARED_BUFFER
>> select QCOM_SCM
>> + select QCOM_PDR_HELPERS
>> help
>> Provides a communication mechanism that allows for clients to
>> make remote method invocations across processor boundary to
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 3e1ab58038ed..d115860fc356 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -22,6 +22,7 @@
>> #include <linux/firmware/qcom/qcom_scm.h>
>> #include <uapi/misc/fastrpc.h>
>> #include <linux/of_reserved_mem.h>
>> +#include <linux/soc/qcom/pdr.h>
>> #define ADSP_DOMAIN_ID (0)
>> #define MDSP_DOMAIN_ID (1)
>> @@ -29,6 +30,7 @@
>> #define CDSP_DOMAIN_ID (3)
>> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
>> #define FASTRPC_MAX_SESSIONS 14
>> +#define FASTRPC_MAX_SPD 4
>> #define FASTRPC_MAX_VMIDS 16
>> #define FASTRPC_ALIGN 128
>> #define FASTRPC_MAX_FDLIST 16
>> @@ -105,6 +107,18 @@
>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device,
>> miscdev)
>> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
>> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
>> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
>> +
>> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME
>> "sensors_pdr_adsp"
>> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
>> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
>> +
>> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
>> +#define SENSORS_PDR_SLPI_SERVICE_NAME
>> SENSORS_PDR_ADSP_SERVICE_NAME
>> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
>> +
>> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>> "sdsp", "cdsp"};
>> struct fastrpc_phy_page {
>> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
>> bool valid;
>> };
>> +struct fastrpc_static_pd {
>> + char *servloc_name;
>> + char *spdname;
>> + void *pdrhandle;
>> + struct fastrpc_channel_ctx *cctx;
>> + struct fastrpc_user *fl;
>> + bool ispdup;
>> +};
>> +
>> struct fastrpc_channel_ctx {
>> int domain_id;
>> int sesscount;
>> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
>> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>> struct rpmsg_device *rpdev;
>> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
>> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
>> spinlock_t lock;
>> struct idr ctx_idr;
>> struct list_head users;
>> @@ -297,10 +321,12 @@ struct fastrpc_user {
>> struct fastrpc_channel_ctx *cctx;
>> struct fastrpc_session_ctx *sctx;
>> struct fastrpc_buf *init_mem;
>> + struct fastrpc_static_pd *spd;
>> int tgid;
>> int pd;
>> bool is_secure_dev;
>> + char *servloc_name;
>> /* Lock for lists */
>> spinlock_t lock;
>> /* lock for allocations */
>> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct
>> fastrpc_user *fl, bool unsigned_pd_reques
>> return false;
>> }
>> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
>> + struct fastrpc_user *fl)
>> +{
>> + int i;
>> + struct fastrpc_static_pd *spd = NULL;
>> + struct fastrpc_channel_ctx *cctx = fl->cctx;
>> +
>> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
>> + if (!cctx->spd[i].servloc_name)
>> + continue;
>> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
>> + spd = &cctx->spd[i];
>> + spd->fl = fl;
>> + break;
>> + }
>> + }
>> +
>> + return spd;
>> +}
>> +
>> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> char __user *argp)
>> {
>> struct fastrpc_init_create_static init;
>> struct fastrpc_invoke_args *args;
>> struct fastrpc_phy_page pages[1];
>> + struct fastrpc_static_pd *spd = NULL;
>> char *name;
>> int err;
>> struct {
>> @@ -1270,6 +1317,19 @@ static int
>> fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> goto err_name;
>> }
>> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
>> +
>> + spd = fastrpc_get_spd_session(fl);
>> + if (!spd) {
>> + err = -EUSERS;
>> + goto err_name;
>> + }
>> +
>> + if (!spd->ispdup) {
>> + err = -ENOTCONN;
>> + goto err_name;
>> + }
>> + fl->spd = spd;
>> if (!fl->cctx->remote_heap) {
>> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>> &fl->cctx->remote_heap);
>> @@ -1645,6 +1705,7 @@ static int fastrpc_dmabuf_alloc(struct
>> fastrpc_user *fl, char __user *argp)
>> static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>> {
>> struct fastrpc_invoke_args args[1];
>> + struct fastrpc_static_pd *spd = NULL;
>> int tgid = fl->tgid;
>> u32 sc;
>> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct
>> fastrpc_user *fl, int pd)
>> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>> fl->pd = pd;
>> + if (pd == SENSORS_PD) {
>> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
>> + fl->servloc_name =
>> SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
>> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
>> + fl->servloc_name =
>> SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
>> +
>> + spd = fastrpc_get_spd_session(fl);
>> + if (!spd)
>> + return -EUSERS;
>> +
>> + if (!spd->ispdup)
>> + return -ENOTCONN;
>> +
>> + fl->spd = spd;
>> + }
>> +
>> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> sc, &args[0]);
>> }
>> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file
>> *file, unsigned int cmd,
>> return err;
>> }
>> +static void fastrpc_notify_users(struct fastrpc_user *user)
>> +{
>> + struct fastrpc_invoke_ctx *ctx;
>> +
>> + spin_lock(&user->lock);
>> + list_for_each_entry(ctx, &user->pending, node) {
>> + ctx->retval = -EPIPE;
>> + complete(&ctx->work);
>> + }
>> + spin_unlock(&user->lock);
>> +}
>> +
>> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
>> + char *servloc_name)
>> +{
>> + struct fastrpc_user *fl;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&cctx->lock, flags);
>> + list_for_each_entry(fl, &cctx->users, user) {
>> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
>> + fastrpc_notify_users(fl);
>> + }
>> + spin_unlock_irqrestore(&cctx->lock, flags);
>> +}
>> +
>> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
>> +{
>> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
>> + struct fastrpc_channel_ctx *cctx;
>> +
>> + if (!spd)
>> + return;
>> +
>> + cctx = spd->cctx;
>> + switch (state) {
>> + case SERVREG_SERVICE_STATE_DOWN:
>> + dev_info(&cctx->rpdev->dev,
>> + "%s: %s (%s) is down for PDR on %s\n",
>> + __func__, spd->spdname,
>> + spd->servloc_name,
>> + domains[cctx->domain_id]);
>> + spd->ispdup = false;
>> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
>> + break;
>> + case SERVREG_SERVICE_STATE_UP:
>> + dev_info(&cctx->rpdev->dev,
>> + "%s: %s (%s) is up for PDR on %s\n",
>> + __func__, spd->spdname,
>> + spd->servloc_name,
>> + domains[cctx->domain_id]);
>> + spd->ispdup = true;
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> static const struct file_operations fastrpc_fops = {
>> .open = fastrpc_device_open,
>> .release = fastrpc_device_release,
>> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct
>> device *dev, struct fastrpc_channel_ct
>> return err;
>> }
>> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx
>> *cctx, char *client_name,
>> + char *service_name, char *service_path, int domain, int
>> spd_session)
>> +{
>> + int err = 0;
>> + struct pdr_handle *handle = NULL;
>> + struct pdr_service *service = NULL;
>> +
>> + /* Register the service locator's callback function */
>> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
>> + if (IS_ERR(handle)) {
>> + err = PTR_ERR(handle);
>> + goto bail;
>> + }
>> + cctx->spd[spd_session].pdrhandle = handle;
>> + cctx->spd[spd_session].servloc_name = client_name;
>> + cctx->spd[spd_session].spdname = service_path;
>> + cctx->spd[spd_session].cctx = cctx;
>> + service = pdr_add_lookup(handle, service_name, service_path);
>> + if (IS_ERR(service)) {
>> + err = PTR_ERR(service);
>> + goto bail;
>> + }
>> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
>> + __func__, service_name, client_name, service_path);
>> +
>> +bail:
>> + if (err) {
>> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
>> + __func__, service_name, client_name, service_path, err);
>> + }
>> + return err;
>> +}
>> +
>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> {
>> struct device *rdev = &rpdev->dev;
>> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>> goto fdev_error;
>> }
>> + if (domain_id == ADSP_DOMAIN_ID) {
>> + err = fastrpc_setup_service_locator(data,
>> AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
>> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME,
>> domain_id, 0);
>> + if (err)
>> + goto populate_error;
>> +
>> + err = fastrpc_setup_service_locator(data,
>> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
>> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME,
>> domain_id, 1);
>> + if (err)
>> + goto populate_error;
>> + } else if (domain_id == SDSP_DOMAIN_ID) {
>> + err = fastrpc_setup_service_locator(data,
>> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
>> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME,
>> domain_id, 0);
>> + if (err)
>> + goto populate_error;
>> + }
>> +
>> kref_init(&data->refcount);
>> dev_set_drvdata(&rpdev->dev, data);
>> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>> return err;
>> }
>> -static void fastrpc_notify_users(struct fastrpc_user *user)
>> -{
>> - struct fastrpc_invoke_ctx *ctx;
>> -
>> - spin_lock(&user->lock);
>> - list_for_each_entry(ctx, &user->pending, node) {
>> - ctx->retval = -EPIPE;
>> - complete(&ctx->work);
>> - }
>> - spin_unlock(&user->lock);
>> -}
>> -
>> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> {
>> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
>> struct fastrpc_buf *buf, *b;
>> struct fastrpc_user *user;
>> unsigned long flags;
>> + int i;
>> /* No invocations past this point */
>> spin_lock_irqsave(&cctx->lock, flags);
>> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct
>> rpmsg_device *rpdev)
>> if (cctx->remote_heap)
>> fastrpc_buf_free(cctx->remote_heap);
>> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
>> + if (cctx->spd[i].pdrhandle)
>> + pdr_handle_release(cctx->spd[i].pdrhandle);
>> + }
>> +
>> of_platform_depopulate(&rpdev->dev);
>> fastrpc_channel_ctx_put(cctx);
>
On Tue, 4 Jun 2024 at 12:23, Bharath Kumar V <[email protected]> wrote:
>
>
>
> On 6/3/2024 4:23 PM, Caleb Connolly wrote:
> > Hi Ekansh,
> >
> > On 30/05/2024 12:20, Ekansh Gupta wrote:
> >> Static PDs on the audio and sensor domains are expected to support
> >> PD restart. The kernel resource handling for the PDs are expected
> >> to be handled by fastrpc driver. For this, there is a requirement
> >> of PD service locator to get the event notifications for static PD
> >> services. Also when events are received, the driver needs to handle
> >> based on PD states. Added changes to add service locator for audio
> >> and sensor domain static PDs and handle the PD restart sequence.
> >
> > Thanks for the patch, I wanted to bring up a larger issue which I've
> > been noticing with the fastrpc driver.
> >
> > This file is now >2.5k LOC, and to my knowledge it features:
> >
> > * Two different memory mapping/unmapping interfaces to the DSP (three if
> > you count dmabufs!)
> > * Different branching codepaths for static vs dynamic processes (with
> > totally different lifetimes).
> > * Over 8 structs just for state management, with the largest being >1500
> > bytes
> > * Zero context for which code paths are relevant for which contexts
> >
> > Your series is a combination of bug fixes and cleanup which span most of
> > the file -- suggesting that the state management is also spread all ove
> > rthe file. Even for someone with some familiarity with the driver these
> > patches are really non-trivial to review (patch 5 being a good example
> > of this), while in addition you introduce new features like this in
> > patch. This doesn't make for a coherent story for reviewers.
> >
> > On top of that, there is very minimal, if any comments in this hugely
> > complex code, and seemingly zero consideration for how to model state or
> > the lifetime of objects.
> >
> > Finally, there is next-to-no public documentation or easily obtained
> > (and used) userspace for this driver. Certainly nothing that I could use
> > to offer a Tested-by on your patches.
> >
> > I am not the maintainer, so while it's not my place, I want to offer
> > some suggestions here:
> >
> > 1. Split the fastrpc driver up into more bite-size chunks which can
> > actually be reasoned about (I would start by splitting the
> > implementation details out from the business logic, and then further
> > abstracting out the static PD support, PDR, etc. Just the "invoke"
> > mechanism should be in it's own file with it's own state...).
> >
> > 2. Add detailed documentation explaining fastrpc from at least a high
> > level (ideally with lower level and driver details too), and code
> > comments (where relevant).
> >
> > 3. Release (and maintain) a reference tool(s) that can be used to
> > validate the driver and better understand proposed new features (you
> > could take inspiration from https://gitlab.com/flamingradian/sensh/).
> >
> > A mechanism to test at least parts of this driver without needing a DSP
> > on the other end would likely help a lot here, but that's obviously a
> > much larger scope.
> >
> > Please let me know your thoughts.
> >
> > Kind Regards,
>
> Thank you for reviewing the driver and the patches. Your feedback is
> greatly appreciated.
> As you mentioned, our current driver is a large file containing various
> APIs to support multiple features. We are actively discussing
> modularizing the driver to enhance clarity and improve understanding.
Please consider dropping the FastRPC interface completely and
switching to drm/accel instead. This is the API that should be used by
accelerators (including, but not limited to AI accelerators).
> Additionally, we are in the process of creating basic documentation, as
> well as feature-specific documentation for better clarity.
> Our latest user space code is available on GitHub at
> github.com/quic/fastrpc. Feel free to explore it, and please let us know
> if you have any feedback or need further assistance. We’re committed to
> continuous improvement.
Note, this is not what is usually expected from open source. This is
just a code drop, dumped to a git repository. It doesn't have
development history, it doesn't answer the most typical comment that
we as open source developers have: "why was it developed in this
way?". Please consider replacing this code drop with the Git tree that
contains development history.
> For testing purposes, we’ve the Hexagon SDK by Qualcomm to the public.
> You can download it from the Qualcomm developer network.
What is the licence for the SDK? What is the licence for the binaries
(including skel.so) generated using the SDK?
Is there an open-source toolchain?
The requirement for any kernel driver using dma-buf is to have
completely functional open source userspace implementation (it doesn't
have to be fully optimal or exercise all hardware features, but it
should be possible to understand what is being passed around and how
the user API interface is supposed to work.
> The SDK
> includes examples covering all the features of fastRPC, along with
> comprehensive documentation. If you encounter any issues accessing the
> Hexagon SDK or have suggestions for improving the documentation, feel
> free to reach out.
A typical problem that we have is if it is possible to integrate
userspace tools into an OE build infrastructure. We want to be able to
build simple test programs and include them into OE images.
This requires being able to download the files without any additional
package managers or click-through licences.
>
> Best Regards,
> Bharath
>
> >>
> >> Signed-off-by: Ekansh Gupta <[email protected]>
> >> ---
> >> drivers/misc/Kconfig | 2 +
> >> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
> >> 2 files changed, 195 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> >> index faf983680040..e2d83cd085b5 100644
> >> --- a/drivers/misc/Kconfig
> >> +++ b/drivers/misc/Kconfig
> >> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
> >> tristate "Qualcomm FastRPC"
> >> depends on ARCH_QCOM || COMPILE_TEST
> >> depends on RPMSG
> >> + depends on NET
> >> select DMA_SHARED_BUFFER
> >> select QCOM_SCM
> >> + select QCOM_PDR_HELPERS
> >> help
> >> Provides a communication mechanism that allows for clients to
> >> make remote method invocations across processor boundary to
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index 3e1ab58038ed..d115860fc356 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -22,6 +22,7 @@
> >> #include <linux/firmware/qcom/qcom_scm.h>
> >> #include <uapi/misc/fastrpc.h>
> >> #include <linux/of_reserved_mem.h>
> >> +#include <linux/soc/qcom/pdr.h>
> >> #define ADSP_DOMAIN_ID (0)
> >> #define MDSP_DOMAIN_ID (1)
> >> @@ -29,6 +30,7 @@
> >> #define CDSP_DOMAIN_ID (3)
> >> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
> >> #define FASTRPC_MAX_SESSIONS 14
> >> +#define FASTRPC_MAX_SPD 4
> >> #define FASTRPC_MAX_VMIDS 16
> >> #define FASTRPC_ALIGN 128
> >> #define FASTRPC_MAX_FDLIST 16
> >> @@ -105,6 +107,18 @@
> >> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device,
> >> miscdev)
> >> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
> >> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
> >> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
> >> +
> >> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME
> >> "sensors_pdr_adsp"
> >> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
> >> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
> >> +
> >> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
> >> +#define SENSORS_PDR_SLPI_SERVICE_NAME
> >> SENSORS_PDR_ADSP_SERVICE_NAME
> >> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
> >> +
> >> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> >> "sdsp", "cdsp"};
> >> struct fastrpc_phy_page {
> >> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
> >> bool valid;
> >> };
> >> +struct fastrpc_static_pd {
> >> + char *servloc_name;
> >> + char *spdname;
> >> + void *pdrhandle;
> >> + struct fastrpc_channel_ctx *cctx;
> >> + struct fastrpc_user *fl;
> >> + bool ispdup;
> >> +};
> >> +
> >> struct fastrpc_channel_ctx {
> >> int domain_id;
> >> int sesscount;
> >> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
> >> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
> >> struct rpmsg_device *rpdev;
> >> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> >> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
> >> spinlock_t lock;
> >> struct idr ctx_idr;
> >> struct list_head users;
> >> @@ -297,10 +321,12 @@ struct fastrpc_user {
> >> struct fastrpc_channel_ctx *cctx;
> >> struct fastrpc_session_ctx *sctx;
> >> struct fastrpc_buf *init_mem;
> >> + struct fastrpc_static_pd *spd;
> >> int tgid;
> >> int pd;
> >> bool is_secure_dev;
> >> + char *servloc_name;
> >> /* Lock for lists */
> >> spinlock_t lock;
> >> /* lock for allocations */
> >> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct
> >> fastrpc_user *fl, bool unsigned_pd_reques
> >> return false;
> >> }
> >> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
> >> + struct fastrpc_user *fl)
> >> +{
> >> + int i;
> >> + struct fastrpc_static_pd *spd = NULL;
> >> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> >> +
> >> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
> >> + if (!cctx->spd[i].servloc_name)
> >> + continue;
> >> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
> >> + spd = &cctx->spd[i];
> >> + spd->fl = fl;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + return spd;
> >> +}
> >> +
> >> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >> char __user *argp)
> >> {
> >> struct fastrpc_init_create_static init;
> >> struct fastrpc_invoke_args *args;
> >> struct fastrpc_phy_page pages[1];
> >> + struct fastrpc_static_pd *spd = NULL;
> >> char *name;
> >> int err;
> >> struct {
> >> @@ -1270,6 +1317,19 @@ static int
> >> fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >> goto err_name;
> >> }
> >> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
> >> +
> >> + spd = fastrpc_get_spd_session(fl);
> >> + if (!spd) {
> >> + err = -EUSERS;
> >> + goto err_name;
> >> + }
> >> +
> >> + if (!spd->ispdup) {
> >> + err = -ENOTCONN;
> >> + goto err_name;
> >> + }
> >> + fl->spd = spd;
> >> if (!fl->cctx->remote_heap) {
> >> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> >> &fl->cctx->remote_heap);
> >> @@ -1645,6 +1705,7 @@ static int fastrpc_dmabuf_alloc(struct
> >> fastrpc_user *fl, char __user *argp)
> >> static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> >> {
> >> struct fastrpc_invoke_args args[1];
> >> + struct fastrpc_static_pd *spd = NULL;
> >> int tgid = fl->tgid;
> >> u32 sc;
> >> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct
> >> fastrpc_user *fl, int pd)
> >> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> >> fl->pd = pd;
> >> + if (pd == SENSORS_PD) {
> >> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
> >> + fl->servloc_name =
> >> SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
> >> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
> >> + fl->servloc_name =
> >> SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
> >> +
> >> + spd = fastrpc_get_spd_session(fl);
> >> + if (!spd)
> >> + return -EUSERS;
> >> +
> >> + if (!spd->ispdup)
> >> + return -ENOTCONN;
> >> +
> >> + fl->spd = spd;
> >> + }
> >> +
> >> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> >> sc, &args[0]);
> >> }
> >> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file
> >> *file, unsigned int cmd,
> >> return err;
> >> }
> >> +static void fastrpc_notify_users(struct fastrpc_user *user)
> >> +{
> >> + struct fastrpc_invoke_ctx *ctx;
> >> +
> >> + spin_lock(&user->lock);
> >> + list_for_each_entry(ctx, &user->pending, node) {
> >> + ctx->retval = -EPIPE;
> >> + complete(&ctx->work);
> >> + }
> >> + spin_unlock(&user->lock);
> >> +}
> >> +
> >> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
> >> + char *servloc_name)
> >> +{
> >> + struct fastrpc_user *fl;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&cctx->lock, flags);
> >> + list_for_each_entry(fl, &cctx->users, user) {
> >> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
> >> + fastrpc_notify_users(fl);
> >> + }
> >> + spin_unlock_irqrestore(&cctx->lock, flags);
> >> +}
> >> +
> >> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> >> +{
> >> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
> >> + struct fastrpc_channel_ctx *cctx;
> >> +
> >> + if (!spd)
> >> + return;
> >> +
> >> + cctx = spd->cctx;
> >> + switch (state) {
> >> + case SERVREG_SERVICE_STATE_DOWN:
> >> + dev_info(&cctx->rpdev->dev,
> >> + "%s: %s (%s) is down for PDR on %s\n",
> >> + __func__, spd->spdname,
> >> + spd->servloc_name,
> >> + domains[cctx->domain_id]);
> >> + spd->ispdup = false;
> >> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> >> + break;
> >> + case SERVREG_SERVICE_STATE_UP:
> >> + dev_info(&cctx->rpdev->dev,
> >> + "%s: %s (%s) is up for PDR on %s\n",
> >> + __func__, spd->spdname,
> >> + spd->servloc_name,
> >> + domains[cctx->domain_id]);
> >> + spd->ispdup = true;
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> +}
> >> +
> >> static const struct file_operations fastrpc_fops = {
> >> .open = fastrpc_device_open,
> >> .release = fastrpc_device_release,
> >> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct
> >> device *dev, struct fastrpc_channel_ct
> >> return err;
> >> }
> >> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx
> >> *cctx, char *client_name,
> >> + char *service_name, char *service_path, int domain, int
> >> spd_session)
> >> +{
> >> + int err = 0;
> >> + struct pdr_handle *handle = NULL;
> >> + struct pdr_service *service = NULL;
> >> +
> >> + /* Register the service locator's callback function */
> >> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
> >> + if (IS_ERR(handle)) {
> >> + err = PTR_ERR(handle);
> >> + goto bail;
> >> + }
> >> + cctx->spd[spd_session].pdrhandle = handle;
> >> + cctx->spd[spd_session].servloc_name = client_name;
> >> + cctx->spd[spd_session].spdname = service_path;
> >> + cctx->spd[spd_session].cctx = cctx;
> >> + service = pdr_add_lookup(handle, service_name, service_path);
> >> + if (IS_ERR(service)) {
> >> + err = PTR_ERR(service);
> >> + goto bail;
> >> + }
> >> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
> >> + __func__, service_name, client_name, service_path);
> >> +
> >> +bail:
> >> + if (err) {
> >> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
> >> + __func__, service_name, client_name, service_path, err);
> >> + }
> >> + return err;
> >> +}
> >> +
> >> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> >> {
> >> struct device *rdev = &rpdev->dev;
> >> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct
> >> rpmsg_device *rpdev)
> >> goto fdev_error;
> >> }
> >> + if (domain_id == ADSP_DOMAIN_ID) {
> >> + err = fastrpc_setup_service_locator(data,
> >> AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
> >> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME,
> >> domain_id, 0);
> >> + if (err)
> >> + goto populate_error;
> >> +
> >> + err = fastrpc_setup_service_locator(data,
> >> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
> >> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME,
> >> domain_id, 1);
> >> + if (err)
> >> + goto populate_error;
> >> + } else if (domain_id == SDSP_DOMAIN_ID) {
> >> + err = fastrpc_setup_service_locator(data,
> >> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
> >> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME,
> >> domain_id, 0);
> >> + if (err)
> >> + goto populate_error;
> >> + }
> >> +
> >> kref_init(&data->refcount);
> >> dev_set_drvdata(&rpdev->dev, data);
> >> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct
> >> rpmsg_device *rpdev)
> >> return err;
> >> }
> >> -static void fastrpc_notify_users(struct fastrpc_user *user)
> >> -{
> >> - struct fastrpc_invoke_ctx *ctx;
> >> -
> >> - spin_lock(&user->lock);
> >> - list_for_each_entry(ctx, &user->pending, node) {
> >> - ctx->retval = -EPIPE;
> >> - complete(&ctx->work);
> >> - }
> >> - spin_unlock(&user->lock);
> >> -}
> >> -
> >> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> >> {
> >> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> >> struct fastrpc_buf *buf, *b;
> >> struct fastrpc_user *user;
> >> unsigned long flags;
> >> + int i;
> >> /* No invocations past this point */
> >> spin_lock_irqsave(&cctx->lock, flags);
> >> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct
> >> rpmsg_device *rpdev)
> >> if (cctx->remote_heap)
> >> fastrpc_buf_free(cctx->remote_heap);
> >> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> >> + if (cctx->spd[i].pdrhandle)
> >> + pdr_handle_release(cctx->spd[i].pdrhandle);
> >> + }
> >> +
> >> of_platform_depopulate(&rpdev->dev);
> >> fastrpc_channel_ctx_put(cctx);
> >
--
With best wishes
Dmitry
Hi Bharath,
>
> Thank you for reviewing the driver and the patches. Your feedback is
> greatly appreciated.
> As you mentioned, our current driver is a large file containing various
> APIs to support multiple features. We are actively discussing
> modularizing the driver to enhance clarity and improve understanding.
> Additionally, we are in the process of creating basic documentation, as
> well as feature-specific documentation for better clarity.
This is really great to hear. I hope to see some patches moving things
in this direction.
Kind regards,
--
// Caleb (they/them)
On 6/3/2024 3:32 PM, Dmitry Baryshkov wrote:
> On Mon, Jun 03, 2024 at 11:57:52AM +0530, Ekansh Gupta wrote:
>> On 5/31/2024 5:19 AM, Dmitry Baryshkov wrote:
>>> On Thu, May 30, 2024 at 03:50:26PM +0530, Ekansh Gupta wrote:
>>>> Some untrusted applications will not have access to open fastrpc
>>>> device nodes and a privileged process can open the device node on
>>>> behalf of the application. Add a check to restrict such untrusted
>>>> applications from offloading to signed PD.
>>>>
>>>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>>>> Cc: stable <[email protected]>
>>>> Signed-off-by: Ekansh Gupta <[email protected]>
>>>> ---
>>>> drivers/misc/fastrpc.c | 23 ++++++++++++++++++-----
>>>> 1 file changed, 18 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 73fa0e536cf9..32615ccde7ac 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -328,6 +328,7 @@ struct fastrpc_user {
>>>> int pd;
>>>> bool is_secure_dev;
>>>> bool is_unsigned_pd;
>>>> + bool untrusted_process;
>>>> char *servloc_name;
>>>> /* Lock for lists */
>>>> spinlock_t lock;
>>>> @@ -1249,13 +1250,17 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>>>> * channel is configured as secure and block untrusted apps on channel
>>>> * that does not support unsigned PD offload
>>>> */
>>>> - if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
>>>> - dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
>>>> - return true;
>>>> - }
>>>> + if (!fl->cctx->unsigned_support || !unsigned_pd_request)
>>>> + goto reject_session;
>>>> }
>>>> + /* Check if untrusted process is trying to offload to signed PD */
>>>> + if (fl->untrusted_process && !unsigned_pd_request)
>>>> + goto reject_session;
>>>> return false;
>>>> +reject_session:
>>>> + dev_dbg(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
>>>> + return true;
>>>> }
>>>> static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
>>>> @@ -1504,12 +1509,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>>> goto err;
>>>> }
>>>> + /*
>>>> + * Third-party apps don't have permission to open the fastrpc device, so
>>> Permissions depend on the end-user setup. Is it going to break if the
>>> user sets 0666 mode for fastrpc nodes?
>> If the root user sets 0666 for fastrpc nodes, it is expected that this check will get bypassed.
> So, any process will be trusted? This looks so Android-centric. Please come
> with a better way to define 'trusted'.
>
> On a typical UNIX system a used has multiple supplementary GIDs (which
> can be used to allow access to the devices) which have no relationship
> to the process effective GID. On a multi-user machine it might be
> logical that fastrpc nodes have separate group-id and group's read/write
> permissions. But then each of the users has their own unique 'effective'
> GID. Which of those should be using for computing the 'trusted' status?
Thanks for your suggestions, Dmitry. I am considering dropping this patch and system unsignedPD patch
from this series(due to the dependency). I'm redesigning the trusted-process term to make it more generic.
Planning to make it depend on the group IDs and have a check with both primary and supplementary GIDs
of the process. I'll share the design with you along with the changes once it's ready.
>
>>>> + * it is opened on their behalf by a priveleged process. This is detected
>>>> + * by comparing current PID with the one stored during device open.
>>>> + */
>>>> + if (current->tgid != fl->tgid)
>>>> + fl->untrusted_process = true;
>>> If the comment talks about PIDs, when why are you comparing GIDs here?
>> It should be GID, I'll update the comment in next spin.
>>
>>>> +
>>>> if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
>>>> fl->is_unsigned_pd = true;
>>>> if (is_session_rejected(fl, fl->is_unsigned_pd)) {
>>>> - err = -ECONNREFUSED;
>>>> + err = -EACCES;
>>>> goto err;
>>>> }
>>>> --
>>>> 2.43.0
>>>>