Received-SPF: pass (google.com: domain of linux-kernel+bounces-74761-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 21 Feb 2024 16:15:12 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Colin Ian King <colin.i.king@gmail.com>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>,
	Zhang Rui <rui.zhang@intel.com>, Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org, kernel-janitors@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH][next] ACPI: thermal_lib: Add missing checks for errors
 in return code ret
Message-ID: <4b78456b-071f-46c0-bb47-919b493cab79@moroto.mountain>
References: <20240221123936.3421462-1-colin.i.king@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240221123936.3421462-1-colin.i.king@gmail.com>

On Wed, Feb 21, 2024 at 12:39:36PM +0000, Colin Ian King wrote:
> Static analysis with clang scan build detected various return codes
> being assigned and not checked. The calls to the trip point functions
> probably should be error return checked as these can fail because
> of unlikely issues such as invalid ACPI object names or ACPI value
> evaluation failures.
> 
> Cleans up clang scan warnings, such as:
> drivers/acpi/thermal_lib.c:106:9: warning: 2nd function call argument
> is an uninitialized value [core.CallAndMessage]
>         return thermal_temp(ret, temp_decik, ret_temp);
> 
> 
> Fixes: 6908097aa5a7 ("ACPI: thermal_lib: Add functions returning temperature in deci-Kelvin")
> Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
> ---
> 
> Note: Not tested.
> 
> ---
>  drivers/acpi/thermal_lib.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/acpi/thermal_lib.c b/drivers/acpi/thermal_lib.c
> index 4e0519ca9739..4d27048ef075 100644
> --- a/drivers/acpi/thermal_lib.c
> +++ b/drivers/acpi/thermal_lib.c
> @@ -103,6 +103,9 @@ int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_tem
>  	int temp_decik;
>  	int ret = acpi_active_trip_temp(adev, id, &temp_decik);
>  
> +	if (ret)
> +		return ret;
> +
>  	return thermal_temp(ret, temp_decik, ret_temp);
                            ^^^
The "ret" variable is checked at the start of the thermal_temp()
function which means the code works the same before and after your
patch.

However, the static checker is correct that we are passing uninitialized
data to this function, it's just never used if "ret" is non-zero.

In this case, the rule is that if the function is parsed inline then
it's not a bug, but if it's not inline then it is a bug.  Technically,
passing uninitialized data to a function is undefined behavior in C but
Linus said that didn't really match with real life and that everyone
should recognize that inlines are different and update the tools
accordingly.  (Something like that, I don't have the email in front of
me).

The other thing to consider is that tools like MEMSan (?) will see
non-inline function calls and we're reading from uninitialized memory
and trigger a warning for that.

In this code, I suspect that thermal_temp() will be inlined so it's
probably a false positive in Clang.

regards,
dan carpenter