Re: [PATCH] mt76: mt7915: fix a couple information leaks

[Felix Fietkau <nbd@nbd.name>]

On 2022-01-07 10:18, Johannes Berg wrote:
> On Fri, 2022-01-07 at 10:36 +0300, Dan Carpenter wrote:
>> Unfortunately this code has stumbled into some deep C standards
>> nonsense.  These two structs have a 3 byte struct hole at the end.  If
>> you partially initialize a struct then the C standard specifies that
>> all the struct holes are zeroed out.  But when you initialize all the
>> members of the struct, as this code does, then struct holes may be left
>> with uninitialized stack data.  This is from C11 section 6.7.9 and how
>> it is implemented in GCC.
> 
> Wow, nice find ...
> 
>> +	memset(&data, 0, sizeof(data));
>> +	data.cmd = cpu_to_le32(MURU_SET_TXC_TX_STATS_EN);
>> +	data.enable = enabled;
>> 
> 
> Maybe add a comment? This is not going to be obvious in the future.
> 
>>  	return mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD(MURU_CTRL),
>> &data,
>>  				sizeof(data), false);
> 
> Or maybe instead just mark the thing __packed (and/or explicitly add the
> padding if needed), it seems weird that we'd send something to the
> *firmware* that has a struct layout subject to compiler/arch padding
> rules.
I would also prefer explicitly adding the padding and leaving the rest 
of the code as-is.

- Felix