[PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Jacob Keller]

The ipw_fw_error structure contains a payload[] flexible array as well as
two pointers to this array area, ->elem, and ->log. The total size of the
allocated structure is computed without use of the <linux/overflow.h>
macros.

There's no reason to keep both a payload[] and an extra pointer to both the
elem and log members. Convert the elem pointer member into the flexible
array member, removing payload.

Fix the allocation of the ipw_fw_error structure to use size_add(),
struct_size(), and array_size() to compute the allocation. This ensures
that any overflow saturates at SIZE_MAX rather than overflowing and
potentially allowing an undersized allocation.

Before the structure change, the layout of ipw_fw_error was:

struct ipw_fw_error {
        long unsigned int          jiffies;              /*     0     8 */
        u32                        status;               /*     8     4 */
        u32                        config;               /*    12     4 */
        u32                        elem_len;             /*    16     4 */
        u32                        log_len;              /*    20     4 */
        struct ipw_error_elem *    elem;                 /*    24     8 */
        struct ipw_event *         log;                  /*    32     8 */
        u8                         payload[];            /*    40     0 */

        /* size: 40, cachelines: 1, members: 8 */
        /* last cacheline: 40 bytes */
};

After this change, the layout is now:

struct ipw_fw_error {
        long unsigned int          jiffies;              /*     0     8 */
        u32                        status;               /*     8     4 */
        u32                        config;               /*    12     4 */
        u32                        elem_len;             /*    16     4 */
        u32                        log_len;              /*    20     4 */
        struct ipw_event *         log;                  /*    24     8 */
        struct ipw_error_elem      elem[];               /*    32     0 */

        /* size: 32, cachelines: 1, members: 7 */
        /* last cacheline: 32 bytes */
};

This saves a total of 8 bytes for every ipw_fw_error allocation, and
removes the risk of a potential overflow on the allocation.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Stanislav Yakovlev <stas.yakovlev@gmail.com>
---
 drivers/net/wireless/intel/ipw2x00/ipw2200.c | 7 +++----
 drivers/net/wireless/intel/ipw2x00/ipw2200.h | 3 +--
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200.c b/drivers/net/wireless/intel/ipw2x00/ipw2200.c
index d382f2017325..b91b1a2d0be7 100644
--- a/drivers/net/wireless/intel/ipw2x00/ipw2200.c
+++ b/drivers/net/wireless/intel/ipw2x00/ipw2200.c
@@ -1234,9 +1234,9 @@ static struct ipw_fw_error *ipw_alloc_error_log(struct ipw_priv *priv)
 	u32 base = ipw_read32(priv, IPW_ERROR_LOG);
 	u32 elem_len = ipw_read_reg32(priv, base);
 
-	error = kmalloc(sizeof(*error) +
-			sizeof(*error->elem) * elem_len +
-			sizeof(*error->log) * log_len, GFP_ATOMIC);
+	error = kmalloc(size_add(struct_size(error, elem, elem_len),
+				 array_size(sizeof(*error->log), log_len)),
+			GFP_ATOMIC);
 	if (!error) {
 		IPW_ERROR("Memory allocation for firmware error log "
 			  "failed.\n");
@@ -1247,7 +1247,6 @@ static struct ipw_fw_error *ipw_alloc_error_log(struct ipw_priv *priv)
 	error->config = priv->config;
 	error->elem_len = elem_len;
 	error->log_len = log_len;
-	error->elem = (struct ipw_error_elem *)error->payload;
 	error->log = (struct ipw_event *)(error->elem + elem_len);
 
 	ipw_capture_event_log(priv, log_len, error->log);
diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200.h b/drivers/net/wireless/intel/ipw2x00/ipw2200.h
index 09ddd21608d4..8ebf09121e17 100644
--- a/drivers/net/wireless/intel/ipw2x00/ipw2200.h
+++ b/drivers/net/wireless/intel/ipw2x00/ipw2200.h
@@ -1106,9 +1106,8 @@ struct ipw_fw_error {	 /* XXX */
 	u32 config;
 	u32 elem_len;
 	u32 log_len;
-	struct ipw_error_elem *elem;
 	struct ipw_event *log;
-	u8 payload[];
+	struct ipw_error_elem elem[];
 } __packed;
 
 #ifdef CONFIG_IPW2200_PROMISCUOUS
-- 
2.39.1.405.gd4c25cc71f83

[PATCH 2/3] wifi: cfg80211: use struct_size and size_sub for payload length

[Jacob Keller]

Replace the calculations for the payload length in
qtnf_cmd_band_fill_iftype with struct_size() and size_sub(). While
the payload length does not get directly passed to an allocation function,
the performed calculation is still calculating the size of a flexible array
structure (minus the size of a header structure).

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Igor Mitsyanko <imitsyanko@quantenna.com>
Cc: Sergey Matyukevich <geomatsi@gmail.com>
---
 drivers/net/wireless/quantenna/qtnfmac/commands.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/quantenna/qtnfmac/commands.c b/drivers/net/wireless/quantenna/qtnfmac/commands.c
index b1b73478d89b..68ae9c7ea95a 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/commands.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/commands.c
@@ -1325,9 +1325,10 @@ static int qtnf_cmd_band_fill_iftype(const u8 *data,
 	struct ieee80211_sband_iftype_data *iftype_data;
 	const struct qlink_tlv_iftype_data *tlv =
 		(const struct qlink_tlv_iftype_data *)data;
-	size_t payload_len = tlv->n_iftype_data * sizeof(*tlv->iftype_data) +
-		sizeof(*tlv) -
-		sizeof(struct qlink_tlv_hdr);
+	size_t payload_len;
+
+	payload_len = struct_size(tlv, iftype_data, tlv->n_iftype_data);
+	payload_len = size_sub(payload_len, sizeof(struct qlink_tlv_hdr));
 
 	if (tlv->hdr.len != cpu_to_le16(payload_len)) {
 		pr_err("bad IFTYPE_DATA TLV len %u\n", tlv->hdr.len);
-- 
2.39.1.405.gd4c25cc71f83

Re: [PATCH 2/3] wifi: cfg80211: use struct_size and size_sub for payload length

[Johannes Berg]

This shouldn't have "wifi: cfg80211:" prefix but rather "wifi:
qtnfmac:"  :-)

johannes

[PATCH 3/3] wifi: nl80211: convert cfg80211_scan_request allocation to *_size macros

[Jacob Keller]

The cfg80211_scan_request structure is followed by a flexible array member
as well as several other arrays that are then stored into pointers in the
structure. These are allocated currently using a simple sequence of
multiplications.

Replace the calculations with struct_size(), size_add(), and array_size()
helper macros. These macros saturate the calculation at SIZE_MAX rather
than overflowing.

Note that we can't use flex_array_size() instead of array_size() because
the fields are not arrays, but simple pointers.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
---
 net/wireless/nl80211.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 112b4bb009c8..e5b08546bf30 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -9019,7 +9019,7 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr *attr;
 	struct wiphy *wiphy;
 	int err, tmp, n_ssids = 0, n_channels, i;
-	size_t ie_len;
+	size_t ie_len, size;
 
 	wiphy = &rdev->wiphy;
 
@@ -9064,10 +9064,10 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 	if (ie_len > wiphy->max_scan_ie_len)
 		return -EINVAL;
 
-	request = kzalloc(sizeof(*request)
-			+ sizeof(*request->ssids) * n_ssids
-			+ sizeof(*request->channels) * n_channels
-			+ ie_len, GFP_KERNEL);
+	size = struct_size(request, channels, n_channels);
+	size = size_add(size, array_size(sizeof(*request->ssids), n_ssids));
+	size = size_add(size, ie_len);
+	request = kzalloc(size, GFP_KERNEL);
 	if (!request)
 		return -ENOMEM;
 
@@ -9400,7 +9400,7 @@ nl80211_parse_sched_scan(struct wiphy *wiphy, struct wireless_dev *wdev,
 	struct nlattr *attr;
 	int err, tmp, n_ssids = 0, n_match_sets = 0, n_channels, i, n_plans = 0;
 	enum nl80211_band band;
-	size_t ie_len;
+	size_t ie_len, size;
 	struct nlattr *tb[NL80211_SCHED_SCAN_MATCH_ATTR_MAX + 1];
 	s32 default_match_rssi = NL80211_SCAN_RSSI_THOLD_OFF;
 
@@ -9509,12 +9509,14 @@ nl80211_parse_sched_scan(struct wiphy *wiphy, struct wireless_dev *wdev,
 	     attrs[NL80211_ATTR_SCHED_SCAN_RSSI_ADJUST]))
 		return ERR_PTR(-EINVAL);
 
-	request = kzalloc(sizeof(*request)
-			+ sizeof(*request->ssids) * n_ssids
-			+ sizeof(*request->match_sets) * n_match_sets
-			+ sizeof(*request->scan_plans) * n_plans
-			+ sizeof(*request->channels) * n_channels
-			+ ie_len, GFP_KERNEL);
+	size = struct_size(request, channels, n_channels);
+	size = size_add(size, array_size(sizeof(*request->ssids), n_ssids));
+	size = size_add(size, array_size(sizeof(*request->match_sets),
+					  n_match_sets));
+	size = size_add(size, array_size(sizeof(*request->scan_plans),
+					 n_plans));
+	size = size_add(size, ie_len);
+	request = kzalloc(size, GFP_KERNEL);
 	if (!request)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.39.1.405.gd4c25cc71f83

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Johannes Berg]

On Tue, 2023-02-28 at 08:28 -0800, Jacob Keller wrote:
> 
> @@ -1247,7 +1247,6 @@ static struct ipw_fw_error *ipw_alloc_error_log(struct ipw_priv *priv)
>  	error->config = priv->config;
>  	error->elem_len = elem_len;
>  	error->log_len = log_len;
> -	error->elem = (struct ipw_error_elem *)error->payload;
>  	error->log = (struct ipw_event *)(error->elem + elem_len);

I really don't know this driver, it's ancient, but that last line looks
wrong to me already, elem_len doesn't seem like # of elems?

But I guess this patch changes nothing here, so hey. Don't think there's
much value in the change either, this driver isn't going to get touched
any more, just removed eventually ;)

johannes

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Jacob Keller]

On 2/28/2023 9:16 AM, Johannes Berg wrote:
> On Tue, 2023-02-28 at 08:28 -0800, Jacob Keller wrote:
>>
>> @@ -1247,7 +1247,6 @@ static struct ipw_fw_error *ipw_alloc_error_log(struct ipw_priv *priv)
>>  	error->config = priv->config;
>>  	error->elem_len = elem_len;
>>  	error->log_len = log_len;
>> -	error->elem = (struct ipw_error_elem *)error->payload;
>>  	error->log = (struct ipw_event *)(error->elem + elem_len);
> 
> I really don't know this driver, it's ancient, but that last line looks
> wrong to me already, elem_len doesn't seem like # of elems?
> 
> But I guess this patch changes nothing here, so hey. Don't think there's
> much value in the change either, this driver isn't going to get touched
> any more, just removed eventually ;)
> 
> johannes
> 

Previous to this change, error struct has two pointers to sections of
memory allocated at the end of the buffer.

The code used to be:

-	error = kmalloc(sizeof(*error) +
-			sizeof(*error->elem) * elem_len +
-			sizeof(*error->log) * log_len, GFP_ATOMIC);

i.e. the elem_len is multiplying sizeof(*error->elem).

The code is essentially trying to get two flexible arrays in the same
allocation, and its a bit messy to do that. I don't see how elem_len
could be anything other than "number of elems" given this code I removed.

I posted these mainly because I was trying to resolve all of the hits
that were found by the coccinelle patch I made, posted at [1]. I wanted
to get it to run clean so that we had no more struct_size hits.

Dropping this would just make that patch have some hits until the driver
is removed, eventually...

Not really a big deal to me, I just didn't want to post a coccinelle
patch without also trying to fix the handful of problems it reported,
since the total number of reports was small.

Thanks,
Jake

[1]:
https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Johannes Berg]

On Tue, 2023-02-28 at 09:44 -0800, Jacob Keller wrote:
> 
> Previous to this change, error struct has two pointers to sections of
> memory allocated at the end of the buffer.
> 
> The code used to be:
> 
> -	error = kmalloc(sizeof(*error) +
> -			sizeof(*error->elem) * elem_len +
> -			sizeof(*error->log) * log_len, GFP_ATOMIC);
> 
> i.e. the elem_len is multiplying sizeof(*error->elem).
> 
> The code is essentially trying to get two flexible arrays in the same
> allocation, and its a bit messy to do that. I don't see how elem_len
> could be anything other than "number of elems" given this code I removed.

Yeah, you're right. I was thinking of more modern HW/FW too much I
guess, I see now even in the driver we have an array walk here (and it
trusts the elem_len from firmware... ahrg!)

> I posted these mainly because I was trying to resolve all of the hits
> that were found by the coccinelle patch I made, posted at [1]. I wanted
> to get it to run clean so that we had no more struct_size hits.
> 
> Dropping this would just make that patch have some hits until the driver
> is removed, eventually...
> 
> Not really a big deal to me, I just didn't want to post a coccinelle
> patch without also trying to fix the handful of problems it reported,
> since the total number of reports was small.
> 

Makes sense. I don't think we'll drop the driver at any point soon, but
I also don't see it being changed much :)

johannes

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Jacob Keller]

On 2/28/2023 9:46 AM, Johannes Berg wrote:
> On Tue, 2023-02-28 at 09:44 -0800, Jacob Keller wrote:
>>
>> Previous to this change, error struct has two pointers to sections of
>> memory allocated at the end of the buffer.
>>
>> The code used to be:
>>
>> -	error = kmalloc(sizeof(*error) +
>> -			sizeof(*error->elem) * elem_len +
>> -			sizeof(*error->log) * log_len, GFP_ATOMIC);
>>
>> i.e. the elem_len is multiplying sizeof(*error->elem).
>>
>> The code is essentially trying to get two flexible arrays in the same
>> allocation, and its a bit messy to do that. I don't see how elem_len
>> could be anything other than "number of elems" given this code I removed.
> 
> Yeah, you're right. I was thinking of more modern HW/FW too much I
> guess, I see now even in the driver we have an array walk here (and it
> trusts the elem_len from firmware... ahrg!)
> 

Ouch.. that makes me feel better about using struct_size/size_add here
since it would help protect against an overflow with a large element size...

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Jacob Keller]

On 2/28/2023 9:46 AM, Johannes Berg wrote:
> On Tue, 2023-02-28 at 09:44 -0800, Jacob Keller wrote:
>>
>> Previous to this change, error struct has two pointers to sections of
>> memory allocated at the end of the buffer.
>>
>> The code used to be:
>>
>> -	error = kmalloc(sizeof(*error) +
>> -			sizeof(*error->elem) * elem_len +
>> -			sizeof(*error->log) * log_len, GFP_ATOMIC);
>>
>> i.e. the elem_len is multiplying sizeof(*error->elem).
>>
>> The code is essentially trying to get two flexible arrays in the same
>> allocation, and its a bit messy to do that. I don't see how elem_len
>> could be anything other than "number of elems" given this code I removed.
> 
> Yeah, you're right. I was thinking of more modern HW/FW too much I
> guess, I see now even in the driver we have an array walk here (and it
> trusts the elem_len from firmware... ahrg!)
> 
>> I posted these mainly because I was trying to resolve all of the hits
>> that were found by the coccinelle patch I made, posted at [1]. I wanted
>> to get it to run clean so that we had no more struct_size hits.
>>
>> Dropping this would just make that patch have some hits until the driver
>> is removed, eventually...
>>
>> Not really a big deal to me, I just didn't want to post a coccinelle
>> patch without also trying to fix the handful of problems it reported,
>> since the total number of reports was small.
>>
> 
> Makes sense. I don't think we'll drop the driver at any point soon, but
> I also don't see it being changed much :)
> 
> johannes

I can drop this one out of the series if you don't have an intention of
taking it, or I can refactor to just use size_add and array_size without
converting it to flexible array, which would prevent that coccinelle
patch from complaining and at least ensure that we can't overflow size
and under-allocate.

Do you have a preference?

Thanks,
Jake

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Johannes Berg]

On Wed, 2023-03-01 at 12:49 -0800, Jacob Keller wrote:
> 
> I can drop this one out of the series if you don't have an intention of
> taking it, or I can refactor to just use size_add and array_size without
> converting it to flexible array, which would prevent that coccinelle
> patch from complaining and at least ensure that we can't overflow size
> and under-allocate.
> 
> Do you have a preference?
> 

Ah, it's up to Kalle, not me :-)

I think it's OK to do, and if it gets rid of drive-by submissions from
the coccinelle patch later, I guess it's better to take it now. And you
already have it anyway.

I might prefer though if you sent the drivers and cfg80211 patches
separately, since that's usually Kalle vs. me applying it, and if it's
in a same series we tend to end up wondering if there's a dependency or
something, which is clearly not the case here.

johannes

Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

[Jacob Keller]

On 3/1/2023 12:53 PM, Johannes Berg wrote:
> On Wed, 2023-03-01 at 12:49 -0800, Jacob Keller wrote:
>>
>> I can drop this one out of the series if you don't have an intention of
>> taking it, or I can refactor to just use size_add and array_size without
>> converting it to flexible array, which would prevent that coccinelle
>> patch from complaining and at least ensure that we can't overflow size
>> and under-allocate.
>>
>> Do you have a preference?
>>
> 
> Ah, it's up to Kalle, not me :-)
> 
> I think it's OK to do, and if it gets rid of drive-by submissions from
> the coccinelle patch later, I guess it's better to take it now. And you
> already have it anyway.
> 
> I might prefer though if you sent the drivers and cfg80211 patches
> separately, since that's usually Kalle vs. me applying it, and if it's
> in a same series we tend to end up wondering if there's a dependency or
> something, which is clearly not the case here.
> 
> johannes

Sure. I'll send them separately in v2.

Thanks,
Jake