diff mbox series

[v2] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]

Message ID 20230307230148.3735684-1-jacob.e.keller@intel.com
State New
Headers show
Series [v2] wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[] | expand

Commit Message

Jacob Keller March 7, 2023, 11:01 p.m. UTC
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>
---
This was discovered by a coccinelle patch I developed, and submitted at:
  https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/

V1: https://lore.kernel.org/linux-wireless/99a9d4a2-d032-1c9d-90c6-3a68f6b3a092@intel.com/
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(-)


base-commit: 8f9850dd8d23c1290cb642ce9548a440da5771ec

Comments

Kalle Valo March 13, 2023, 1:44 p.m. UTC | #1
Jacob Keller <jacob.e.keller@intel.com> 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 <jacob.e.keller@intel.com>
> Cc: Stanislav Yakovlev <stas.yakovlev@gmail.com>

Patch applied to wireless-next.git, thanks.

a23c82e006db wifi: ipw2x00: convert ipw_fw_error->elem to flexible array[]
diff mbox series

Patch

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