Prevent undefined behavior by adding WARN_ONCE() when find_ec_sensor_index()
returns a negative value.

Even though unsupported attributes are filtered out by asus_ec_hwmon_is_visible(),
a programming error could still cause an invalid sensor access.

Instead of silently returning an error, log a warning to highlight the problem,
and propagate the original error code.

Update both asus_ec_hwmon_read() and asus_ec_hwmon_read_string() accordingly.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: d0ddfd241e57 ("hwmon: (asus-ec-sensors) add driver for ASUS EC")
Signed-off-by: Alexei Safin <a.safin@rosa.ru>
---
v2: Use WARN_ONCE() instead of returning -EINVAL, and update
    both asus_ec_hwmon_read() and asus_ec_hwmon_read_string()
    as suggested by Eugene Shalygin.
    
 drivers/hwmon/asus-ec-sensors.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index d893cfd1cb82..a563d7acef2e 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -820,9 +820,8 @@ static int asus_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 	struct ec_sensors_data *state = dev_get_drvdata(dev);
 	int sidx = find_ec_sensor_index(state, type, channel);
 
-	if (sidx < 0) {
+	if (WARN_ONCE(sidx < 0, "asus-ec-sensors: sensor not found\n"))
 		return sidx;
-	}
 
 	ret = get_cached_value_or_update(dev, sidx, state, &value);
 	if (!ret) {
@@ -839,6 +838,10 @@ static int asus_ec_hwmon_read_string(struct device *dev,
 {
 	struct ec_sensors_data *state = dev_get_drvdata(dev);
 	int sensor_index = find_ec_sensor_index(state, type, channel);
+
+	if (WARN_ONCE(sensor_index < 0, "asus-ec-sensors: sensor not found\n"))
+		return sensor_index;
+
 	*str = get_sensor_info(state, sensor_index)->label;
 
 	return 0;
-- 
2.39.5 (Apple Git-154)

On Fri, 25 Apr 2025 at 22:00, Alexei Safin <a.safin@rosa.ru> wrote:
>
> Prevent undefined behavior by adding WARN_ONCE() when find_ec_sensor_index()
> returns a negative value.

I'm not sure about WARN_ONCE, does it bring anything useful? Clients
see the error in the return value, sensor reading is absent, do we
need to duplicate that in the log? Guenter, may I leave it up to you,
please?

Best regards,
Eugene

On 4/28/25 00:11, Eugene Shalygin wrote:
> On Fri, 25 Apr 2025 at 22:00, Alexei Safin <a.safin@rosa.ru> wrote:
>>
>> Prevent undefined behavior by adding WARN_ONCE() when find_ec_sensor_index()
>> returns a negative value.
> 
> I'm not sure about WARN_ONCE, does it bring anything useful? Clients
> see the error in the return value, sensor reading is absent, do we
> need to duplicate that in the log? Guenter, may I leave it up to you,
> please?
> 

If this is seen, it is an implementation error which needs to be fixed.
Returning an error to userspace will leave users annoyed but will not
result in a fix. The warning backtrace is warranted in this situation.

Guenter

> If this is seen, it is an implementation error which needs to be fixed.
> Returning an error to userspace will leave users annoyed but will not
> result in a fix. The warning backtrace is warranted in this situation.

Thanks for the explanation! I'm still not convinced that such a
generic error message (without the type and channel) can be of great
help. Something serious needs to happen to trigger this error, like a
hardware change, or a RAM failure... If the purpose of the message is
only to grab attention, it could say that explicitly. But anyway,

Reviewed-by: Eugene Shalygin <eugene.shalygin@gmail.com>

Eugene

On 4/29/25 04:30, Eugene Shalygin wrote:
>> If this is seen, it is an implementation error which needs to be fixed.
>> Returning an error to userspace will leave users annoyed but will not
>> result in a fix. The warning backtrace is warranted in this situation.
> 
> Thanks for the explanation! I'm still not convinced that such a
> generic error message (without the type and channel) can be of great

Feel free to suggest a better one. Maybe I misunderstood your earlier
concerns, but it seemed to me that you objected to having a message
in the first place, not to its contents.

Thanks,
Guenter

> help. Something serious needs to happen to trigger this error, like a
> hardware change, or a RAM failure... If the purpose of the message is
> only to grab attention, it could say that explicitly. But anyway,
> 
> Reviewed-by: Eugene Shalygin <eugene.shalygin@gmail.com>
> 
> Eugene

On Wed, 30 Apr 2025 at 06:13, Guenter Roeck <linux@roeck-us.net> wrote:

> > Thanks for the explanation! I'm still not convinced that such a
> > generic error message (without the type and channel) can be of great
>
> Feel free to suggest a better one. Maybe I misunderstood your earlier
> concerns, but it seemed to me that you objected to having a message
> in the first place, not to its contents.

The only two conditions I can imaging to trigger the log message are
hardware and/or firmware change and RAM instability. In both cases the
message is not helpful: for the hardware change case I would need
sensor type and channel, for the RAM-related cases I would need to
know how often the problem repeats itself. Neither is possible with
this log message, and therefore I'd rather log every time and with
sensor type and channel.

Best regards,
Eugene

On 5/1/25 01:07, Eugene Shalygin wrote:
> On Wed, 30 Apr 2025 at 06:13, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> Thanks for the explanation! I'm still not convinced that such a
>>> generic error message (without the type and channel) can be of great
>>
>> Feel free to suggest a better one. Maybe I misunderstood your earlier
>> concerns, but it seemed to me that you objected to having a message
>> in the first place, not to its contents.
> 
> The only two conditions I can imaging to trigger the log message are
> hardware and/or firmware change and RAM instability. In both cases the
> message is not helpful: for the hardware change case I would need
> sensor type and channel, for the RAM-related cases I would need to
> know how often the problem repeats itself. Neither is possible with
> this log message, and therefore I'd rather log every time and with
> sensor type and channel.
> 

The message would only be expected if there is some programming error,
where is_visible does not correctly disable non-existing sensors.
That is the situation the messages try to catch.

You do have an excellent point, though: The messages should display the
sensor type and channel. As for logging it every time: No, because _if_
there is a programming error it would clog the log, so once is enough
to tell "something is wrong with the code, fix it". The same is actually
true if the hardware changes "under the hood".

Regarding RAM errors: Those won't be caught by checks like this. You'd have
to litter the whole kernel with checks at almost every line of code
to even have a remote chance to catch problems like bit flips. Even
then you'd still not catch cases where the code itself is changed.
Even trying to catch that would be futile.

Thanks,
Guenter

On Thu, 1 May 2025 at 14:34, Guenter Roeck <linux@roeck-us.net> wrote:

> You do have an excellent point, though: The messages should display the
> sensor type and channel. As for logging it every time: No, because _if_
> there is a programming error it would clog the log, so once is enough
> to tell "something is wrong with the code, fix it". The same is actually
> true if the hardware changes "under the hood".

But WARM_ONCE() can not behave the way we want, as far as I
understand, printing a single warning for each sensor type and
channel, can it?

> Regarding RAM errors: Those won't be caught by checks like this. You'd have
> to litter the whole kernel with checks at almost every line of code
> to even have a remote chance to catch problems like bit flips. Even
> then you'd still not catch cases where the code itself is changed.
> Even trying to catch that would be futile.

I already had a bug report (race condition in this driver, another log
message in the code) that kept me puzzled until the user reported that
it was caused by a malfunctioning RAM module.

Eugene