Message-ID: <97dbf02a-e3c7-aa4a-c404-45fc6189dc10@nbd.name>
Date:   Fri, 7 Jan 2022 11:08:28 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.4.1
Subject: Re: [PATCH] mt76: mt7915: fix a couple information leaks
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        Dan Carpenter <dan.carpenter@oracle.com>
Cc:     Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Shayne Chen <shayne.chen@mediatek.com>,
        Sean Wang <sean.wang@mediatek.com>,
        Kalle Valo <kvalo@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        MeiChia Chiu <meichia.chiu@mediatek.com>,
        Money Wang <Money.Wang@mediatek.com>,
        linux-wireless@vger.kernel.org, linux-mediatek@lists.infradead.org,
        kernel-janitors@vger.kernel.org
References: <20220107073609.GH22086@kili>
 <f61c6a25c7ad1ed452b4facf38c7e451d47c5dc0.camel@sipsolutions.net>
From:   Felix Fietkau <nbd@nbd.name>
In-Reply-To: <f61c6a25c7ad1ed452b4facf38c7e451d47c5dc0.camel@sipsolutions.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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