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

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

[Dan Carpenter]

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.

Anyway, add some memsets to prevent exposing uninitialized stack data
with the user.  Debugfs is root only so the real life impact of these
leaks is very small.

Fixes: 1966a5078f2d ("mt76: mt7915: add mu-mimo and ofdma debugfs knobs")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 .../net/wireless/mediatek/mt76/mt7915/mcu.c   | 21 ++++++++++---------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index 0911b6f973b5..19c340c65465 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -2999,10 +2999,11 @@ int mt7915_mcu_muru_debug_set(struct mt7915_dev *dev, bool enabled)
 	struct {
 		__le32 cmd;
 		u8 enable;
-	} data = {
-		.cmd = cpu_to_le32(MURU_SET_TXC_TX_STATS_EN),
-		.enable = enabled,
-	};
+	} data;
+
+	memset(&data, 0, sizeof(data));
+	data.cmd = cpu_to_le32(MURU_SET_TXC_TX_STATS_EN);
+	data.enable = enabled;
 
 	return mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD(MURU_CTRL), &data,
 				sizeof(data), false);
@@ -3014,15 +3015,15 @@ int mt7915_mcu_muru_debug_get(struct mt7915_phy *phy, void *ms)
 	struct sk_buff *skb;
 	struct mt7915_mcu_muru_stats *mu_stats =
 				(struct mt7915_mcu_muru_stats *)ms;
-	int ret;
-
 	struct {
 		__le32 cmd;
 		u8 band_idx;
-	} req = {
-		.cmd = cpu_to_le32(MURU_GET_TXC_TX_STATS),
-		.band_idx = phy != &dev->phy,
-	};
+	} req;
+	int ret;
+
+	memset(&req, 0, sizeof(req));
+	req.cmd = cpu_to_le32(MURU_GET_TXC_TX_STATS);
+	req.band_idx = phy != &dev->phy;
 
 	ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD(MURU_CTRL),
 					&req, sizeof(req), true, &skb);
-- 
2.20.1

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

[Dan Carpenter]

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.

Anyway, add some memsets to prevent exposing uninitialized stack data
with the user.  Debugfs is root only so the real life impact of these
leaks is very small.

Fixes: 1966a5078f2d ("mt76: mt7915: add mu-mimo and ofdma debugfs knobs")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 .../net/wireless/mediatek/mt76/mt7915/mcu.c   | 21 ++++++++++---------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index 0911b6f973b5..19c340c65465 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -2999,10 +2999,11 @@ int mt7915_mcu_muru_debug_set(struct mt7915_dev *dev, bool enabled)
 	struct {
 		__le32 cmd;
 		u8 enable;
-	} data = {
-		.cmd = cpu_to_le32(MURU_SET_TXC_TX_STATS_EN),
-		.enable = enabled,
-	};
+	} data;
+
+	memset(&data, 0, sizeof(data));
+	data.cmd = cpu_to_le32(MURU_SET_TXC_TX_STATS_EN);
+	data.enable = enabled;
 
 	return mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD(MURU_CTRL), &data,
 				sizeof(data), false);
@@ -3014,15 +3015,15 @@ int mt7915_mcu_muru_debug_get(struct mt7915_phy *phy, void *ms)
 	struct sk_buff *skb;
 	struct mt7915_mcu_muru_stats *mu_stats =
 				(struct mt7915_mcu_muru_stats *)ms;
-	int ret;
-
 	struct {
 		__le32 cmd;
 		u8 band_idx;
-	} req = {
-		.cmd = cpu_to_le32(MURU_GET_TXC_TX_STATS),
-		.band_idx = phy != &dev->phy,
-	};
+	} req;
+	int ret;
+
+	memset(&req, 0, sizeof(req));
+	req.cmd = cpu_to_le32(MURU_GET_TXC_TX_STATS);
+	req.band_idx = phy != &dev->phy;
 
 	ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD(MURU_CTRL),
 					&req, sizeof(req), true, &skb);
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Johannes Berg]

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

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

[Johannes Berg]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Felix Fietkau]

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

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

[Felix Fietkau]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Johannes Berg]

On Fri, 2022-01-07 at 11:08 +0100, Felix Fietkau wrote:
> > 
> > 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.
> 

Arguably, if you add padding explicitly, you might want to also mark it
__packed or add some BUILD_BUG_ON() ensuring there's no more padding
added by the compiler because of weird architectures, or whatnot?

johannes

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

[Johannes Berg]

On Fri, 2022-01-07 at 11:08 +0100, Felix Fietkau wrote:
> > 
> > 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.
> 

Arguably, if you add padding explicitly, you might want to also mark it
__packed or add some BUILD_BUG_ON() ensuring there's no more padding
added by the compiler because of weird architectures, or whatnot?

johannes

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek