Re: [PATCH] init/Kconfig: extend -Wno-array-bounds to gcc 13

Re: [PATCH] init/Kconfig: extend -Wno-array-bounds to gcc 13

[Kees Cook]

*thread necromancy*

On Mon, Mar 06, 2023 at 05:09:47PM -0500, Tom Rix wrote:
> With gcc 13.0.1 on x86, there are several false positives like
> 
> drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:167:31:
>   error: array subscript 4 is above array bounds of ‘const struct sparx5_psfp_gce[4]’ [-Werror=array-bounds=]
>   167 |                 gce = &sg->gce[i];
>       |                        ~~~~~~~^~~
> In file included from drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c:8:
> drivers/net/ethernet/microchip/sparx5/sparx5_main.h:506:32: note: while referencing ‘gce’
>   506 |         struct sparx5_psfp_gce gce[SPX5_PSFP_GCE_CNT];
>       |                                ^~~
> 
> The code lines for the reported problem
> 	/* For each scheduling entry */
> 	for (i = 0; i < sg->num_entries; i++) {
> 		gce = &sg->gce[i];
> 
> i is bounded by num_entries, which is set in sparx5_tc_flower.c
> 	if (act->gate.num_entries >= SPX5_PSFP_GCE_CNT) {
> 		NL_SET_ERR_MSG_MOD(extack, "Invalid number of gate entries");
> 		return -EINVAL;
> 	}
> ..
> 	sg->num_entries = act->gate.num_entries;
> 
> So disable array-bounds as was done on gcc 11 and 12

So, as it turns out, after much discussion with GCC devs (and Qing's great
patience with me), I finally see that this is not a false positive. GCC
correctly sees a test for "i >= SPX5_PSFP_GCE_CNT", hidden away in the
macros and inlines, and is reporting "Look, you have an execution path
where i is out of bounds."

(It is not a result of the shift sanitizer nor the array bounds sanitizer
-- I was convinced these were responsible, but I was wrong.)

Here are the details:


"gce" is this array of size 4:

struct sparx5_psfp_sg {
	...
        u32 num_entries;            /* PSFPAdminControlListLength */
        struct sparx5_psfp_gce gce[SPX5_PSFP_GCE_CNT];
};

#define SPX5_PSFP_GCE_CNT 4

with the loop:

        for (i = 0; i < sg->num_entries; i++) {
                gce = &sg->gce[i];
		...
                spx5_wr(ANA_AC_SG_GCL_GS_CONFIG_IPS_SET(ips) |
                        ANA_AC_SG_GCL_GS_CONFIG_GATE_STATE_SET(gce->gate_state),
                        sparx5, ANA_AC_SG_GCL_GS_CONFIG(i));

That last macro is:

#define ANA_AC_SG_GCL_GS_CONFIG(r) __REG(TARGET_ANA_AC,\
                                        0, 1, 851584, 0, 1, 128, 0, r, 4, 4)

Note the "r, 4", which is actually "i, 4". These are passed into
spx5_wr() as rinst and rcnt respectively:

static inline void spx5_wr(u32 val, struct sparx5 *sparx5,
                           int id, int tinst, int tcnt,
                           int gbase, int ginst, int gcnt, int gwidth,
                           int raddr, int rinst, int rcnt, int rwidth)
{
        writel(val, spx5_addr(sparx5->regs, id, tinst, tcnt,
                              gbase, ginst, gcnt, gwidth,
                              raddr, rinst, rcnt, rwidth));
}

and spx5_addr() does:

static inline void __iomem *spx5_addr(void __iomem *base[],
                                      int id, int tinst, int tcnt,
                                      int gbase, int ginst,
                                      int gcnt, int gwidth,
                                      int raddr, int rinst,
                                      int rcnt, int rwidth)
{
	...
        WARN_ON((rinst) >= rcnt);


It is specifically checking for "i >= 4", and GCC is correctly reporting
that this is therefore a reachable path.

So, the correct fix here is to do the check up front and make sure an
invalid array indexing cannot happen:

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c b/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c
index 8dee1ab1fa75..23c40b9b74d5 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c
@@ -163,7 +163,7 @@ static int sparx5_psfp_sg_set(struct sparx5 *sparx5, u32 id,
 	spx5_wr(sg->cycletimeext, sparx5, ANA_AC_SG_CONFIG_REG_5);
 
 	/* For each scheduling entry */
-	for (i = 0; i < sg->num_entries; i++) {
+	for (i = 0; i < ARRAY_SIZE(sg->gce) && i < sg->num_entries; i++) {
 		gce = &sg->gce[i];
 		ips = sparx5_psfp_ipv_to_ips(gce->ipv);
 		/* hardware needs TimeInterval to be cumulative */

Or, if you want to retain the WARN:


diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c b/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c
index 8dee1ab1fa75..5ffeb299fcb4 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_psfp.c
@@ -164,6 +164,8 @@ static int sparx5_psfp_sg_set(struct sparx5 *sparx5, u32 id,
 
 	/* For each scheduling entry */
 	for (i = 0; i < sg->num_entries; i++) {
+		if (WARN_ON_ONCE(i >= ARRAY_SIZE(sg->gce)))
+			break;
 		gce = &sg->gce[i];
 		ips = sparx5_psfp_ipv_to_ips(gce->ipv);
 		/* hardware needs TimeInterval to be cumulative */


-- 
Kees Cook