netdev - Re: [PATCH] drivers: net: ethernet: intel: e1000: e1000

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <53F21BEC.2070204@intel.com>
Date:	Mon, 18 Aug 2014 08:29:48 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	Joe Perches <joe@...ches.com>,
	Krzysztof Majzerowicz-Jaszcz <cristos@...serv.org>
CC:	jeffrey.t.kirsher@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: net: ethernet: intel: e1000: e1000_ethtool.c
 coding style fixes

On 08/16/2014 11:41 AM, Joe Perches wrote:
> On Sat, 2014-08-16 at 11:12 +0200, Krzysztof Majzerowicz-Jaszcz wrote:
>> @@ -1835,11 +1830,11 @@ static void e1000_get_ethtool_stats(struct net_device *netdev,
>>  	for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {
>>  		switch (e1000_gstrings_stats[i].type) {
>>  		case NETDEV_STATS:
>> -			p = (char *) netdev +
>> +			p = (char *)netdev +
>>  					e1000_gstrings_stats[i].stat_offset;
>>  			break;
>>  		case E1000_STATS:
>> -			p = (char *) adapter +
>> +			p = (char *)adapter +
>>  					e1000_gstrings_stats[i].stat_offset;
>>  			brseak;
>>  		}
> 
> Maybe use a temporary for &e1000_gstring_stats[i]
> 
> Something like: (w/ void * for char *, WARN_ONCE, trigraph->if/else)
> 
> static void e1000_get_ethtool_stats(struct net_device *netdev,
> 				    struct ethtool_stats *stats, u64 *data)
> {
> 	struct e1000_adapter *adapter = netdev_priv(netdev);
> 	int i;
> 	void *p = NULL;
> 	const struct e1000_stats *stat = e1000_gstring_stats;
> 
> 	e1000_update_stats(adapter);
> 
> 	for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {
> 		switch (stat->type) {
> 		case NETDEV_STATS:
> 			p = (void *)netdev + stat->stat_offset;
> 			break;
> 		case E1000_STATS:
> 			p = (void *)adapter + stat->stat_offset;
> 			break;
> 		default:
> 			WARN_ONCE(1, "Invalid E1000 stat type: %u index %d\n",
> 				  stat->type, i);
> 			break;
> 		}
> 
> 		if (stat->sizeof_stat == sizeof(u64))
> 			data[i] = *(u64 *)p;
> 		else
> 			data[i] = *(u32 *)p;
> 
> 		stat++;
> 	}
> }
> 

Doing any kind of pointer math on a void pointer is generally unsafe as
it is an incomplete type.  The only reason why it works in GCC is
because GCC has a nonstandard extension that makes it report as having a
size of 1.

This is why the math is being done on a char * as it is a complete type
with a size of 1.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html