hwmon: (cros-ec_hwmon) Fix access to restricted __le16

[PATCH] hwmon: (cros-ec_hwmon) Fix access to restricted __le16

Posted by Guenter Roeck 1 year, 8 months ago

0-day complains:

drivers-hwmon-cros_ec_hwmon.c:sparse:sparse:cast-to-restricted-__le16

Fix by using a __le16 typed variable as parameter to le16_to_cpu().

Fixes: bc3e45258096 ("hwmon: add ChromeOS EC driver")
Cc: Thomas Weißschuh <linux@weissschuh.net>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 41f268fa8260..f586987c3502 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -26,12 +26,13 @@ struct cros_ec_hwmon_priv {
 static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
 {
 	int ret;
+	__le16 __speed;
 
-	ret = cros_ec_cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, speed);
+	ret = cros_ec_cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &__speed);
 	if (ret < 0)
 		return ret;
 
-	*speed = le16_to_cpu(*speed);
+	*speed = le16_to_cpu(__speed);
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH] hwmon: (cros-ec_hwmon) Fix access to restricted __le16

Posted by Thomas Weißschuh 1 year, 8 months ago

Thanks!

On 2024-06-06 11:05:07+0000, Guenter Roeck wrote:
> 0-day complains:
> 
> drivers-hwmon-cros_ec_hwmon.c:sparse:sparse:cast-to-restricted-__le16
> 
> Fix by using a __le16 typed variable as parameter to le16_to_cpu().
> 
> Fixes: bc3e45258096 ("hwmon: add ChromeOS EC driver")
> Cc: Thomas Weißschuh <linux@weissschuh.net>
> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Thomas Weißschuh <linux@weissschuh.net>

Guenter, does sparse work locally for you?

When I run it via "make C=1 drivers/hwmon/cros_ec_hwmon.o" I only get
some random warnings from included headers but nothing of value.

Tzung-Bi, could you pick up this patch through chrome-platform?
(And also "hwmon: (cros_ec) Prevent read overflow in probe()" [0])

[0] https://lore.kernel.org/lkml/42331b70-bd3c-496c-8c79-3ec4faad40b8@moroto.mountain/

> ---
>  drivers/hwmon/cros_ec_hwmon.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 41f268fa8260..f586987c3502 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -26,12 +26,13 @@ struct cros_ec_hwmon_priv {
>  static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
>  {
>  	int ret;
> +	__le16 __speed;
>  
> -	ret = cros_ec_cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, speed);
> +	ret = cros_ec_cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &__speed);
>  	if (ret < 0)
>  		return ret;
>  
> -	*speed = le16_to_cpu(*speed);
> +	*speed = le16_to_cpu(__speed);
>  	return 0;
>  }
>  
> -- 
> 2.39.2
>

Re: [PATCH] hwmon: (cros-ec_hwmon) Fix access to restricted __le16

Posted by Guenter Roeck 1 year, 8 months ago

On 6/6/24 12:53, Thomas Weißschuh wrote:
> Thanks!
> 
> On 2024-06-06 11:05:07+0000, Guenter Roeck wrote:
>> 0-day complains:
>>
>> drivers-hwmon-cros_ec_hwmon.c:sparse:sparse:cast-to-restricted-__le16
>>
>> Fix by using a __le16 typed variable as parameter to le16_to_cpu().
>>
>> Fixes: bc3e45258096 ("hwmon: add ChromeOS EC driver")
>> Cc: Thomas Weißschuh <linux@weissschuh.net>
>> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Acked-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> Guenter, does sparse work locally for you?
> 

It does, but I use the version from git://repo.or.cz/smatch.git.

Guenter

Re: [PATCH] hwmon: (cros-ec_hwmon) Fix access to restricted __le16

Posted by Thomas Weißschuh 1 year, 8 months ago

On 2024-06-06 13:16:59+0000, Guenter Roeck wrote:
> On 6/6/24 12:53, Thomas Weißschuh wrote:
> > Thanks!
> > 
> > On 2024-06-06 11:05:07+0000, Guenter Roeck wrote:
> > > 0-day complains:
> > > 
> > > drivers-hwmon-cros_ec_hwmon.c:sparse:sparse:cast-to-restricted-__le16
> > > 
> > > Fix by using a __le16 typed variable as parameter to le16_to_cpu().
> > > 
> > > Fixes: bc3e45258096 ("hwmon: add ChromeOS EC driver")
> > > Cc: Thomas Weißschuh <linux@weissschuh.net>
> > > Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > Acked-by: Thomas Weißschuh <linux@weissschuh.net>
> > 
> > Guenter, does sparse work locally for you?
> > 
> 
> It does, but I use the version from git://repo.or.cz/smatch.git.

That does indeed look much better, thanks.

I have another question about the endianness conversion in general.
The only places I see doing a conversion are
cros_ec_sensors_cmd_read_u16() and the original cros_ec hwmon driver.

Also the documentation of the EC protocol does not specify anything in
that regard.
Instead there is the following comment in host_event_set_bit():

   /*
    * The overall host event implementation assumes it's running on and
    * communicating with little-endian architectures.
    */

Can the conversion be dropped?

Re: [PATCH] hwmon: (cros-ec_hwmon) Fix access to restricted __le16

Posted by Guenter Roeck 1 year, 8 months ago

On 6/6/24 13:48, Thomas Weißschuh wrote:
> On 2024-06-06 13:16:59+0000, Guenter Roeck wrote:
>> On 6/6/24 12:53, Thomas Weißschuh wrote:
>>> Thanks!
>>>
>>> On 2024-06-06 11:05:07+0000, Guenter Roeck wrote:
>>>> 0-day complains:
>>>>
>>>> drivers-hwmon-cros_ec_hwmon.c:sparse:sparse:cast-to-restricted-__le16
>>>>
>>>> Fix by using a __le16 typed variable as parameter to le16_to_cpu().
>>>>
>>>> Fixes: bc3e45258096 ("hwmon: add ChromeOS EC driver")
>>>> Cc: Thomas Weißschuh <linux@weissschuh.net>
>>>> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> Acked-by: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> Guenter, does sparse work locally for you?
>>>
>>
>> It does, but I use the version from git://repo.or.cz/smatch.git.
> 
> That does indeed look much better, thanks.
> 
> 
> I have another question about the endianness conversion in general.
> The only places I see doing a conversion are
> cros_ec_sensors_cmd_read_u16() and the original cros_ec hwmon driver.
> 
> Also the documentation of the EC protocol does not specify anything in
> that regard.
> Instead there is the following comment in host_event_set_bit():
> 
>     /*
>      * The overall host event implementation assumes it's running on and
>      * communicating with little-endian architectures.
>      */
> 
> Can the conversion be dropped?
> 


Unless it is wrong I don't see a reason to drop it.

Guenter