linux-kernel - Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <7d5af347-749a-43e3-8321-445569f5b78c@foss.st.com>
Date: Fri, 16 Feb 2024 09:15:04 +0100
From: Christophe Kerello <christophe.kerello@...s.st.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <conor+dt@...nel.org>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support



On 2/15/24 19:56, Krzysztof Kozlowski wrote:
> On 15/02/2024 10:00, Christophe Kerello wrote:
>>
>>
>> On 2/14/24 11:07, Krzysztof Kozlowski wrote:
>>> On 13/02/2024 14:15, Christophe Kerello wrote:
>>>>>> +
>>>>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (resource >= FMC2_MAX_RESOURCES)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
>>>>
>>>> Hi Krzysztof,
>>>>
>>>>>
>>>>> No checking of read value?
>>>>>
>>>>
>>>> No, it should never failed.
>>>
>>> And you tested that neither smatch, sparse nor Coverity report here
>>> warnings?
>>>
>>
>> Hi Krzysztof,
>>
>> There is a lot of driver in the Kernel that are using same
>> implementation, and I am surprised to not have had this comment 4 years
>> ago when the driver was introduced.
> 
> Really? Care to give some pointers? Heh, I don't know what to respond to
> it. Either you say that my comment is incorrect or you say that it's
> okay to sneak poor code if no one notices? We can argue on the first,
> whether my comment is reasonable or not. But if you claim that previous
> poor choice of code is argument of bringing more of such poor choices,
> then we are done here. It's the oldest argument: someone did it that
> way, so I can do the same. Nope.
> 
>>
>> So, how should I proceed? Shall I initialize all local variables used by
>> regmap_read? Or shall I check the return value of regmap_read?
>> And, as there is a lot of regmap_read call in this driver, shall I fix
>> them in a separate patch?
> 
> regmap operations, depending on the regmap used, can fail. Most of the
> errors are result of static configuration, e.g. alignment, regmap in
> cache mode etc. Then certain regmap implementations can produce errors,
> which is not a static condition but dynamic.
> 
> You have neither error checking nor value initialization. You risk here
> to have quite tricky to find, unnoticeable bugs, if there any mistake
> leading to regmap errors.
> 
> Indeed neither smatch nor sparse report this as error currently, but
> maybe that's their limitation?
> 

Hi Krzysztof,

I will check the return value of regmap_read API.

Regards,
Christophe Kerello.

> 
> Best regards,
> Krzysztof
>