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

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(-)

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

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/

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