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]>
---
This was discovered by a coccinelle patch I developed, and submitted at:
https://lore.kernel.org/all/[email protected]/
V1: https://lore.kernel.org/linux-wireless/[email protected]/
Changes since v1
* Split series into individual postings to avoid confusion about dependency
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
base-commit: 8f9850dd8d23c1290cb642ce9548a440da5771ec
--
2.39.1.405.gd4c25cc71f83
Jacob Keller <[email protected]> wrote:
> 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]>
Patch applied to wireless-next.git, thanks.
a23c82e006db wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches