| Message ID | 20230228162827.3876606-1-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | net: wireless: use struct_size where appropriate | expand |
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 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
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
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(-)