linux-kernel - Re: [PATCH v5 1/1] hwmon: Add driver for Astera Labs PT5161L retimer

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: <a05ec2fe-cfe4-48d8-bff4-9f3689c585d3@roeck-us.net>
Date: Mon, 5 Feb 2024 19:26:04 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Cosmo Chou <chou.cosmo@...il.com>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
 conor+dt@...nel.org, jdelvare@...e.com, corbet@....net, broonie@...nel.org,
 naresh.solanki@...ements.com, vincent@...emblay.dev,
 patrick.rudolph@...ements.com, luca.ceresoli@...tlin.com,
 bhelgaas@...gle.com, festevam@...x.de, alexander.stein@...tq-group.com,
 heiko@...ech.de, jernej.skrabec@...il.com, macromorgan@...mail.com,
 forbidden405@...mail.com, sre@...nel.org, linus.walleij@...aro.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
 cosmo.chou@...ntatw.com
Subject: Re: [PATCH v5 1/1] hwmon: Add driver for Astera Labs PT5161L retimer

On 2/5/24 19:05, Cosmo Chou wrote:
> On Tue, Feb 06, 2024 at 3:43 AM +0800, Guenter Roeck wrote:
>>
>> On Mon, Feb 05, 2024 at 11:20:13PM +0800, Cosmo Chou wrote:
>>> This driver implements support for temperature monitoring of Astera Labs
>>> PT5161L series PCIe retimer chips.
>>>
>>> This driver implementation originates from the CSDK available at
>>> Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
>>> The communication protocol utilized is based on the I2C/SMBus standard.
>>>
>>> Signed-off-by: Cosmo Chou <chou.cosmo@...il.com>
>>> ---
>> [ ... ]
>>
>>> +static ssize_t pt5161l_debugfs_read_fw_ver(struct file *file, char __user *buf,
>>> +                                        size_t count, loff_t *ppos)
>>> +{
>>> +     struct pt5161l_data *data = file->private_data;
>>> +     int ret;
>>> +     char ver[32];
>>> +
>>> +     mutex_lock(&data->lock);
>>> +     ret = pt5161l_fwsts_check(data);
>>> +     mutex_unlock(&data->lock);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major,
>>> +                    data->fw_ver.minor, data->fw_ver.build);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>
>> You almost got me here ;-). snprintf() never returns a negative error code,
>> so checking for it is not necessary.
>>
> Oh! You're right.
> 
>>> +     return simple_read_from_buffer(buf, count, ppos, ver, ret + 1);
>>
>> Number of bytes written plus 1 ? Why ?
> It's just to include the string terminator '\0'.
> 

If that was needed, it would be risky. snprintf() truncates the output
if the buffer is not large enough. You might want to consider using
scnprintf() instead. But then I am not sure if that is needed in the first
place. Almost all code I checked doesn't do that, and it seems to be likely
that the few drivers who do that are simply wrong. Can you explain why the
string terminator needs to be added to the output ?

Thanks,
Guenter