diff mbox series

[4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

Message ID 20221213140724.8612-5-quic_sibis@quicinc.com
State New
Headers
Series Fix XPU violation during modem metadata authentication |

Commit Message

Sibi Sankar Dec. 13, 2022, 2:07 p.m. UTC 
  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.

 drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 24 deletions(-)

Comments

Robin Murphy Dec. 13, 2022, 3:07 p.m. UTC | #1 
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.

Thanks,
Robin.

>   drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
>   1 file changed, 61 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index fddb63cffee0..8264275ecbd0 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -211,6 +211,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;
> @@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>   {
>   	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
>   	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> +	void *mdata_region;
>   	struct page **pages;
>   	struct page *page;
>   	dma_addr_t phys;
> @@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>   	if (IS_ERR(metadata))
>   		return PTR_ERR(metadata);
>   
> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!page) {
> -		kfree(metadata);
> -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> -		return -ENOMEM;
> -	}
> +	if (qproc->mdata_phys) {
> +		mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> +		if (!mdata_region) {
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> +				&qproc->mdata_phys, size);
> +			ret = -EBUSY;
> +			goto free_dma_attrs;
> +		}
>   
> -	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> -	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> -	if (!pages) {
> -		ret = -ENOMEM;
> -		goto free_dma_attrs;
> -	}
> +		memcpy(mdata_region, metadata, size);
> +		memunmap(mdata_region);
> +		phys = qproc->mdata_phys;
> +	} else {
> +		page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +		if (!page) {
> +			kfree(metadata);
> +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> +			return -ENOMEM;
> +		}
>   
> -	for (i = 0; i < count; i++)
> -		pages[i] = nth_page(page, i);
> +		count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +		pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> +		if (!pages) {
> +			ret = -ENOMEM;
> +			goto free_dma_attrs;
> +		}
>   
> -	vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> -	kfree(pages);
> -	if (!vaddr) {
> -		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
> -		ret = -EBUSY;
> -		goto free_dma_attrs;
> -	}
> +		for (i = 0; i < count; i++)
> +			pages[i] = nth_page(page, i);
>   
> -	memcpy(vaddr, metadata, size);
> +		vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> +		kfree(pages);
> +		if (!vaddr) {
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
> +			ret = -EBUSY;
> +			goto free_dma_attrs;
> +		}
>   
> -	vunmap(vaddr);
> +		memcpy(vaddr, metadata, size);
> +
> +		vunmap(vaddr);
> +	}
>   
>   	/* Hypervisor mapping to access metadata by modem */
>   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> @@ -1008,7 +1024,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, page, phys, dma_attrs);
> +	if (!qproc->mdata_phys)
> +		dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
>   	kfree(metadata);
>   
>   	return ret < 0 ? ret : 0;
> @@ -1882,6 +1899,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;
>   }
>
Sibi Sankar Dec. 13, 2022, 3:57 p.m. UTC | #2 
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.

- Sibi

> 
> Thanks,
> Robin.
> 
>>   drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
>>   1 file changed, 61 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index fddb63cffee0..8264275ecbd0 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -211,6 +211,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;
>> @@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5 
>> *qproc, const struct firmware *fw,
>>   {
>>       unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | 
>> DMA_ATTR_NO_KERNEL_MAPPING;
>>       unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
>> +    void *mdata_region;
>>       struct page **pages;
>>       struct page *page;
>>       dma_addr_t phys;
>> @@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5 
>> *qproc, const struct firmware *fw,
>>       if (IS_ERR(metadata))
>>           return PTR_ERR(metadata);
>> -    page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, 
>> dma_attrs);
>> -    if (!page) {
>> -        kfree(metadata);
>> -        dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> -        return -ENOMEM;
>> -    }
>> +    if (qproc->mdata_phys) {
>> +        mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
>> +        if (!mdata_region) {
>> +            dev_err(qproc->dev, "unable to map memory region: 
>> %pa+%zx\n",
>> +                &qproc->mdata_phys, size);
>> +            ret = -EBUSY;
>> +            goto free_dma_attrs;
>> +        }
>> -    count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> -    pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
>> -    if (!pages) {
>> -        ret = -ENOMEM;
>> -        goto free_dma_attrs;
>> -    }
>> +        memcpy(mdata_region, metadata, size);
>> +        memunmap(mdata_region);
>> +        phys = qproc->mdata_phys;
>> +    } else {
>> +        page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, 
>> dma_attrs);
>> +        if (!page) {
>> +            kfree(metadata);
>> +            dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> +            return -ENOMEM;
>> +        }
>> -    for (i = 0; i < count; i++)
>> -        pages[i] = nth_page(page, i);
>> +        count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +        pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
>> +        if (!pages) {
>> +            ret = -ENOMEM;
>> +            goto free_dma_attrs;
>> +        }
>> -    vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
>> -    kfree(pages);
>> -    if (!vaddr) {
>> -        dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", 
>> &phys, size);
>> -        ret = -EBUSY;
>> -        goto free_dma_attrs;
>> -    }
>> +        for (i = 0; i < count; i++)
>> +            pages[i] = nth_page(page, i);
>> -    memcpy(vaddr, metadata, size);
>> +        vaddr = vmap(pages, count, flags, 
>> pgprot_dmacoherent(PAGE_KERNEL));
>> +        kfree(pages);
>> +        if (!vaddr) {
>> +            dev_err(qproc->dev, "unable to map memory region: 
>> %pa+%zx\n", &phys, size);
>> +            ret = -EBUSY;
>> +            goto free_dma_attrs;
>> +        }
>> -    vunmap(vaddr);
>> +        memcpy(vaddr, metadata, size);
>> +
>> +        vunmap(vaddr);
>> +    }
>>       /* Hypervisor mapping to access metadata by modem */
>>       mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>> @@ -1008,7 +1024,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, page, phys, dma_attrs);
>> +    if (!qproc->mdata_phys)
>> +        dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
>>       kfree(metadata);
>>       return ret < 0 ? ret : 0;
>> @@ -1882,6 +1899,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;
>>   }
Manivannan Sadhasivam Dec. 13, 2022, 4:07 p.m. UTC | #3 
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?

Thanks,
Mani

> - Sibi
> 
> > 
> > Thanks,
> > Robin.
> > 
> > >   drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
> > >   1 file changed, 61 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
> > > b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index fddb63cffee0..8264275ecbd0 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -211,6 +211,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;
> > > @@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5
> > > *qproc, const struct firmware *fw,
> > >   {
> > >       unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS |
> > > DMA_ATTR_NO_KERNEL_MAPPING;
> > >       unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> > > +    void *mdata_region;
> > >       struct page **pages;
> > >       struct page *page;
> > >       dma_addr_t phys;
> > > @@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5
> > > *qproc, const struct firmware *fw,
> > >       if (IS_ERR(metadata))
> > >           return PTR_ERR(metadata);
> > > -    page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL,
> > > dma_attrs);
> > > -    if (!page) {
> > > -        kfree(metadata);
> > > -        dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > -        return -ENOMEM;
> > > -    }
> > > +    if (qproc->mdata_phys) {
> > > +        mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> > > +        if (!mdata_region) {
> > > +            dev_err(qproc->dev, "unable to map memory region:
> > > %pa+%zx\n",
> > > +                &qproc->mdata_phys, size);
> > > +            ret = -EBUSY;
> > > +            goto free_dma_attrs;
> > > +        }
> > > -    count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > > -    pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> > > -    if (!pages) {
> > > -        ret = -ENOMEM;
> > > -        goto free_dma_attrs;
> > > -    }
> > > +        memcpy(mdata_region, metadata, size);
> > > +        memunmap(mdata_region);
> > > +        phys = qproc->mdata_phys;
> > > +    } else {
> > > +        page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL,
> > > dma_attrs);
> > > +        if (!page) {
> > > +            kfree(metadata);
> > > +            dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > +            return -ENOMEM;
> > > +        }
> > > -    for (i = 0; i < count; i++)
> > > -        pages[i] = nth_page(page, i);
> > > +        count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > > +        pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> > > +        if (!pages) {
> > > +            ret = -ENOMEM;
> > > +            goto free_dma_attrs;
> > > +        }
> > > -    vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> > > -    kfree(pages);
> > > -    if (!vaddr) {
> > > -        dev_err(qproc->dev, "unable to map memory region:
> > > %pa+%zx\n", &phys, size);
> > > -        ret = -EBUSY;
> > > -        goto free_dma_attrs;
> > > -    }
> > > +        for (i = 0; i < count; i++)
> > > +            pages[i] = nth_page(page, i);
> > > -    memcpy(vaddr, metadata, size);
> > > +        vaddr = vmap(pages, count, flags,
> > > pgprot_dmacoherent(PAGE_KERNEL));
> > > +        kfree(pages);
> > > +        if (!vaddr) {
> > > +            dev_err(qproc->dev, "unable to map memory region:
> > > %pa+%zx\n", &phys, size);
> > > +            ret = -EBUSY;
> > > +            goto free_dma_attrs;
> > > +        }
> > > -    vunmap(vaddr);
> > > +        memcpy(vaddr, metadata, size);
> > > +
> > > +        vunmap(vaddr);
> > > +    }
> > >       /* Hypervisor mapping to access metadata by modem */
> > >       mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> > > @@ -1008,7 +1024,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, page, phys, dma_attrs);
> > > +    if (!qproc->mdata_phys)
> > > +        dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
> > >       kfree(metadata);
> > >       return ret < 0 ? ret : 0;
> > > @@ -1882,6 +1899,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;
> > >   }
Krzysztof Kozlowski Dec. 13, 2022, 7:47 p.m. UTC | #4 
On 13/12/2022 15: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>
> ---

Thank you for your patch. There is something to discuss/improve.
>  
>  	return ret < 0 ? ret : 0;
> @@ -1882,6 +1899,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");

Bindings do not allow to have child "metadata", do they?

> +		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;
>  }
>  

Best regards,
Krzysztof
Sibi Sankar Dec. 14, 2022, 10:33 a.m. UTC | #5 
On 12/14/22 01:17, Krzysztof Kozlowski wrote:
> On 13/12/2022 15: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>
>> ---
> 
> Thank you for your patch. There is something to discuss/improve.
>>   
>>   	return ret < 0 ? ret : 0;
>> @@ -1882,6 +1899,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");
> 
> Bindings do not allow to have child "metadata", do they?

memory-region property was used to specify mba/mpss region in a phandle
array only from SC7180 SoC. All the older dtbs in the wild/upstream
still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
as to not break bindings when newer kernel uses a older dtb.

- Sibi

> 
>> +		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;
>>   }
>>   
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Dec. 14, 2022, 11:28 a.m. UTC | #6 
On 14/12/2022 11:33, Sibi Sankar wrote:
> 
> 
> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>> On 13/12/2022 15: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>
>>> ---
>>
>> Thank you for your patch. There is something to discuss/improve.
>>>   
>>>   	return ret < 0 ? ret : 0;
>>> @@ -1882,6 +1899,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");
>>
>> Bindings do not allow to have child "metadata", do they?
> 
> memory-region property was used to specify mba/mpss region in a phandle
> array only from SC7180 SoC. All the older dtbs in the wild/upstream
> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
> as to not break bindings when newer kernel uses a older dtb.

This does not explain why you extend the driver without extending the
bindings. You do not do it for legacy stuff but for SC7180. But even for
legacy devices you cannot add new properties without having it in some
legacy bindings.


Best regards,
Krzysztof
Sibi Sankar Dec. 14, 2022, 11:51 a.m. UTC | #7 
On 12/14/22 16:58, Krzysztof Kozlowski wrote:
> On 14/12/2022 11:33, Sibi Sankar wrote:
>>
>>
>> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>>> On 13/12/2022 15: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>
>>>> ---
>>>
>>> Thank you for your patch. There is something to discuss/improve.
>>>>    
>>>>    	return ret < 0 ? ret : 0;
>>>> @@ -1882,6 +1899,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");
>>>
>>> Bindings do not allow to have child "metadata", do they?
>>
>> memory-region property was used to specify mba/mpss region in a phandle
>> array only from SC7180 SoC. All the older dtbs in the wild/upstream
>> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
>> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
>> as to not break bindings when newer kernel uses a older dtb.
> 
> This does not explain why you extend the driver without extending the
> bindings. You do not do it for legacy stuff but for SC7180. But even for
> legacy devices you cannot add new properties without having it in some
> legacy bindings.

https://patchwork.kernel.org/project/linux-arm-msm/patch/20221213140724.8612-4-quic_sibis@quicinc.com/

The legacy bindings are a part of patch 3 ^^.

> 
> 
> Best regards,
> Krzysztof
>
Robin Murphy Dec. 14, 2022, 12:49 p.m. UTC | #8 
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.
Krzysztof Kozlowski Dec. 15, 2022, 8:51 a.m. UTC | #9 
On 14/12/2022 12:51, Sibi Sankar wrote:
> 
> 
> On 12/14/22 16:58, Krzysztof Kozlowski wrote:
>> On 14/12/2022 11:33, Sibi Sankar wrote:
>>>
>>>
>>> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>>>> On 13/12/2022 15: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>
>>>>> ---
>>>>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>    
>>>>>    	return ret < 0 ? ret : 0;
>>>>> @@ -1882,6 +1899,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");
>>>>
>>>> Bindings do not allow to have child "metadata", do they?
>>>
>>> memory-region property was used to specify mba/mpss region in a phandle
>>> array only from SC7180 SoC. All the older dtbs in the wild/upstream
>>> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
>>> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
>>> as to not break bindings when newer kernel uses a older dtb.
>>
>> This does not explain why you extend the driver without extending the
>> bindings. You do not do it for legacy stuff but for SC7180. But even for
>> legacy devices you cannot add new properties without having it in some
>> legacy bindings.
> 
> https://patchwork.kernel.org/project/linux-arm-msm/patch/20221213140724.8612-4-quic_sibis@quicinc.com/
> 
> The legacy bindings are a part of patch 3 ^^.

Ah, ok.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index fddb63cffee0..8264275ecbd0 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -211,6 +211,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;
@@ -935,6 +936,7 @@  static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 {
 	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
 	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
+	void *mdata_region;
 	struct page **pages;
 	struct page *page;
 	dma_addr_t phys;
@@ -951,34 +953,48 @@  static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
-	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
-	if (!page) {
-		kfree(metadata);
-		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
-		return -ENOMEM;
-	}
+	if (qproc->mdata_phys) {
+		mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
+		if (!mdata_region) {
+			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
+				&qproc->mdata_phys, size);
+			ret = -EBUSY;
+			goto free_dma_attrs;
+		}
 
-	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
-	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
-	if (!pages) {
-		ret = -ENOMEM;
-		goto free_dma_attrs;
-	}
+		memcpy(mdata_region, metadata, size);
+		memunmap(mdata_region);
+		phys = qproc->mdata_phys;
+	} else {
+		page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
+		if (!page) {
+			kfree(metadata);
+			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
+			return -ENOMEM;
+		}
 
-	for (i = 0; i < count; i++)
-		pages[i] = nth_page(page, i);
+		count = PAGE_ALIGN(size) >> PAGE_SHIFT;
+		pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
+		if (!pages) {
+			ret = -ENOMEM;
+			goto free_dma_attrs;
+		}
 
-	vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
-	kfree(pages);
-	if (!vaddr) {
-		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
-		ret = -EBUSY;
-		goto free_dma_attrs;
-	}
+		for (i = 0; i < count; i++)
+			pages[i] = nth_page(page, i);
 
-	memcpy(vaddr, metadata, size);
+		vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
+		kfree(pages);
+		if (!vaddr) {
+			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
+			ret = -EBUSY;
+			goto free_dma_attrs;
+		}
 
-	vunmap(vaddr);
+		memcpy(vaddr, metadata, size);
+
+		vunmap(vaddr);
+	}
 
 	/* Hypervisor mapping to access metadata by modem */
 	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
@@ -1008,7 +1024,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, page, phys, dma_attrs);
+	if (!qproc->mdata_phys)
+		dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
 	kfree(metadata);
 
 	return ret < 0 ? ret : 0;
@@ -1882,6 +1899,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;
 }