linux-kernel - Re: [PATCH] of/address: Add error logging for of_match

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] [day] [month] [year] [list]

Message-ID: <1aba663d-67d2-4672-805b-7ffc20d0e2ff@quicinc.com>
Date: Tue, 19 Aug 2025 11:56:55 +0800
From: Zhenhua Huang <quic_zhenhuah@...cinc.com>
To: Rob Herring <robh@...nel.org>
CC: <saravanak@...gle.com>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] of/address: Add error logging for of_match_bus() in
 address translation path

Hi Rob, Thanks for your review.

On 8/19/2025 12:49 AM, Rob Herring wrote:
> On Mon, Aug 11, 2025 at 05:53:42PM +0800, Zhenhua Huang wrote:
>> The change introduced in
>> commit 045b14ca5c36 ("of: WARN on deprecated #address-cells/#size-cells handling")
>> triggers a warning on the direct ancestor node when translating properties
>> like "iommu-addresses"/"reg". However, it fails to issue a warning if the
>> ancestor’s ancestor is missing the required cells.
>> For instance, if node_c lacks the necessary properties, no warning will be
>> generated. Potential issues will be trigger further.
> 
> The point of the WARN is to only to check the immediate ancestor.

Yes, that's exactly what I wanted to point out. In fact, during the 
translation phase, a warning should as well be issued when checking the 
node_c as described below, Otherwise, I noticed that the translation 
failure tends to go unnoticed in practice... which is further leading to 
other issues etc.

> 
>> node_c {
>> 		//NO WARN
>> 	node_b {
>> 		//WARN on missing of "address-cells" and "size-cells"
>> 		node_a {
>> 			xxx = <memory_reion>  //contains "iommu-addresses"
>> 		}
>> 	}
>> }
> 
> Whether a warning is appropriate here depends on whether there's
> 'ranges' properties or not. If your schemas are complete, then they
> should warn on missing 'ranges'. If ranges is present, then we should
> get warnings if #address-cells or #size-cells is missing.
> 
>> Since of_match_bus() is now expected to succeed in traslation path,
> 
> now expected? Nothing changed in that aspect.
My bad, The wording may have caused some confusion. What I intended to 
convey is that for example the of_device_alloc() path, as described 
below, of_match_bus() is not expected to always succeed.

> 
>> routine __of_translate_address. Print an error message would help in
>> identifying cases where it fails, making such issues easier to diagnose.
> 
> For errors in the DT (as opposed to errors using the API), it would be
> better if we can check this at build time rather than run-time. And
> generally I think we should already, but there could be some corner case
> that we don't.
> 
>>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@...cinc.com>
>> ---
>>   drivers/of/address.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index f0f8f0dd191c..cd33ab64ccf3 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -515,8 +515,10 @@ static u64 __of_translate_address(struct device_node *node,
>>   	if (parent == NULL)
>>   		return OF_BAD_ADDR;
>>   	bus = of_match_bus(parent);
>> -	if (!bus)
>> +	if (!bus) {
>> +		pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>>   		return OF_BAD_ADDR;
>> +	}
>>   
>>   	/* Count address cells & copy address locally */
>>   	bus->count_cells(dev, &na, &ns);
>> @@ -560,8 +562,10 @@ static u64 __of_translate_address(struct device_node *node,
>>   
>>   		/* Get new parent bus and counts */
>>   		pbus = of_match_bus(parent);
>> -		if (!pbus)
>> +		if (!pbus) {
>> +			pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>>   			return OF_BAD_ADDR;
> 
> If there's no case we expect of_match_bus() failing is correct
> operation, then the error msg should be in the of_match_bus() function
> rather than duplicated here. I'm not sure if there is any such case.
Yeah...
That’s what I initially did. However, in a case where the node doesn’t 
have a "reg" property (as with the "default" of_bus), the path below 
will call of_match_bus() and fail. In such scenarios, its failure should 
be considered expected ?
of_device_alloc
	of_address_count(np);
	..
		__of_get_address
			of_match_bus()

I moved the error checking into __of_translate_address then, limiting it 
to cases where actual address translation is being performed. Because it 
appears to be MUST successful in the scenario.

> 
> Rob