| Message ID | ZMgiiEmBORZRaobc@work (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] cpufreq: brcmstb-avs-cpufreq: Fix -Warray-bounds bug | expand |
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
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
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);
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(-)