linux-kernel - Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

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

Message-ID: <3dac349c-6470-1673-effb-354da2b52481@roeck-us.net>
Date:   Mon, 14 Mar 2022 18:22:47 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Agathe Porte <agathe.porte@...ia.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>
Cc:     Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Adamski, Krzysztof (Nokia - PL/Wroclaw)" 
        <krzysztof.adamski@...ia.com>
Subject: Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and
 TMP468

Hi Agathe,

On 3/14/22 08:46, Agathe Porte wrote:
> Hi,
> 
> Le 2/22/2022 à 11:36 PM, Guenter Roeck a écrit :
>>   of_property_read_string(child,"label", &data->channel[channel].label);
> 
> Upon trying to merge v7 in our codebase, our static analyzer tool detected that the return code of this function was not checked.
> 
> As I guess putting a label is optional, maybe we should add a `(void)` on the same line just before the function call to clearly indicate that not checking the return value is intentional and that it is not a coding mistake?
> 
> EDIT: As I was reading the implementation of of_property_read_string [1], it will not touch the destination string in case of error. Which means that labels may sit uninitialized and contain garbage data?
> 

Thanks for the feedback.

If of_property_read_string() returns an error, it will not set the pointer
to &data->channel[channel].label, which by default is NULL because the
data structure was allocated with devm_kzalloc(). That means tmp464_is_visible()
will disable the label attribute. I don't see a problem with the current
code.

There are lots of examples in the kernel where the return value from
of_property_read_string() is silently ignored. Not a single one of
those uses a (void) typecast. I don't really want to start making
such changes just to make static analyzers happy.

Thanks,
Guenter