question about potential use of uninitialized variable

Hello everybody,

While looking into Coverity ID 1398130 I ran into the following piece of code at drivers/net/dsa/mv88e6xxx/chip.c:849:

849static uint64_t _mv88e6xxx_get_ethtool_stat(struct mv88e6xxx_chip *chip,

 850                                            struct mv88e6xxx_hw_stat *s,
 851                                            int port, u16 bank1_select,
 852                                            u16 histogram)
 853{
 854        u32 low;
 855        u32 high = 0;
 856        u16 reg = 0;
 857        int err;
 858        u64 value;

859
860 switch (s->type) {
861 case STATS_TYPE_PORT:
862 err = mv88e6xxx_port_read(chip, port, s->reg, &reg);
863 if (err)
864 return UINT64_MAX;
865
866 low = reg;
867 if (s->sizeof_stat == 4) {
868 err = mv88e6xxx_port_read(chip, port,
s->reg + 1, &reg);
869 if (err)
870 return UINT64_MAX;
871 high = reg;
872 }
873 break;
874 case STATS_TYPE_BANK1:
875 reg = bank1_select;
876 /* fall through */
877 case STATS_TYPE_BANK0:

 878                reg |= s->reg | histogram;
 879                mv88e6xxx_g1_stats_read(chip, reg, &low);
 880                if (s->sizeof_stat == 8)
 881                        mv88e6xxx_g1_stats_read(chip, reg + 1, &high);
 882        }
 883        value = (((u64)high) << 16) | low;
 884        return value;

885}

My question here is if there is any chance for the execution path to directly jump from line 860 to line 883, hence ending up using the uninitialized variable _low_?

Quoted 43 lines

Hello everybody,

While looking into Coverity ID 1398130 I ran into the following piece of code at drivers/net/dsa/mv88e6xxx/chip.c:849:

849static uint64_t _mv88e6xxx_get_ethtool_stat(struct mv88e6xxx_chip *chip,

 850                                            struct mv88e6xxx_hw_stat *s,
 851                                            int port, u16 bank1_select,
 852                                            u16 histogram)
 853{
 854        u32 low;
 855        u32 high = 0;
 856        u16 reg = 0;
 857        int err;
 858        u64 value;

859
860 switch (s->type) {
861 case STATS_TYPE_PORT:
862 err = mv88e6xxx_port_read(chip, port, s->reg, &reg);
863 if (err)
864 return UINT64_MAX;
865
866 low = reg;
867 if (s->sizeof_stat == 4) {
868 err = mv88e6xxx_port_read(chip, port,
s->reg + 1, &reg);
869 if (err)
870 return UINT64_MAX;
871 high = reg;
872 }
873 break;
874 case STATS_TYPE_BANK1:
875 reg = bank1_select;
876 /* fall through */
877 case STATS_TYPE_BANK0:

 878                reg |= s->reg | histogram;
 879                mv88e6xxx_g1_stats_read(chip, reg, &low);
 880                if (s->sizeof_stat == 8)
 881                        mv88e6xxx_g1_stats_read(chip, reg + 1, &high);
 882        }
 883        value = (((u64)high) << 16) | low;
 884        return value;

Quoted 5 lines

885}

My question here is if there is any chance for the execution path to directly jump from line 860 to line 883, hence ending up using the uninitialized variable _low_?

Hi Gustavo

It would require that s->type not have one of the listed case values. Currently all members of mv88e6xxx_hw_stats due use expected values. However, it would not hurt to add a

	 default:
		return UINT64_MAX;

Do you want to submit a patch?

Add default case to switch in order to avoid any chance of using an uninitialized variable _low_, in case s->type does not match any of the listed case values.

Addresses-Coverity-ID: 1398130

Suggested-by Andrew Lunn <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>