Re: [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch

[Dwivedi, Avaneesh Kumar (avani)]

On 5/26/2017 7:46 AM, Bjorn Andersson wrote:
> On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>
> > +static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc,
> > +		phys_addr_t addr, size_t size)
> > +{
> > +	struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA,
> > +		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} };
> struct qcom_scm_destVmPerm next = {
> 	.vmid = QCOM_SCM_VMID_MSS_MSA,
> 	.perm = QCOM_SCM_PERM_RW
> };
OK.
> > +	int ret;
> > +
> > +	size = ALIGN(size, SZ_4K);
> I believe it will be cleaner if you just put this alignment directly in
> the function call below.
OK.
> > +	if (!qproc->need_mem_protection)
> > +		return 0;
> Put a blank line here, to give separation between the sections of the
> function.
OK.
> > +	ret = qcom_scm_assign_mem(addr, size,
> > +		BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next));
> 	qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
> 			    BIT(QCOM_SCM_VMID_HLOS), &next, 1);
OK
> > +	if (ret)
> > +		pr_err("%s: Failed to assign memory access, ret = %d\n",
> > +			__func__, ret);
> There's no point in printing an error here and in the calling function,
> but as it makes sense to have the error message to include which memory
> region we operated on (mba vs mpss) I think you should remove the print
> here and keep it in the callers.
OK
> So just return qcom-scm_assign_mem().
OK
> > +	return ret;
> > +}
> > +
> > +static int q6v5_assign_mem_to_linux(struct q6v5 *qproc,
> > +		phys_addr_t addr, size_t size)
> > +{
> > +	struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS,
> > +		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)}
> > +		};
> struct qcom_scm_destVmPerm next = {
> 	.vmid = QCOM_SCM_VMID_HLOS,
> 	.perm = QCOM_SCM_PERM_RWX,
> };
>
> (And add RWX to the list of defines in patch 1)
OK
> [..]
> > @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
> >   		dev_err(qproc->dev,
> >   			"metadata authentication failed: %d\n", ret);
> >   
> > +	/* Metadata authentication done, remove modem access */
> > +	ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size);
> > +	if (ret)
> > +		dev_err(qproc->dev,
> > +			"Failed to reclaim metadata memory, ret - %d\n", ret);
> If this assignment fails (for any reason) we can't return the memory to
> the free pool in Linux, because at some point in the future these pages
> will be allocated to someone else resulting in a memory access violation
> scenario that will be just terrible to debug.
>
> I do however not have a better idea at the moment than just leaking the
> memory.
Then shall we BUG_ON here itself?
> >   	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
> >   
> >   	return ret < 0 ? ret : 0;
> [..]
> > @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
> >   
> >   	ret = q6v5_mpss_load(qproc);
> >   	if (ret)
> > -		goto halt_axi_ports;
> > +		goto reclaim_mem;
> This doesn't allow us to distinguish between failures before or after
> the memory assignment in mpss_load(), so although you're duplicating the
> reclaim code, mpss_load() must be responsible of restoring the state on
> failure.
OK, will move one of reclaim path in mpss_load it self.
> The timeout below still has to reclaim the memory.
Yes that any way will do.
> >   
> >   	ret = wait_for_completion_timeout(&qproc->start_done,
> >   					  msecs_to_jiffies(5000));
> >   	if (ret == 0) {
> >   		dev_err(qproc->dev, "start timed out\n");
> >   		ret = -ETIMEDOUT;
> > -		goto halt_axi_ports;
> > +		goto reclaim_mem;
> >   	}
> >   
> > +	ret = q6v5_assign_mem_to_linux(qproc,
> > +		qproc->mba_phys, qproc->mba_size);
> > +	if (ret)
> > +		dev_err(qproc->dev,
> > +			"Failed to reclaim mba memory, ret - %d\n", ret);
> I think it's okay for symmetrical purposes to keep the memory assigned
> to the remote until we call stop().
Actually MBA image is transferred into internal memory of q6 and does 
not run from DDR
that is why it is OK to release it here. let me know if you dont want to 
do that.
> Although this shows that we should be able to release the mba allocation
> here.
Yes that is true.
> >   	qproc->running = true;
> >   
> >   	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
> > @@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc)
> >   
> >   	return 0;
> >   
> > +reclaim_mem:
> > +	assign_mem_result =
> > +		q6v5_assign_mem_to_linux(qproc,
> > +		qproc->mpss_phys, qproc->mpss_size);
> If this fails we're screwed. We have no way to fail in a way that will
> not free the memory at any later point in time.
>
> So I do believe you should have a BUG_ON(assign_mem_result); here. With
> a clear comment in the lines of:
>
> /*
>   * Failed to reclaim memory, any future access to these pages will cause
>   * security violations and a seemingly random crash
>   */
Yes this is very good suggestion thanks.
> >   halt_axi_ports:
> >   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
> >   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> >   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
> >   	q6v5_clk_disable(qproc->dev, qproc->active_clks,
> >   			 qproc->active_clk_count);
> > +	assign_mem_result =
> > +		q6v5_assign_mem_to_linux(qproc,
> > +		qproc->mba_phys, qproc->mba_size);
> Shouldn't we move them even further down?
There does not seem any restriction w.r.t. keeping it here.
Do you think any side effect ?
> Same BUG_ON() as above.
Yes sure.
> >   assert_reset:
> >   	reset_control_assert(qproc->mss_restart);
> >   disable_vdd:
> Regards,
> Bjorn

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.