linux-kernel - Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2

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]

Message-ID: <d01bed96-7811-4ace-8b92-1ee9fafac649@wanadoo.fr>
Date: Sun, 8 Sep 2024 18:58:52 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Biju Das <biju.das.jz@...renesas.com>,
 Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
 "linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
 "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>
Subject: Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: Fix an error handling
 path in rcar_gen3_phy_usb2_probe()

Le 08/09/2024 à 18:39, Biju Das a écrit :
> Hi Christophe JAILLET,
> 
> Thanks for the patch.
> 
>> -----Original Message-----
>> From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> Sent: Saturday, September 7, 2024 2:59 PM
>> Subject: [PATCH v2] phy: renesas: rcar-gen3-usb2: Fix an error handling path in
>> rcar_gen3_phy_usb2_probe()
>>
>> If an error occurs after the rcar_gen3_phy_usb2_init_bus(),
>> reset_control_assert() must be called, as already done in the remove function.
>>
>> This is fine to re-use the existing error handling path, because even if "channel->rstc" is still NULL
>> at this point, it is safe to call reset_control_assert(NULL).
>>
>> Fixes: 4eae16375357 ("phy: renesas: rcar-gen3-usb2: Add support to initialize the bus")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>> Changes in v2:
>>    - Re-use 'error' to simplify the patch   [claudiu beznea]
>>    - Update the commit description to explain why it is safe.
>>
>> v1:
>> https://lore.kernel.org/all/fc9f7b444f0ca645411868992bbe16514aeccfed.1725652654.git.christophe.jaillet
>> @wanadoo.fr/
>> ---
>>   drivers/phy/renesas/phy-rcar-gen3-usb2.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> index 58e123305152..ccb0b54b70f7 100644
>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> @@ -803,6 +803,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>>   	return 0;
>>
>>   error:
>> +	reset_control_assert(channel->rstc);
> 
> This will result in either kernel crash [1] or reset usage count imbalance in case
> of error [2] and [3] in rcar_gen3_phy_usb2_init_bus() see [4]. Also reset control API is not
> respected for SoCs other than RZ/G3S. For those SoC's reset assert is
> called without calling a get(). Maybe add a check (phy_data->init_bus) for
> assert api's, that guarantees assert is called after calling a get() as it
> valid only for RZ/G3S??
> 
> [1]
> channel->rstc = devm_reset_control_array_get_shared(dev);
> 	if (IS_ERR(channel->rstc))
> 		return PTR_ERR(channel->rstc);
> 
> [2]
> ret = pm_runtime_resume_and_get(dev);
> 	if (ret)
> 		return ret;
> [3]
> ret = reset_control_deassert(channel->rstc);
> 	if (ret)
> 		goto rpm_put;
> 
> [4] https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/reset/core.c#L483

So, if I understand correctly, v1 [5] was correct. :)


I don't think that [1] would crash, because of [6]. It would only 
WARN_ON. But with v1, it is not called.

With v1, reset_control_assert() is not called if 
rcar_gen3_phy_usb2_init_bus() fails. So [2] and [3] can't occur.

I can send a v3, which is the same of v1, or you can pick v1 as-is (if 
I'm correct... :)) or you can just ignore it if "reset control API is 
not respected for SoCs".


If of interest, I spotted it with one of my coccinelle script that 
compares functions called in .remove function, but not in error handling 
path of probe.


CJ

[5]: 
https://lore.kernel.org/all/fc9f7b444f0ca645411868992bbe16514aeccfed.1725652654.git.christophe.jaillet@wanadoo.fr/

[6]: 
https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/reset/core.c#L473

> 
> Cheers,
> Biju
> 
>>   	pm_runtime_disable(dev);
>>
>>   	return ret;
>> --
>> 2.46.0
>>
> 
> 
>