| Message ID | 20230109034843.23759-7-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix XPU violation during modem metadata authentication | expand |
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. 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 >
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 >> >
On Mon, Jan 09, 2023 at 03:35:31PM +0530, Sibi Sankar wrote: > 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. > Oops, I missed that. But yeah it is confusing too with the current way of freeing metadata. I'd suggest using a separate label instead. Thanks, Mani > > > > 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 > > > > >
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; + } + } 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; }
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(-)