Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 14 Dec 2022 12:49:47 +0000 | Subject | Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers | From | Robin Murphy <> |
| |
On 2022-12-13 16:07, Manivannan Sadhasivam wrote: > On Tue, Dec 13, 2022 at 09:27:04PM +0530, Sibi Sankar wrote: >> Hey Robin, >> >> Thanks for taking time to review the series. >> >> On 12/13/22 20:37, Robin Murphy wrote: >>> On 2022-12-13 14:07, Sibi Sankar wrote: >>>> The memory region allocated using dma_alloc_attr with no kernel mapping >>>> attribute set would still be a part of the linear kernel map. Any access >>>> to this 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-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> >>>> --- >>>> >>>> The addition of the carveout and memunmap is required only on SoCs that >>>> mandate memory protection before transferring control to Q6, hence the >>>> driver falls back to dynamic memory allocation in the absence of the >>>> modem metadata carveout. >>> >>> The DMA_ATTR_NO_KERNEL_MAPPING stuff is still broken and pointless, so >>> I'd expect to see this solution replacing it, not being added alongside. >>> It's just silly to say pass the "I don't need a CPU mapping" flag, then >>> manually open-code the same CPU mapping you would have got if you >>> hadn't, in a way that only works at all when a cacheable alias exists >>> anyway. >> >> only a subset of SoCs supported by the driver are affected by >> the bug i.e. on the others dma_alloc_attr would still work >> without problems. I can perhaps drop the NO_KERNEL_MAPPING along >> with the vmap/vunmap and simplify things for those SoCs. >> > > Or perhaps revert fc156629b23a?
Oh, indeed, if it's already self-contained that's even neater. Basically that whole commit is based on a misunderstanding, doesn't actually do what it thinks it does, and you'd be far better off not maintaining the extra code.
Thanks, Robin.
| ![\](/images/icornerr.gif) |