Messages in this thread | | | Date | Mon, 9 Jan 2023 15:35:31 +0530 | Subject | Re: [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers | From | Sibi Sankar <> |
| |
Hey Mani,
On 1/9/23 14:02, Manivannan Sadhasivam wrote: > On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote: >> Any access to the dynamically allocated metadata region by the application >> processor after assigning it to the remote Q6 will result in a XPU >> violation. Fix this by replacing the dynamically allocated memory region >> with a no-map carveout and unmap the modem metadata memory region before >> passing control to the remote Q6. >> >> Reported-and-tested-by: Amit Pundir <amit.pundir@linaro.org> >> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch") >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> >> v2: >> * Revert no_kernel_mapping [Mani/Robin] >> >> drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++---- >> 1 file changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c >> index e2f765f87ec9..b7a158751cef 100644 >> --- a/drivers/remoteproc/qcom_q6v5_mss.c >> +++ b/drivers/remoteproc/qcom_q6v5_mss.c >> @@ -215,6 +215,7 @@ struct q6v5 { >> size_t mba_size; >> size_t dp_size; >> >> + phys_addr_t mdata_phys; >> phys_addr_t mpss_phys; >> phys_addr_t mpss_reloc; >> size_t mpss_size; >> @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, >> if (IS_ERR(metadata)) >> return PTR_ERR(metadata); >> >> - ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); >> - if (!ptr) { >> - kfree(metadata); >> - dev_err(qproc->dev, "failed to allocate mdt buffer\n"); >> - return -ENOMEM; >> + if (qproc->mdata_phys) { >> + phys = qproc->mdata_phys; >> + ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC); >> + if (!ptr) { >> + dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", >> + &qproc->mdata_phys, size); >> + ret = -EBUSY; >> + goto free_dma_attrs; > > There is no memory to free at this point.
we would just free the metadata in the no-map carveout scenario since mdata_phys wouldn't be NULL. I can do a kfree(metadata) directly from this branch and return as well if you think it makes things more readable.
> > Thanks, > Mani > >> + } >> + } else { >> + ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); >> + if (!ptr) { >> + kfree(metadata); >> + dev_err(qproc->dev, "failed to allocate mdt buffer\n"); >> + return -ENOMEM; >> + } >> } >> >> memcpy(ptr, metadata, size); >> >> + if (qproc->mdata_phys) >> + memunmap(ptr); >> + >> /* Hypervisor mapping to access metadata by modem */ >> mdata_perm = BIT(QCOM_SCM_VMID_HLOS); >> ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true, >> @@ -1010,7 +1025,8 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, >> "mdt buffer not reclaimed system may become unstable\n"); >> >> free_dma_attrs: >> - dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs); >> + if (!qproc->mdata_phys) >> + dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs); >> kfree(metadata); >> >> return ret < 0 ? ret : 0; >> @@ -1893,6 +1909,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc) >> qproc->mpss_phys = qproc->mpss_reloc = r.start; >> qproc->mpss_size = resource_size(&r); >> >> + if (!child) { >> + node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2); >> + } else { >> + child = of_get_child_by_name(qproc->dev->of_node, "metadata"); >> + node = of_parse_phandle(child, "memory-region", 0); >> + of_node_put(child); >> + } >> + >> + if (!node) >> + return 0; >> + >> + ret = of_address_to_resource(node, 0, &r); >> + of_node_put(node); >> + if (ret) { >> + dev_err(qproc->dev, "unable to resolve metadata region\n"); >> + return ret; >> + } >> + >> + qproc->mdata_phys = r.start; >> + >> return 0; >> } >> >> -- >> 2.17.1 >> >
| |