Date:   Mon, 13 Dec 2021 10:37:33 -0800
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc:     robh+dt@kernel.org, gregkh@linuxfoundation.org,
        devicetree@vger.kernel.org, ekangupt@qti.qualcomm.com,
        jeyr@codeaurora.org, bkumar@qti.qualcomm.com,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 6/8] misc: fastrpc: add secure domain support
Message-ID: <YbeS7KXj1slU2qgf@ripper>
References: <20211209120626.26373-1-srinivas.kandagatla@linaro.org>
 <20211209120626.26373-7-srinivas.kandagatla@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211209120626.26373-7-srinivas.kandagatla@linaro.org>
Precedence: bulk

On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:

> ADSP/MDSP/SDSP are by default secured, which means it can only be loaded
> with a Signed process.
> Where as CDSP can be either be secured/unsecured. non-secured Compute DSP
> would allow users to load unsigned process and run hexagon instructions,
> but blocking access to secured hardware within the DSP. Where as signed
> process with secure CDSP would be allowed to access all the dsp resources.
> 
> This patch adds basic code to create device nodes as per device tree property.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/misc/fastrpc.c | 61 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 79fc59caacef..50f8e23b6b04 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -240,12 +240,15 @@ struct fastrpc_channel_ctx {
>  	/* Flag if dsp attributes are cached */
>  	bool valid_attributes;
>  	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
> +	struct fastrpc_device *secure_fdevice;
>  	struct fastrpc_device *fdevice;
> +	bool secure;
>  };
>  
>  struct fastrpc_device {
>  	struct fastrpc_channel_ctx *cctx;
>  	struct miscdevice miscdev;
> +	bool secure;
>  };
>  
>  struct fastrpc_user {
> @@ -1876,7 +1879,7 @@ static struct platform_driver fastrpc_cb_driver = {
>  };
>  
>  static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
> -				   const char *domain)
> +				   bool is_secured, const char *domain)
>  {
>  	struct fastrpc_device *fdev;
>  	int err;
> @@ -1885,15 +1888,21 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>  	if (!fdev)
>  		return -ENOMEM;
>  
> +	fdev->secure = is_secured;
>  	fdev->cctx = cctx;
>  	fdev->miscdev.minor = MISC_DYNAMIC_MINOR;
>  	fdev->miscdev.fops = &fastrpc_fops;
> -	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s", domain);
> +	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s%s",
> +					    domain, is_secured ? "-secure" : "");

Will this not result in existing userspace using the wrong misc device?

>  	err = misc_register(&fdev->miscdev);
> -	if (err)
> +	if (err) {
>  		kfree(fdev);
> -	else
> -		cctx->fdevice = fdev;
> +	} else {
> +		if (is_secured)
> +			cctx->secure_fdevice = fdev;
> +		else
> +			cctx->fdevice = fdev;
> +	}
>  
>  	return err;
>  }
> @@ -1904,6 +1913,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	struct fastrpc_channel_ctx *data;
>  	int i, err, domain_id = -1;
>  	const char *domain;
> +	bool secure_dsp = false;

Afaict this is only every accessed after first being written. So no need
to initialize it.

>  
>  	err = of_property_read_string(rdev->of_node, "label", &domain);
>  	if (err) {
> @@ -1927,10 +1937,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> -	err = fastrpc_device_register(rdev, data, domains[domain_id]);
> -	if (err) {
> -		kfree(data);
> -		return err;
> +
> +	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
> +	data->secure = secure_dsp;
> +
> +	switch (domain_id) {
> +	case ADSP_DOMAIN_ID:
> +	case MDSP_DOMAIN_ID:
> +	case SDSP_DOMAIN_ID:
> +		err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
> +		if (err)
> +			goto fdev_error;
> +		break;
> +	case CDSP_DOMAIN_ID:
> +		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
> +		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> +		if (err)
> +			goto fdev_error;
> +
> +		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
> +		if (err)
> +			goto fdev_error;
> +		break;
> +	default:
> +		err = -EINVAL;
> +		goto fdev_error;
>  	}
>  
>  	kref_init(&data->refcount);
> @@ -1943,7 +1974,14 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	data->domain_id = domain_id;
>  	data->rpdev = rpdev;
>  
> -	return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	dev_info(rdev, "%s complete for %s with secure flag(%d) return: %d\n",
> +			__func__, domains[domain_id], secure_dsp, err);

I would prefer that we don't spam the kernel log with such useful
information, in particular since it will happen every time we start or
restart a remoteproc with fastrpc. So dev_dbg perhaps?

> +	return err;

I think that in the event that of_platform_populate() actually failed,
you will return an error here, fastrpc_rpmsg_remove() won't be called,
so you won't release the misc device or release &data->refcount. This
issue exists in the code today though...

Regards,
Bjorn

> +
> +fdev_error:
> +	kfree(data);
> +	return err;
>  }
>  
>  static void fastrpc_notify_users(struct fastrpc_user *user)
> @@ -1970,6 +2008,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>  	if (cctx->fdevice)
>  		misc_deregister(&cctx->fdevice->miscdev);
>  
> +	if (cctx->secure_fdevice)
> +		misc_deregister(&cctx->secure_fdevice->miscdev);
> +
>  	of_platform_depopulate(&rpdev->dev);
>  
>  	cctx->rpdev = NULL;
> -- 
> 2.21.0
>