All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
@ 2023-11-13 19:38 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-13 19:38 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Joel Stanley, Andrew Jeffery
  Cc: linux-hwmon, linux-arm-kernel, linux-aspeed, linux-kernel,
	Gustavo A. R. Silva, linux-hardening

Based on the documentation below, the maximum number of Fan tach
channels is 16:

Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
 45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
 46                 integer value in the range 0 through 15, with 0 indicating
 47                 Fan tach channel 0 and 15 indicating Fan tach channel 15.
 48                 At least one Fan tach input channel is required.

However, the compiler doesn't know that, and legitimaly warns about a potential
overwrite in array `u8 fan_tach_ch_source[16]` in `struct aspeed_pwm_tacho_data`,
in case `index` takes a value outside the boundaries of the array:

drivers/hwmon/aspeed-pwm-tacho.c:
179 struct aspeed_pwm_tacho_data {
...
184         bool fan_tach_present[16];
...
193         u8 fan_tach_ch_source[16];
...
196 };

In function ‘aspeed_create_fan_tach_channel’,
    inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
    inlined from ‘aspeed_pwm_tacho_probe’ at drivers/hwmon/aspeed-pwm-tacho.c:936:9:
drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  751 |                 priv->fan_tach_ch_source[index] = pwm_source;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into destination object ‘fan_tach_ch_source’ of size 16
  193 |         u8 fan_tach_ch_source[16];
      |            ^~~~~~~~~~~~~~~~~~

Fix this by sanity checking `index` before using it to index arrays of
size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
completeness, add a `pr_err()` message to display in the unlikely case
`0 > index >= 16`.

This is probably the last remaining -Wstringop-overflow issue in the
kernel, and this patch helps with the ongoing efforts to enable such
compiler option globally.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/hwmon/aspeed-pwm-tacho.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 997df4b40509..092a81916325 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -166,6 +166,8 @@
 
 #define MAX_CDEV_NAME_LEN 16
 
+#define MAX_ASPEED_FAN_TACH_CHANNELS 16
+
 struct aspeed_cooling_device {
 	char name[16];
 	struct aspeed_pwm_tacho_data *priv;
@@ -181,7 +183,7 @@ struct aspeed_pwm_tacho_data {
 	struct reset_control *rst;
 	unsigned long clk_freq;
 	bool pwm_present[8];
-	bool fan_tach_present[16];
+	bool fan_tach_present[MAX_ASPEED_FAN_TACH_CHANNELS];
 	u8 type_pwm_clock_unit[3];
 	u8 type_pwm_clock_division_h[3];
 	u8 type_pwm_clock_division_l[3];
@@ -190,7 +192,7 @@ struct aspeed_pwm_tacho_data {
 	u16 type_fan_tach_unit[3];
 	u8 pwm_port_type[8];
 	u8 pwm_port_fan_ctrl[8];
-	u8 fan_tach_ch_source[16];
+	u8 fan_tach_ch_source[MAX_ASPEED_FAN_TACH_CHANNELS];
 	struct aspeed_cooling_device *cdev[8];
 	const struct attribute_group *groups[3];
 };
@@ -746,10 +748,14 @@ static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv,
 
 	for (val = 0; val < count; val++) {
 		index = fan_tach_ch[val];
-		aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
-		priv->fan_tach_present[index] = true;
-		priv->fan_tach_ch_source[index] = pwm_source;
-		aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
+		if (index < MAX_ASPEED_FAN_TACH_CHANNELS) {
+			aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
+			priv->fan_tach_present[index] = true;
+			priv->fan_tach_ch_source[index] = pwm_source;
+			aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
+		} else {
+			pr_err("Invalid Fan Tach input channel %u\n.", index);
+		}
 	}
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
@ 2023-11-13 19:38 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-13 19:38 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Joel Stanley, Andrew Jeffery
  Cc: linux-hwmon, linux-arm-kernel, linux-aspeed, linux-kernel,
	Gustavo A. R. Silva, linux-hardening

Based on the documentation below, the maximum number of Fan tach
channels is 16:

Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
 45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
 46                 integer value in the range 0 through 15, with 0 indicating
 47                 Fan tach channel 0 and 15 indicating Fan tach channel 15.
 48                 At least one Fan tach input channel is required.

However, the compiler doesn't know that, and legitimaly warns about a potential
overwrite in array `u8 fan_tach_ch_source[16]` in `struct aspeed_pwm_tacho_data`,
in case `index` takes a value outside the boundaries of the array:

drivers/hwmon/aspeed-pwm-tacho.c:
179 struct aspeed_pwm_tacho_data {
...
184         bool fan_tach_present[16];
...
193         u8 fan_tach_ch_source[16];
...
196 };

In function ‘aspeed_create_fan_tach_channel’,
    inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
    inlined from ‘aspeed_pwm_tacho_probe’ at drivers/hwmon/aspeed-pwm-tacho.c:936:9:
drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  751 |                 priv->fan_tach_ch_source[index] = pwm_source;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into destination object ‘fan_tach_ch_source’ of size 16
  193 |         u8 fan_tach_ch_source[16];
      |            ^~~~~~~~~~~~~~~~~~

Fix this by sanity checking `index` before using it to index arrays of
size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
completeness, add a `pr_err()` message to display in the unlikely case
`0 > index >= 16`.

This is probably the last remaining -Wstringop-overflow issue in the
kernel, and this patch helps with the ongoing efforts to enable such
compiler option globally.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/hwmon/aspeed-pwm-tacho.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 997df4b40509..092a81916325 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -166,6 +166,8 @@
 
 #define MAX_CDEV_NAME_LEN 16
 
+#define MAX_ASPEED_FAN_TACH_CHANNELS 16
+
 struct aspeed_cooling_device {
 	char name[16];
 	struct aspeed_pwm_tacho_data *priv;
@@ -181,7 +183,7 @@ struct aspeed_pwm_tacho_data {
 	struct reset_control *rst;
 	unsigned long clk_freq;
 	bool pwm_present[8];
-	bool fan_tach_present[16];
+	bool fan_tach_present[MAX_ASPEED_FAN_TACH_CHANNELS];
 	u8 type_pwm_clock_unit[3];
 	u8 type_pwm_clock_division_h[3];
 	u8 type_pwm_clock_division_l[3];
@@ -190,7 +192,7 @@ struct aspeed_pwm_tacho_data {
 	u16 type_fan_tach_unit[3];
 	u8 pwm_port_type[8];
 	u8 pwm_port_fan_ctrl[8];
-	u8 fan_tach_ch_source[16];
+	u8 fan_tach_ch_source[MAX_ASPEED_FAN_TACH_CHANNELS];
 	struct aspeed_cooling_device *cdev[8];
 	const struct attribute_group *groups[3];
 };
@@ -746,10 +748,14 @@ static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv,
 
 	for (val = 0; val < count; val++) {
 		index = fan_tach_ch[val];
-		aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
-		priv->fan_tach_present[index] = true;
-		priv->fan_tach_ch_source[index] = pwm_source;
-		aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
+		if (index < MAX_ASPEED_FAN_TACH_CHANNELS) {
+			aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
+			priv->fan_tach_present[index] = true;
+			priv->fan_tach_ch_source[index] = pwm_source;
+			aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
+		} else {
+			pr_err("Invalid Fan Tach input channel %u\n.", index);
+		}
 	}
 }
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
  2023-11-13 19:38 ` Gustavo A. R. Silva
@ 2023-11-14 14:52   ` Guenter Roeck
  -1 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2023-11-14 14:52 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Jean Delvare, Joel Stanley, Andrew Jeffery, linux-hwmon,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-hardening

On Mon, Nov 13, 2023 at 01:38:12PM -0600, Gustavo A. R. Silva wrote:
> Based on the documentation below, the maximum number of Fan tach
> channels is 16:
> 
> Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
>  45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
>  46                 integer value in the range 0 through 15, with 0 indicating
>  47                 Fan tach channel 0 and 15 indicating Fan tach channel 15.
>  48                 At least one Fan tach input channel is required.
> 
> However, the compiler doesn't know that, and legitimaly warns about a potential
> overwrite in array `u8 fan_tach_ch_source[16]` in `struct aspeed_pwm_tacho_data`,
> in case `index` takes a value outside the boundaries of the array:
> 

All that doesn't warrant introducing checkpatch warnings.

> drivers/hwmon/aspeed-pwm-tacho.c:
> 179 struct aspeed_pwm_tacho_data {
> ...
> 184         bool fan_tach_present[16];
> ...
> 193         u8 fan_tach_ch_source[16];
> ...
> 196 };
> 
> In function ‘aspeed_create_fan_tach_channel’,
>     inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
>     inlined from ‘aspeed_pwm_tacho_probe’ at drivers/hwmon/aspeed-pwm-tacho.c:936:9:
> drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>   751 |                 priv->fan_tach_ch_source[index] = pwm_source;
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
> drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into destination object ‘fan_tach_ch_source’ of size 16
>   193 |         u8 fan_tach_ch_source[16];
>       |            ^~~~~~~~~~~~~~~~~~
> 
> Fix this by sanity checking `index` before using it to index arrays of
> size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
> completeness, add a `pr_err()` message to display in the unlikely case
> `0 > index >= 16`.
> 
> This is probably the last remaining -Wstringop-overflow issue in the
> kernel, and this patch helps with the ongoing efforts to enable such
> compiler option globally.
> 

I am sorry, but this description almost completely misses the point.
The real issue is that the values in aspeed,fan-tach-ch are not range
checked, which can cause real problems if is elements are set to values
larger than 15. This is a real problem and has nothing to do with string
overflows.

> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/hwmon/aspeed-pwm-tacho.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index 997df4b40509..092a81916325 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -166,6 +166,8 @@
>  
>  #define MAX_CDEV_NAME_LEN 16
>  
> +#define MAX_ASPEED_FAN_TACH_CHANNELS 16
> +
>  struct aspeed_cooling_device {
>  	char name[16];
>  	struct aspeed_pwm_tacho_data *priv;
> @@ -181,7 +183,7 @@ struct aspeed_pwm_tacho_data {
>  	struct reset_control *rst;
>  	unsigned long clk_freq;
>  	bool pwm_present[8];
> -	bool fan_tach_present[16];
> +	bool fan_tach_present[MAX_ASPEED_FAN_TACH_CHANNELS];
>  	u8 type_pwm_clock_unit[3];
>  	u8 type_pwm_clock_division_h[3];
>  	u8 type_pwm_clock_division_l[3];
> @@ -190,7 +192,7 @@ struct aspeed_pwm_tacho_data {
>  	u16 type_fan_tach_unit[3];
>  	u8 pwm_port_type[8];
>  	u8 pwm_port_fan_ctrl[8];
> -	u8 fan_tach_ch_source[16];
> +	u8 fan_tach_ch_source[MAX_ASPEED_FAN_TACH_CHANNELS];
>  	struct aspeed_cooling_device *cdev[8];
>  	const struct attribute_group *groups[3];
>  };
> @@ -746,10 +748,14 @@ static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv,
>  
>  	for (val = 0; val < count; val++) {
>  		index = fan_tach_ch[val];
> -		aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
> -		priv->fan_tach_present[index] = true;
> -		priv->fan_tach_ch_source[index] = pwm_source;
> -		aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
> +		if (index < MAX_ASPEED_FAN_TACH_CHANNELS) {
> +			aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
> +			priv->fan_tach_present[index] = true;
> +			priv->fan_tach_ch_source[index] = pwm_source;
> +			aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
> +		} else {
> +			pr_err("Invalid Fan Tach input channel %u\n.", index);

This should use dev_err() (and, yes, that means dev needs to be passed
as argument), and the function should return -EINVAL if this is
encountered. Also, error handling should come first.

		if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
			dev_err(dev, "Invalid Fan Tach input channel %u\n.", index);
			return -EINVAL;
		}

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
@ 2023-11-14 14:52   ` Guenter Roeck
  0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2023-11-14 14:52 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Jean Delvare, Joel Stanley, Andrew Jeffery, linux-hwmon,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-hardening

On Mon, Nov 13, 2023 at 01:38:12PM -0600, Gustavo A. R. Silva wrote:
> Based on the documentation below, the maximum number of Fan tach
> channels is 16:
> 
> Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
>  45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
>  46                 integer value in the range 0 through 15, with 0 indicating
>  47                 Fan tach channel 0 and 15 indicating Fan tach channel 15.
>  48                 At least one Fan tach input channel is required.
> 
> However, the compiler doesn't know that, and legitimaly warns about a potential
> overwrite in array `u8 fan_tach_ch_source[16]` in `struct aspeed_pwm_tacho_data`,
> in case `index` takes a value outside the boundaries of the array:
> 

All that doesn't warrant introducing checkpatch warnings.

> drivers/hwmon/aspeed-pwm-tacho.c:
> 179 struct aspeed_pwm_tacho_data {
> ...
> 184         bool fan_tach_present[16];
> ...
> 193         u8 fan_tach_ch_source[16];
> ...
> 196 };
> 
> In function ‘aspeed_create_fan_tach_channel’,
>     inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
>     inlined from ‘aspeed_pwm_tacho_probe’ at drivers/hwmon/aspeed-pwm-tacho.c:936:9:
> drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>   751 |                 priv->fan_tach_ch_source[index] = pwm_source;
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
> drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into destination object ‘fan_tach_ch_source’ of size 16
>   193 |         u8 fan_tach_ch_source[16];
>       |            ^~~~~~~~~~~~~~~~~~
> 
> Fix this by sanity checking `index` before using it to index arrays of
> size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
> completeness, add a `pr_err()` message to display in the unlikely case
> `0 > index >= 16`.
> 
> This is probably the last remaining -Wstringop-overflow issue in the
> kernel, and this patch helps with the ongoing efforts to enable such
> compiler option globally.
> 

I am sorry, but this description almost completely misses the point.
The real issue is that the values in aspeed,fan-tach-ch are not range
checked, which can cause real problems if is elements are set to values
larger than 15. This is a real problem and has nothing to do with string
overflows.

> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/hwmon/aspeed-pwm-tacho.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index 997df4b40509..092a81916325 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -166,6 +166,8 @@
>  
>  #define MAX_CDEV_NAME_LEN 16
>  
> +#define MAX_ASPEED_FAN_TACH_CHANNELS 16
> +
>  struct aspeed_cooling_device {
>  	char name[16];
>  	struct aspeed_pwm_tacho_data *priv;
> @@ -181,7 +183,7 @@ struct aspeed_pwm_tacho_data {
>  	struct reset_control *rst;
>  	unsigned long clk_freq;
>  	bool pwm_present[8];
> -	bool fan_tach_present[16];
> +	bool fan_tach_present[MAX_ASPEED_FAN_TACH_CHANNELS];
>  	u8 type_pwm_clock_unit[3];
>  	u8 type_pwm_clock_division_h[3];
>  	u8 type_pwm_clock_division_l[3];
> @@ -190,7 +192,7 @@ struct aspeed_pwm_tacho_data {
>  	u16 type_fan_tach_unit[3];
>  	u8 pwm_port_type[8];
>  	u8 pwm_port_fan_ctrl[8];
> -	u8 fan_tach_ch_source[16];
> +	u8 fan_tach_ch_source[MAX_ASPEED_FAN_TACH_CHANNELS];
>  	struct aspeed_cooling_device *cdev[8];
>  	const struct attribute_group *groups[3];
>  };
> @@ -746,10 +748,14 @@ static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv,
>  
>  	for (val = 0; val < count; val++) {
>  		index = fan_tach_ch[val];
> -		aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
> -		priv->fan_tach_present[index] = true;
> -		priv->fan_tach_ch_source[index] = pwm_source;
> -		aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
> +		if (index < MAX_ASPEED_FAN_TACH_CHANNELS) {
> +			aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
> +			priv->fan_tach_present[index] = true;
> +			priv->fan_tach_ch_source[index] = pwm_source;
> +			aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
> +		} else {
> +			pr_err("Invalid Fan Tach input channel %u\n.", index);

This should use dev_err() (and, yes, that means dev needs to be passed
as argument), and the function should return -EINVAL if this is
encountered. Also, error handling should come first.

		if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
			dev_err(dev, "Invalid Fan Tach input channel %u\n.", index);
			return -EINVAL;
		}

Thanks,
Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
  2023-11-14 14:52   ` Guenter Roeck
@ 2023-11-14 21:20     ` Gustavo A. R. Silva
  -1 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-14 21:20 UTC (permalink / raw)
  To: Guenter Roeck, Gustavo A. R. Silva
  Cc: Jean Delvare, Joel Stanley, Andrew Jeffery, linux-hwmon,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-hardening



On 11/14/23 08:52, Guenter Roeck wrote:
> On Mon, Nov 13, 2023 at 01:38:12PM -0600, Gustavo A. R. Silva wrote:
>> Based on the documentation below, the maximum number of Fan tach
>> channels is 16:
>>
>> Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
>>   45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
>>   46                 integer value in the range 0 through 15, with 0 indicating
>>   47                 Fan tach channel 0 and 15 indicating Fan tach channel 15.
>>   48                 At least one Fan tach input channel is required.
>>
>> However, the compiler doesn't know that, and legitimaly warns about a potential
>> overwrite in array `u8 fan_tach_ch_source[16]` in `struct aspeed_pwm_tacho_data`,
>> in case `index` takes a value outside the boundaries of the array:
>>
> 
> All that doesn't warrant introducing checkpatch warnings.

Do you mean this?

WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#17:
  46                 integer value in the range 0 through 15, with 0 indicating

I honestly didn't consider that relevant, and I didn't want to alter the format of
the Doc text.

However, if you want me to split any offending line, that's not a problem. Just let
me know. :)

> 
>> drivers/hwmon/aspeed-pwm-tacho.c:
>> 179 struct aspeed_pwm_tacho_data {
>> ...
>> 184         bool fan_tach_present[16];
>> ...
>> 193         u8 fan_tach_ch_source[16];
>> ...
>> 196 };
>>
>> In function ‘aspeed_create_fan_tach_channel’,
>>      inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
>>      inlined from ‘aspeed_pwm_tacho_probe’ at drivers/hwmon/aspeed-pwm-tacho.c:936:9:
>> drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>>    751 |                 priv->fan_tach_ch_source[index] = pwm_source;
>>        |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
>> drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
>> drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into destination object ‘fan_tach_ch_source’ of size 16
>>    193 |         u8 fan_tach_ch_source[16];
>>        |            ^~~~~~~~~~~~~~~~~~
>>
>> Fix this by sanity checking `index` before using it to index arrays of
>> size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
>> completeness, add a `pr_err()` message to display in the unlikely case
>> `0 > index >= 16`.
>>
>> This is probably the last remaining -Wstringop-overflow issue in the
>> kernel, and this patch helps with the ongoing efforts to enable such
>> compiler option globally.
>>
> 
> I am sorry, but this description almost completely misses the point.
> The real issue is that the values in aspeed,fan-tach-ch are not range
> checked, which can cause real problems if is elements are set to values
> larger than 15. This is a real problem and has nothing to do with string
> overflows.

Yeah, the above paragraph was extra, and I removed it in v2[1]. The rest
of the changelog text describes the issue in the code.

> 
> This should use dev_err() (and, yes, that means dev needs to be passed
> as argument), and the function should return -EINVAL if this is
> encountered. Also, error handling should come first.
> 
> 		if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
> 			dev_err(dev, "Invalid Fan Tach input channel %u\n.", index);
> 			return -EINVAL;
> 		}

Done in v2.

Thanks a lot for the feedback.
--
Gustavo

[1] https://lore.kernel.org/linux-hardening/ZVPQJIP26dIzRAr6@work/

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
@ 2023-11-14 21:20     ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-14 21:20 UTC (permalink / raw)
  To: Guenter Roeck, Gustavo A. R. Silva
  Cc: Jean Delvare, Joel Stanley, Andrew Jeffery, linux-hwmon,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-hardening



On 11/14/23 08:52, Guenter Roeck wrote:
> On Mon, Nov 13, 2023 at 01:38:12PM -0600, Gustavo A. R. Silva wrote:
>> Based on the documentation below, the maximum number of Fan tach
>> channels is 16:
>>
>> Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
>>   45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
>>   46                 integer value in the range 0 through 15, with 0 indicating
>>   47                 Fan tach channel 0 and 15 indicating Fan tach channel 15.
>>   48                 At least one Fan tach input channel is required.
>>
>> However, the compiler doesn't know that, and legitimaly warns about a potential
>> overwrite in array `u8 fan_tach_ch_source[16]` in `struct aspeed_pwm_tacho_data`,
>> in case `index` takes a value outside the boundaries of the array:
>>
> 
> All that doesn't warrant introducing checkpatch warnings.

Do you mean this?

WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#17:
  46                 integer value in the range 0 through 15, with 0 indicating

I honestly didn't consider that relevant, and I didn't want to alter the format of
the Doc text.

However, if you want me to split any offending line, that's not a problem. Just let
me know. :)

> 
>> drivers/hwmon/aspeed-pwm-tacho.c:
>> 179 struct aspeed_pwm_tacho_data {
>> ...
>> 184         bool fan_tach_present[16];
>> ...
>> 193         u8 fan_tach_ch_source[16];
>> ...
>> 196 };
>>
>> In function ‘aspeed_create_fan_tach_channel’,
>>      inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
>>      inlined from ‘aspeed_pwm_tacho_probe’ at drivers/hwmon/aspeed-pwm-tacho.c:936:9:
>> drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>>    751 |                 priv->fan_tach_ch_source[index] = pwm_source;
>>        |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
>> drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
>> drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into destination object ‘fan_tach_ch_source’ of size 16
>>    193 |         u8 fan_tach_ch_source[16];
>>        |            ^~~~~~~~~~~~~~~~~~
>>
>> Fix this by sanity checking `index` before using it to index arrays of
>> size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
>> completeness, add a `pr_err()` message to display in the unlikely case
>> `0 > index >= 16`.
>>
>> This is probably the last remaining -Wstringop-overflow issue in the
>> kernel, and this patch helps with the ongoing efforts to enable such
>> compiler option globally.
>>
> 
> I am sorry, but this description almost completely misses the point.
> The real issue is that the values in aspeed,fan-tach-ch are not range
> checked, which can cause real problems if is elements are set to values
> larger than 15. This is a real problem and has nothing to do with string
> overflows.

Yeah, the above paragraph was extra, and I removed it in v2[1]. The rest
of the changelog text describes the issue in the code.

> 
> This should use dev_err() (and, yes, that means dev needs to be passed
> as argument), and the function should return -EINVAL if this is
> encountered. Also, error handling should come first.
> 
> 		if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
> 			dev_err(dev, "Invalid Fan Tach input channel %u\n.", index);
> 			return -EINVAL;
> 		}

Done in v2.

Thanks a lot for the feedback.
--
Gustavo

[1] https://lore.kernel.org/linux-hardening/ZVPQJIP26dIzRAr6@work/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-14 21:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-13 19:38 [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel() Gustavo A. R. Silva
2023-11-13 19:38 ` Gustavo A. R. Silva
2023-11-14 14:52 ` Guenter Roeck
2023-11-14 14:52   ` Guenter Roeck
2023-11-14 21:20   ` Gustavo A. R. Silva
2023-11-14 21:20     ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.