netdev - Re: [PATCH net-next v7 08/11] net: bcmasp: Add support for ethtool driver stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 15 Jun 2023 10:07:26 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Justin Chen <justin.chen@...adcom.com>
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	bcm-kernel-feedback-list@...adcom.com,
	florian.fainelli@...adcom.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	conor+dt@...nel.org, opendmb@...il.com, andrew@...n.ch,
	hkallweit1@...il.com, linux@...linux.org.uk,
	richardcochran@...il.com, sumit.semwal@...aro.org,
	christian.koenig@....com
Subject: Re: [PATCH net-next v7 08/11] net: bcmasp: Add support for ethtool
 driver stats

On Wed, Jun 14, 2023 at 03:30:17PM -0700, Justin Chen wrote:

...

> +static void bcmasp_update_mib_counters(struct bcmasp_intf *intf)
> +{
> +	int i;
> +
> +	for (i = 0; i < BCMASP_STATS_LEN; i++) {
> +		const struct bcmasp_stats *s;
> +		u32 offset, val;
> +		char *p;
> +
> +		s = &bcmasp_gstrings_stats[i];
> +		offset = bcmasp_stat_fixup_offset(intf, s);
> +		switch (s->type) {
> +		case BCMASP_STAT_SOFT:
> +			continue;
> +		case BCMASP_STAT_RX_EDPKT:
> +			val = rx_edpkt_core_rl(intf->parent, offset);
> +			break;
> +		case BCMASP_STAT_RX_CTRL:
> +			val = rx_ctrl_core_rl(intf->parent, offset);
> +			break;
> +		case BCMASP_STAT_RX_CTRL_PER_INTF:
> +			offset += sizeof(u32) * intf->port;
> +			val = rx_ctrl_core_rl(intf->parent, offset);
> +			break;
> +		}
> +		p = (char *)(&intf->mib) + (i * sizeof(u32));
> +		put_unaligned(val, (u32 *)p);

Hi Justin,

GCC 12.2.0, in a W=1 build, warns that val may be used uninitialised here.

I think that, in theory, that can occur if s->type doesn't match
any of the case statements above. Perhaps in practice that cannot occur.
But, perhaps it would be worth adding a default case with some suitable
handling.

 In file included from drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c:4:
 ./include/asm-generic/unaligned.h: In function 'bcmasp_get_ethtool_stats':
 ./include/asm-generic/unaligned.h:19:19: warning: 'val' may be used uninitialized [-Wmaybe-uninitialized]
    19 |         __pptr->x = (val);                                                      \
       |                   ^
 drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c:125:29: note: 'val' was declared here
   125 |                 u32 offset, val;
       |  

> +	}
> +}

...