diff mbox series

[V3,05/10] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

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

Commit Message

Sibi Sankar Jan. 11, 2023, 11:43 a.m. UTC 
  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>
---

v3:
 * Drop revert no_kernel_mapping since it's already on the list [Mani]
 * kfree metadata from the branch for parity

 drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 6 deletions(-)

Comments

Manivannan Sadhasivam Jan. 11, 2023, 11:54 a.m. UTC | #1 
On Wed, Jan 11, 2023 at 05:13:32PM +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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
> 
> v3:
>  * Drop revert no_kernel_mapping since it's already on the list [Mani]

I thought you are going to include Christoph's patch into your series. That way
all the patches will be in the same series, makig life easier for Bjorn.

Thanks,
Mani

>  * kfree metadata from the branch for parity
> 
>  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..e25d44e20ae7 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) {
> +			kfree(metadata);
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> +				&qproc->mdata_phys, size);
> +			return -EBUSY;
> +		}
> +	} 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
>
Sibi Sankar Jan. 11, 2023, 12:28 p.m. UTC | #2 
Hey Mani,

Thanks for taking time to review the series.

On 1/11/23 17:24, Manivannan Sadhasivam wrote:
> On Wed, Jan 11, 2023 at 05:13:32PM +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>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
>> ---
>>
>> v3:
>>   * Drop revert no_kernel_mapping since it's already on the list [Mani]
> 
> I thought you are going to include Christoph's patch into your series. That way
> all the patches will be in the same series, makig life easier for Bjorn.
> 

Since there were multiple patches in Christoph's original series, I
decided I'll just drop the revert and have it depends on instead.


> Thanks,
> Mani
> 
>>   * kfree metadata from the branch for parity
>>
>>   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..e25d44e20ae7 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) {
>> +			kfree(metadata);
>> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
>> +				&qproc->mdata_phys, size);
>> +			return -EBUSY;
>> +		}
>> +	} 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
>>
>
Manivannan Sadhasivam Jan. 11, 2023, 5:40 p.m. UTC | #3 
On Wed, Jan 11, 2023 at 05:58:49PM +0530, Sibi Sankar wrote:
> Hey Mani,
> 
> Thanks for taking time to review the series.
> 
> On 1/11/23 17:24, Manivannan Sadhasivam wrote:
> > On Wed, Jan 11, 2023 at 05:13:32PM +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>
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > > ---
> > > 
> > > v3:
> > >   * Drop revert no_kernel_mapping since it's already on the list [Mani]
> > 
> > I thought you are going to include Christoph's patch into your series. That way
> > all the patches will be in the same series, makig life easier for Bjorn.
> > 
> 
> Since there were multiple patches in Christoph's original series, I
> decided I'll just drop the revert and have it depends on instead.
> 

There is only one patch in that series and that was independent of the
remoteproc change. So, it should be fine to include the revert patch to this
series.

Thanks,
Mani

> 
> > Thanks,
> > Mani
> > 
> > >   * kfree metadata from the branch for parity
> > > 
> > >   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..e25d44e20ae7 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) {
> > > +			kfree(metadata);
> > > +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> > > +				&qproc->mdata_phys, size);
> > > +			return -EBUSY;
> > > +		}
> > > +	} 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 mbox series

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index e2f765f87ec9..e25d44e20ae7 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) {
+			kfree(metadata);
+			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
+				&qproc->mdata_phys, size);
+			return -EBUSY;
+		}
+	} 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;
 }