| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <200f2cc9-1747-95a3-82b1-600650cff1df@linaro.org>
Date: Tue, 6 Sep 2022 15:10:41 +0100
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Abel Vesa <abel.vesa@...aro.org>,
Amol Maheshwari <amahesh@....qualcomm.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Ekansh Gupta <quic_ekagupt@...cinc.com>
Cc: Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-arm-msm@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
devicetree@...r.kernel.org
Subject: Re: [PATCH v2 07/10] misc: fastrpc: Add support for audiopd
Thanks Abel,
Some minor comments.
On 02/09/2022 16:48, Abel Vesa wrote:
> In order to be able to start the adsp listener for audiopd using adsprpcd,
> we need to add the corresponding ioctl for creating a static process.
> On that ioctl call we need to allocate the heap. Allocating the heap needs
> to be happening only once and needs to be kept between different device
> open calls, so attach it to the channel context to make sure that remains
> until the RPMSG driver is removed. Then, if there are any VMIDs associated
> with the static ADSP process, do a call to SCM to assign it.
> And then, send all the necessary info related to heap to the DSP.
>
> Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> ---
> drivers/misc/fastrpc.c | 126 ++++++++++++++++++++++++++++++++++++
> include/uapi/misc/fastrpc.h | 7 ++
> 2 files changed, 133 insertions(+)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7c364c58e379..2c656da4ca5e 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -37,8 +37,20 @@
> #define FASTRPC_DSP_UTILITIES_HANDLE 2
> #define FASTRPC_CTXID_MASK (0xFF0)
> #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> +#define INIT_FILE_NAMELEN_MAX (128)
> #define FASTRPC_DEVICE_NAME "fastrpc"
> +
> +/* Add memory to static PD pool, protection thru XPU */
> +#define ADSP_MMAP_HEAP_ADDR 4
> +/* MAP static DMA buffer on DSP User PD */
> +#define ADSP_MMAP_DMA_BUFFER 6
> +/* Add memory to static PD pool protection thru hypervisor */
> +#define ADSP_MMAP_REMOTE_HEAP_ADDR 8
> +/* Add memory to userPD pool, for user heap */
> #define ADSP_MMAP_ADD_PAGES 0x1000
> +/* Add memory to userPD pool, for LLC heap */
> +#define ADSP_MMAP_ADD_PAGES_LLC 0x3000,
> +
> #define DSP_UNSUPPORTED_API (0x80000414)
> /* MAX NUMBER of DSP ATTRIBUTES SUPPORTED */
> #define FASTRPC_MAX_DSP_ATTRIBUTES (256)
> @@ -72,6 +84,7 @@
> FASTRPC_BUILD_SCALARS(0, method, in, out, 0, 0)
>
> #define FASTRPC_CREATE_PROCESS_NARGS 6
> +#define FASTRPC_CREATE_STATIC_PROCESS_NARGS 3
> /* Remote Method id table */
> #define FASTRPC_RMID_INIT_ATTACH 0
> #define FASTRPC_RMID_INIT_RELEASE 1
> @@ -261,6 +274,7 @@ 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;
> bool secure;
> bool unsigned_support;
> };
> @@ -1167,6 +1181,7 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel,
> spin_unlock(&fl->lock);
> fastrpc_context_put(ctx);
> }
> +
new line not relevant to this patch.
> if (err)
> dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err);
>
> @@ -1191,6 +1206,111 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +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];
> + char *name;
> + int err;
> + struct {
> + int pgid;
> + u32 namelen;
> + u32 pageslen;
> + } inbuf;
> + u32 sc;
> +
> + args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> + if (!args)
> + return -ENOMEM;
> +
> + if (copy_from_user(&init, argp, sizeof(init))) {
> + err = -EFAULT;
> + goto err;
> + }
> +
> + if (init.namelen > INIT_FILE_NAMELEN_MAX) {
> + err = -EINVAL;
> + goto err;
> + }
> +
> + name = kzalloc(init.namelen, GFP_KERNEL);
> + if (!name) {
> + err = -ENOMEM;
> + goto err;
> + }
> +
> + if (copy_from_user(name, (void __user *)(uintptr_t)init.name, init.namelen)) {
> + err = -EFAULT;
> + goto err_name;
> + }
> +
> + if (!fl->cctx->remote_heap) {
> + err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> + &fl->cctx->remote_heap);
> + if (err)
> + goto err_name;
> +
> + /* Map if we have any heap VMIDs associated with this ADSP Static Process. */
> + if (fl->cctx->vmcount) {
> + unsigned int perms = BIT(QCOM_SCM_VMID_HLOS);
> +
> + dev_dbg(fl->sctx->dev, "Assinging memory with phys 0x%llx size 0x%llx perms 0x%x, vmperms %x, vmcount %x\n",
> + fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size,
> + perms, fl->cctx->vmperms, fl->cctx->vmcount);
Please remove this debug, do we really need this?
> + err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
> + (u64)fl->cctx->remote_heap->size, &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",
> + fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> + goto err_map;
> + }
> + }
> + }
> +
> + inbuf.pgid = fl->tgid;
> + inbuf.namelen = init.namelen;
> + inbuf.pageslen = 0;
> + fl->pd = USER_PD;
> +
> + args[0].ptr = (u64)(uintptr_t)&inbuf;
> + args[0].length = sizeof(inbuf);
> + args[0].fd = -1;
> +
> + args[1].ptr = (u64)(uintptr_t)name;
> + 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;
> +
> + args[2].ptr = (u64)(uintptr_t) pages;
> + args[2].length = sizeof(*pages);
> + args[2].fd = -1;
> +
> + sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
> +
> + err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> + sc, args);
> + if (err)
> + goto err_invoke;
> +
> + kfree(args);
> +
> + return 0;
> +err_invoke:
What happens to the secure mappings here, if invoke failed should we not
cleanup that part?
Or may be a not saying that we will be reusing this would be nice.
> +err_map:
> + fastrpc_buf_free(fl->cctx->remote_heap);
> +err_name:
> + kfree(name);
> +err:
> + kfree(args);
> +
> + return err;
> +}
> +
> static int fastrpc_init_create_process(struct fastrpc_user *fl,
> char __user *argp)
> {
> @@ -1918,6 +2038,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> case FASTRPC_IOCTL_INIT_ATTACH_SNS:
> err = fastrpc_init_attach(fl, SENSORS_PD);
> break;
> + case FASTRPC_IOCTL_INIT_CREATE_STATIC:
> + err = fastrpc_init_create_static_process(fl, argp);
> + break;
> case FASTRPC_IOCTL_INIT_CREATE:
> err = fastrpc_init_create_process(fl, argp);
> break;
> @@ -2183,6 +2306,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> if (cctx->secure_fdevice)
> misc_deregister(&cctx->secure_fdevice->miscdev);
>
> + if (cctx->remote_heap)
> + fastrpc_buf_free(cctx->remote_heap);
> +
> of_platform_depopulate(&rpdev->dev);
>
> cctx->rpdev = NULL;
> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index 5e29f2cfa42d..2cdf2f137d33 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -16,6 +16,7 @@
> #define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map)
> #define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap)
> #define FASTRPC_IOCTL_GET_DSP_INFO _IOWR('R', 13, struct fastrpc_ioctl_capability)
> +#define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 15, struct fastrpc_init_create_static)
>
> /**
> * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
> @@ -87,6 +88,12 @@ struct fastrpc_init_create {
> __u64 file; /* pointer to elf file */
> };
>
> +struct fastrpc_init_create_static {
> + __u32 namelen; /* length of pd process name */
> + __u32 memlen;
> + __u64 name; /* pd process name */
> +};
> +
> struct fastrpc_alloc_dma_buf {
> __s32 fd; /* fd */
> __u32 flags; /* flags to map with */
Powered by blists - more mailing lists