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 <[email protected]>
Cc: Stanislav Yakovlev <[email protected]>
---
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
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 <[email protected]>
Cc: Igor Mitsyanko <[email protected]>
Cc: Sergey Matyukevich <[email protected]>
---
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
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 <[email protected]>
Cc: Johannes Berg <[email protected]>
---
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
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
This shouldn't have "wifi: cfg80211:" prefix but rather "wifi:
qtnfmac:" :-)
johannes
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/[email protected]/
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
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...
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
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
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