linux-kernel - Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Dec 2022 12:28:30 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sibi Sankar <quic_sibis@...cinc.com>, andersson@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
        manivannan.sadhasivam@...aro.org
Cc:     agross@...nel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        konrad.dybcio@...ainline.org, amit.pundir@...aro.org,
        regressions@...mhuis.info, sumit.semwal@...aro.org,
        will@...nel.org, catalin.marinas@....com, robin.murphy@....com
Subject: Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to
 authenticate modem headers

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@...aro.org>
>>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>>> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.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