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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 15 Feb 2024 19:56:04 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Christophe Kerello <christophe.kerello@...s.st.com>,
 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 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?

Best regards,
Krzysztof