Message-ID: <f61c6a25c7ad1ed452b4facf38c7e451d47c5dc0.camel@sipsolutions.net>
Subject: Re: [PATCH] mt76: mt7915: fix a couple information leaks
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        Felix Fietkau <nbd@nbd.name>
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
Date:   Fri, 07 Jan 2022 10:18:25 +0100
In-Reply-To: <20220107073609.GH22086@kili>
References: <20220107073609.GH22086@kili>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.2 (3.42.2-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

johannes