All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug
@ 2023-07-31 21:07 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-07-31 21:07 UTC (permalink / raw)
  To: Markus Mayer, Broadcom internal kernel review list,
	Rafael J. Wysocki, Viresh Kumar, Florian Fainelli
  Cc: linux-pm, linux-arm-kernel, linux-kernel, Gustavo A. R. Silva,
	linux-hardening

Update the iteration conditions in the for() loop to avoid writing in
array `table` beyond its allocated size at:

drivers/cpufreq/brcmstb-avs-cpufreq.c:
449         table[i].frequency = CPUFREQ_TABLE_END;

This fixes the following -Warray-bounds warning seen after building
ARM with multi_v7_defconfig (GCC 13):
In function 'brcm_avs_get_freq_table',
    inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: warning: array subscript 5 is outside array bounds of 'void[60]' [-Warray-bounds=]
  449 |         table[i].frequency = CPUFREQ_TABLE_END;
In file included from include/linux/node.h:18,
                 from include/linux/cpu.h:17,
                 from include/linux/cpufreq.h:12,
                 from drivers/cpufreq/brcmstb-avs-cpufreq.c:44:
In function 'devm_kmalloc_array',
    inlined from 'devm_kcalloc' at include/linux/device.h:328:9,
    inlined from 'brcm_avs_get_freq_table' at drivers/cpufreq/brcmstb-avs-cpufreq.c:437:10,
    inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
include/linux/device.h:323:16: note: at offset 60 into object of size 60 allocated by 'devm_kmalloc'
  323 |         return devm_kmalloc(dev, bytes, flags);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -Warray-bounds.

Link: https://github.com/KSPP/linux/issues/324
Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Update changelog text. Add more details.

v1:
 - Link: https://lore.kernel.org/linux-hardening/ZMgfWEA0GAN%2FRog8@work/

 drivers/cpufreq/brcmstb-avs-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 1bdd513bcd19..99ba2d707eff 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -439,7 +439,7 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
 	if (!table)
 		return ERR_PTR(-ENOMEM);
 
-	for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
+	for (i = AVS_PSTATE_P0; i < AVS_PSTATE_MAX; i++) {
 		ret = brcm_avs_set_pstate(priv, i);
 		if (ret)
 			return ERR_PTR(ret);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug
@ 2023-07-31 21:07 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-07-31 21:07 UTC (permalink / raw)
  To: Markus Mayer, Broadcom internal kernel review list,
	Rafael J. Wysocki, Viresh Kumar, Florian Fainelli
  Cc: linux-pm, linux-arm-kernel, linux-kernel, Gustavo A. R. Silva,
	linux-hardening

Update the iteration conditions in the for() loop to avoid writing in
array `table` beyond its allocated size at:

drivers/cpufreq/brcmstb-avs-cpufreq.c:
449         table[i].frequency = CPUFREQ_TABLE_END;

This fixes the following -Warray-bounds warning seen after building
ARM with multi_v7_defconfig (GCC 13):
In function 'brcm_avs_get_freq_table',
    inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: warning: array subscript 5 is outside array bounds of 'void[60]' [-Warray-bounds=]
  449 |         table[i].frequency = CPUFREQ_TABLE_END;
In file included from include/linux/node.h:18,
                 from include/linux/cpu.h:17,
                 from include/linux/cpufreq.h:12,
                 from drivers/cpufreq/brcmstb-avs-cpufreq.c:44:
In function 'devm_kmalloc_array',
    inlined from 'devm_kcalloc' at include/linux/device.h:328:9,
    inlined from 'brcm_avs_get_freq_table' at drivers/cpufreq/brcmstb-avs-cpufreq.c:437:10,
    inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
include/linux/device.h:323:16: note: at offset 60 into object of size 60 allocated by 'devm_kmalloc'
  323 |         return devm_kmalloc(dev, bytes, flags);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -Warray-bounds.

Link: https://github.com/KSPP/linux/issues/324
Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Update changelog text. Add more details.

v1:
 - Link: https://lore.kernel.org/linux-hardening/ZMgfWEA0GAN%2FRog8@work/

 drivers/cpufreq/brcmstb-avs-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 1bdd513bcd19..99ba2d707eff 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -439,7 +439,7 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
 	if (!table)
 		return ERR_PTR(-ENOMEM);
 
-	for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
+	for (i = AVS_PSTATE_P0; i < AVS_PSTATE_MAX; i++) {
 		ret = brcm_avs_set_pstate(priv, i);
 		if (ret)
 			return ERR_PTR(ret);
-- 
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 v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug
  2023-07-31 21:07 ` Gustavo A. R. Silva
@ 2023-07-31 23:16   ` Kees Cook
  -1 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-07-31 23:16 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Markus Mayer, Broadcom internal kernel review list,
	Rafael J. Wysocki, Viresh Kumar, Florian Fainelli, linux-pm,
	linux-arm-kernel, linux-kernel, linux-hardening

On Mon, Jul 31, 2023 at 03:07:20PM -0600, Gustavo A. R. Silva wrote:
> Update the iteration conditions in the for() loop to avoid writing in
> array `table` beyond its allocated size at:
> 
> drivers/cpufreq/brcmstb-avs-cpufreq.c:
> 449         table[i].frequency = CPUFREQ_TABLE_END;
> 
> This fixes the following -Warray-bounds warning seen after building
> ARM with multi_v7_defconfig (GCC 13):
> In function 'brcm_avs_get_freq_table',
>     inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
> drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: warning: array subscript 5 is outside array bounds of 'void[60]' [-Warray-bounds=]
>   449 |         table[i].frequency = CPUFREQ_TABLE_END;

#define AVS_PSTATE_P0           0x0
#define AVS_PSTATE_P1           0x1
#define AVS_PSTATE_P2           0x2
#define AVS_PSTATE_P3           0x3
#define AVS_PSTATE_P4           0x4
#define AVS_PSTATE_MAX          AVS_PSTATE_P4

        table = devm_kcalloc(dev, AVS_PSTATE_MAX + 1, sizeof(*table),
                             GFP_KERNEL);
	...
        for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
		...
	}
        table[i].frequency = CPUFREQ_TABLE_END;

I see "AVS_PSTATE_MAX + 1" being used for the allocation, and so the
loop is likely correctly doing P0 through P4. If there is supposed to be
a terminating element in the table, I think the correct fix would be to
allocate an additional element, not stop the loop from processing P4.

> [...]
> -	for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
> +	for (i = AVS_PSTATE_P0; i < AVS_PSTATE_MAX; i++) {
>  		ret = brcm_avs_set_pstate(priv, i);
>  		if (ret)
>  			return ERR_PTR(ret);

-Kees

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug
@ 2023-07-31 23:16   ` Kees Cook
  0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-07-31 23:16 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Markus Mayer, Broadcom internal kernel review list,
	Rafael J. Wysocki, Viresh Kumar, Florian Fainelli, linux-pm,
	linux-arm-kernel, linux-kernel, linux-hardening

On Mon, Jul 31, 2023 at 03:07:20PM -0600, Gustavo A. R. Silva wrote:
> Update the iteration conditions in the for() loop to avoid writing in
> array `table` beyond its allocated size at:
> 
> drivers/cpufreq/brcmstb-avs-cpufreq.c:
> 449         table[i].frequency = CPUFREQ_TABLE_END;
> 
> This fixes the following -Warray-bounds warning seen after building
> ARM with multi_v7_defconfig (GCC 13):
> In function 'brcm_avs_get_freq_table',
>     inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
> drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: warning: array subscript 5 is outside array bounds of 'void[60]' [-Warray-bounds=]
>   449 |         table[i].frequency = CPUFREQ_TABLE_END;

#define AVS_PSTATE_P0           0x0
#define AVS_PSTATE_P1           0x1
#define AVS_PSTATE_P2           0x2
#define AVS_PSTATE_P3           0x3
#define AVS_PSTATE_P4           0x4
#define AVS_PSTATE_MAX          AVS_PSTATE_P4

        table = devm_kcalloc(dev, AVS_PSTATE_MAX + 1, sizeof(*table),
                             GFP_KERNEL);
	...
        for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
		...
	}
        table[i].frequency = CPUFREQ_TABLE_END;

I see "AVS_PSTATE_MAX + 1" being used for the allocation, and so the
loop is likely correctly doing P0 through P4. If there is supposed to be
a terminating element in the table, I think the correct fix would be to
allocate an additional element, not stop the loop from processing P4.

> [...]
> -	for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
> +	for (i = AVS_PSTATE_P0; i < AVS_PSTATE_MAX; i++) {
>  		ret = brcm_avs_set_pstate(priv, i);
>  		if (ret)
>  			return ERR_PTR(ret);

-Kees

-- 
Kees Cook

_______________________________________________
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 v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug
  2023-07-31 23:16   ` Kees Cook
@ 2023-08-01  3:20     ` Gustavo A. R. Silva
  -1 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-08-01  3:20 UTC (permalink / raw)
  To: Kees Cook, Gustavo A. R. Silva
  Cc: Markus Mayer, Broadcom internal kernel review list,
	Rafael J. Wysocki, Viresh Kumar, Florian Fainelli, linux-pm,
	linux-arm-kernel, linux-kernel, linux-hardening



On 7/31/23 17:16, Kees Cook wrote:
> On Mon, Jul 31, 2023 at 03:07:20PM -0600, Gustavo A. R. Silva wrote:
>> Update the iteration conditions in the for() loop to avoid writing in
>> array `table` beyond its allocated size at:
>>
>> drivers/cpufreq/brcmstb-avs-cpufreq.c:
>> 449         table[i].frequency = CPUFREQ_TABLE_END;
>>
>> This fixes the following -Warray-bounds warning seen after building
>> ARM with multi_v7_defconfig (GCC 13):
>> In function 'brcm_avs_get_freq_table',
>>      inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
>> drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: warning: array subscript 5 is outside array bounds of 'void[60]' [-Warray-bounds=]
>>    449 |         table[i].frequency = CPUFREQ_TABLE_END;
> 
> #define AVS_PSTATE_P0           0x0
> #define AVS_PSTATE_P1           0x1
> #define AVS_PSTATE_P2           0x2
> #define AVS_PSTATE_P3           0x3
> #define AVS_PSTATE_P4           0x4
> #define AVS_PSTATE_MAX          AVS_PSTATE_P4
> 
>          table = devm_kcalloc(dev, AVS_PSTATE_MAX + 1, sizeof(*table),
>                               GFP_KERNEL);
> 	...
>          for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
> 		...
> 	}
>          table[i].frequency = CPUFREQ_TABLE_END;
> 
> I see "AVS_PSTATE_MAX + 1" being used for the allocation, and so the
> loop is likely correctly doing P0 through P4. If there is supposed to be
> a terminating element in the table, I think the correct fix would be to
> allocate an additional element, not stop the loop from processing P4.

Yeah; I think you're right. And it seems that this function header makes it
clear, too:

drivers/cpufreq/brcmstb-avs-cpufreq.c:
421 /*
422  * We determine which frequencies are supported by cycling through all P-states
423  * and reading back what frequency we are running at for each P-state.
424  */
425 static struct cpufreq_frequency_table *
426 brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)

I just sent v3:
https://lore.kernel.org/linux-hardening/ZMh45KH2iPIpNktr@work/

Let's see what the maintainers say.

Thanks for the feedback!
--
Gustavo

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug
@ 2023-08-01  3:20     ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-08-01  3:20 UTC (permalink / raw)
  To: Kees Cook, Gustavo A. R. Silva
  Cc: Markus Mayer, Broadcom internal kernel review list,
	Rafael J. Wysocki, Viresh Kumar, Florian Fainelli, linux-pm,
	linux-arm-kernel, linux-kernel, linux-hardening



On 7/31/23 17:16, Kees Cook wrote:
> On Mon, Jul 31, 2023 at 03:07:20PM -0600, Gustavo A. R. Silva wrote:
>> Update the iteration conditions in the for() loop to avoid writing in
>> array `table` beyond its allocated size at:
>>
>> drivers/cpufreq/brcmstb-avs-cpufreq.c:
>> 449         table[i].frequency = CPUFREQ_TABLE_END;
>>
>> This fixes the following -Warray-bounds warning seen after building
>> ARM with multi_v7_defconfig (GCC 13):
>> In function 'brcm_avs_get_freq_table',
>>      inlined from 'brcm_avs_cpufreq_init' at drivers/cpufreq/brcmstb-avs-cpufreq.c:623:15:
>> drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: warning: array subscript 5 is outside array bounds of 'void[60]' [-Warray-bounds=]
>>    449 |         table[i].frequency = CPUFREQ_TABLE_END;
> 
> #define AVS_PSTATE_P0           0x0
> #define AVS_PSTATE_P1           0x1
> #define AVS_PSTATE_P2           0x2
> #define AVS_PSTATE_P3           0x3
> #define AVS_PSTATE_P4           0x4
> #define AVS_PSTATE_MAX          AVS_PSTATE_P4
> 
>          table = devm_kcalloc(dev, AVS_PSTATE_MAX + 1, sizeof(*table),
>                               GFP_KERNEL);
> 	...
>          for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
> 		...
> 	}
>          table[i].frequency = CPUFREQ_TABLE_END;
> 
> I see "AVS_PSTATE_MAX + 1" being used for the allocation, and so the
> loop is likely correctly doing P0 through P4. If there is supposed to be
> a terminating element in the table, I think the correct fix would be to
> allocate an additional element, not stop the loop from processing P4.

Yeah; I think you're right. And it seems that this function header makes it
clear, too:

drivers/cpufreq/brcmstb-avs-cpufreq.c:
421 /*
422  * We determine which frequencies are supported by cycling through all P-states
423  * and reading back what frequency we are running at for each P-state.
424  */
425 static struct cpufreq_frequency_table *
426 brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)

I just sent v3:
https://lore.kernel.org/linux-hardening/ZMh45KH2iPIpNktr@work/

Let's see what the maintainers say.

Thanks for the feedback!
--
Gustavo

_______________________________________________
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-08-01  6:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-31 21:07 [PATCH v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug Gustavo A. R. Silva
2023-07-31 21:07 ` Gustavo A. R. Silva
2023-07-31 23:16 ` Kees Cook
2023-07-31 23:16   ` Kees Cook
2023-08-01  3:20   ` Gustavo A. R. Silva
2023-08-01  3: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.