linux-kernel - Re: [PATCH] hwmon: Use 64-bit arithmetic instead of 32-bit

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

Message-ID: <89e646a2-9377-4544-91e6-cfc55bbbf783@roeck-us.net>
Date: Thu, 30 Jan 2025 06:46:54 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Ваторопин Андрей
 <a.vatoropin@...t.ru>, Jean Delvare <jdelvare@...e.com>
Cc: "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>
Subject: Re: [PATCH] hwmon: Use 64-bit arithmetic instead of 32-bit

On 1/29/25 23:03, Ваторопин Андрей wrote:
> From: Andrey Vatoropin <a.vatoropin@...t.ru>
> 
> Add suffix ULL to constant 500 in order to give the compiler complete
> information about the proper arithmetic to use. Notice that this
> constant is used in a context that expects an expression of type
> u64 (64 bits, unsigned).
> 
> The expression PCC_NUM_RETRIES * cppc_ss->latency, which at
> preprocessing time translates to cppc_ss->latency; is currently
> being evaluated using 32-bit arithmetic.
> 
> This is similar to commit b52f45110502 ("ACPI / CPPC: Use 64-bit
> arithmetic instead of 32-bit")
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 

First, this does not affect the entire hardware monitoring subsystem,
just a single driver. The subject is misleading.

Second, the result of the calculation is used as parameter of a function
which takes unsigned int as parameter, and calculates jiffies from it.
If the number of jiffies to be calculated ever exceeds 4,294,967,295
microseconds, the code has severe other problems to be concerned about.

In other words, your patch does not fix anything, and neither does the
referenced commit. It just helps hiding a (hopefully non-existing) different
problem.

Yes, commit b52f45110502 "fixes" a similar "problem", and also completely
misses the point, since the 64-bit result of the calculation is also used
as parameter to usecs_to_jiffies(). The same applies to commit 8338b74a750c5
and maybe others.

I am not going to apply this patch.

Guenter

> Signed-off-by: Andrey Vatoropin <a.vatoropin@...t.ru>
> ---
>   drivers/hwmon/xgene-hwmon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index 1e3bd129a922..43a08ddb964b 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -61,7 +61,7 @@
>    * Arbitrary retries in case the remote processor is slow to respond
>    * to PCC commands
>    */
> -#define PCC_NUM_RETRIES			500
> +#define PCC_NUM_RETRIES			500ULL
>   
>   #define ASYNC_MSG_FIFO_SIZE		16
>   #define MBOX_OP_TIMEOUTMS		1000