The mux_chip structure size is over allocated to additionally include both
the array of mux controllers as well as a device specific private area.
The controllers array is then pointed to by assigning mux_chip->mux to the
first block of extra memory, while the private area is extracted via
mux_chip_priv() and points to the area just after the controllers.
The size of the mux_chip allocation uses direct multiplication and addition
rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
struct member wastes space by having to store the pointer as part of the
structures.
Convert struct mux_chip to use a flexible array member for the mux
controller array. Use struct_size() and size_add() to compute the size of
the structure while protecting against overflow.
After converting the mux pointer, notice that two 4-byte holes remain in
the structure layout due to the alignment requirements for the dev
sub-structure and the ops pointer.
These can be easily fixed through re-ordering the id field to the 4-byte
hole just after the controllers member.
This changes the layout from:
struct mux_chip {
unsigned int controllers; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
struct mux_control * mux; /* 8 8 */
struct device dev __attribute__((__aligned__(8))); /* 16 1400 */
/* XXX last struct has 3 bytes of padding */
/* --- cacheline 22 boundary (1408 bytes) was 8 bytes ago --- */
int id; /* 1416 4 */
/* XXX 4 bytes hole, try to pack */
const struct mux_control_ops * ops; /* 1424 8 */
/* size: 1432, cachelines: 23, members: 5 */
/* sum members: 1424, holes: 2, sum holes: 8 */
/* paddings: 1, sum paddings: 3 */
/* forced alignments: 1 */
/* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));
To the following:
struct mux_chip {
unsigned int controllers; /* 0 4 */
int id; /* 4 4 */
struct device dev __attribute__((__aligned__(8))); /* 8 1400 */
/* XXX last struct has 3 bytes of padding */
/* --- cacheline 22 boundary (1408 bytes) --- */
const struct mux_control_ops * ops; /* 1408 8 */
struct mux_control mux[]; /* 1416 0 */
/* size: 1416, cachelines: 23, members: 5 */
/* paddings: 1, sum paddings: 3 */
/* forced alignments: 1 */
/* last cacheline: 8 bytes */
} __attribute__((__aligned__(8)));
This both removes risk of overflowing and performing an under-allocation,
as well as saves 16 bytes of otherwise wasted space for every mux_chip.
Signed-off-by: Jacob Keller <[email protected]>
---
drivers/mux/core.c | 7 +++----
include/linux/mux/driver.h | 10 +++++-----
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 49bedbe6316c..3e26b9911cc2 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -100,13 +100,12 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
if (WARN_ON(!dev || !controllers))
return ERR_PTR(-EINVAL);
- mux_chip = kzalloc(sizeof(*mux_chip) +
- controllers * sizeof(*mux_chip->mux) +
- sizeof_priv, GFP_KERNEL);
+ mux_chip = kzalloc(size_add(struct_size(mux_chip, mux, controllers),
+ sizeof_priv),
+ GFP_KERNEL);
if (!mux_chip)
return ERR_PTR(-ENOMEM);
- mux_chip->mux = (struct mux_control *)(mux_chip + 1);
mux_chip->dev.class = &mux_class;
mux_chip->dev.type = &mux_type;
mux_chip->dev.parent = dev;
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..84dc0d3e79d6 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -56,18 +56,18 @@ struct mux_control {
/**
* struct mux_chip - Represents a chip holding mux controllers.
* @controllers: Number of mux controllers handled by the chip.
- * @mux: Array of mux controllers that are handled.
- * @dev: Device structure.
* @id: Used to identify the device internally.
+ * @dev: Device structure.
* @ops: Mux controller operations.
+ * @mux: Flexible array of mux controllers that are handled.
*/
struct mux_chip {
unsigned int controllers;
- struct mux_control *mux;
- struct device dev;
int id;
-
+ struct device dev;
const struct mux_control_ops *ops;
+
+ struct mux_control mux[];
};
#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
base-commit: 307e14c039063f0c9bd7a18a7add8f940580dcc9
--
I found this while developing a coccinelle patch that helps detect potential
code that could be converted to struct_size() and noticed this weird case
that could be a flexible array and save memory.
2.39.1.405.gd4c25cc71f83
On 2/22/2023 5:42 PM, Jacob Keller wrote:
> The mux_chip structure size is over allocated to additionally include both
> the array of mux controllers as well as a device specific private area.
> The controllers array is then pointed to by assigning mux_chip->mux to the
> first block of extra memory, while the private area is extracted via
> mux_chip_priv() and points to the area just after the controllers.
>
> The size of the mux_chip allocation uses direct multiplication and addition
> rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
> struct member wastes space by having to store the pointer as part of the
> structures.
>
> Convert struct mux_chip to use a flexible array member for the mux
> controller array. Use struct_size() and size_add() to compute the size of
> the structure while protecting against overflow.
>
> After converting the mux pointer, notice that two 4-byte holes remain in
> the structure layout due to the alignment requirements for the dev
> sub-structure and the ops pointer.
>
> These can be easily fixed through re-ordering the id field to the 4-byte
> hole just after the controllers member.
Looks good to me (just a driver dev, not a mux dev!). Also added
linux-i2c mailing list and a couple others for more review.
Reviewed-by: Jesse Brandeburg <[email protected]>
related thread (cocci script) at [1]
[1]
https://lore.kernel.org/all/[email protected]/
On Mon, Feb 27, 2023 at 12:28:43PM -0800, Jesse Brandeburg wrote:
> On 2/22/2023 5:42 PM, Jacob Keller wrote:
> > The mux_chip structure size is over allocated to additionally include both
> > the array of mux controllers as well as a device specific private area.
> > The controllers array is then pointed to by assigning mux_chip->mux to the
> > first block of extra memory, while the private area is extracted via
> > mux_chip_priv() and points to the area just after the controllers.
> >
> > The size of the mux_chip allocation uses direct multiplication and addition
> > rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
> > struct member wastes space by having to store the pointer as part of the
> > structures.
> >
> > Convert struct mux_chip to use a flexible array member for the mux
> > controller array. Use struct_size() and size_add() to compute the size of
> > the structure while protecting against overflow.
> >
> > After converting the mux pointer, notice that two 4-byte holes remain in
> > the structure layout due to the alignment requirements for the dev
> > sub-structure and the ops pointer.
> >
> > These can be easily fixed through re-ordering the id field to the 4-byte
> > hole just after the controllers member.
>
> Looks good to me (just a driver dev, not a mux dev!). Also added
> linux-i2c mailing list and a couple others for more review.
>
> Reviewed-by: Jesse Brandeburg <[email protected]>
>
> related thread (cocci script) at [1]
>
> [1]
> https://lore.kernel.org/all/[email protected]/
*thread necromancy*
Can we land this? It's the last struct_size() instance that the above
Coccinelle script flags.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On 2/18/2024 9:04 PM, Kees Cook wrote:
> On Mon, Feb 27, 2023 at 12:28:43PM -0800, Jesse Brandeburg wrote:
>> On 2/22/2023 5:42 PM, Jacob Keller wrote:
>>> The mux_chip structure size is over allocated to additionally include both
>>> the array of mux controllers as well as a device specific private area.
>>> The controllers array is then pointed to by assigning mux_chip->mux to the
>>> first block of extra memory, while the private area is extracted via
>>> mux_chip_priv() and points to the area just after the controllers.
>>>
>>> The size of the mux_chip allocation uses direct multiplication and addition
>>> rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
>>> struct member wastes space by having to store the pointer as part of the
>>> structures.
>>>
>>> Convert struct mux_chip to use a flexible array member for the mux
>>> controller array. Use struct_size() and size_add() to compute the size of
>>> the structure while protecting against overflow.
>>>
>>> After converting the mux pointer, notice that two 4-byte holes remain in
>>> the structure layout due to the alignment requirements for the dev
>>> sub-structure and the ops pointer.
>>>
>>> These can be easily fixed through re-ordering the id field to the 4-byte
>>> hole just after the controllers member.
>>
>> Looks good to me (just a driver dev, not a mux dev!). Also added
>> linux-i2c mailing list and a couple others for more review.
>>
>> Reviewed-by: Jesse Brandeburg <[email protected]>
>>
>> related thread (cocci script) at [1]
>>
>> [1]
>> https://lore.kernel.org/all/[email protected]/
>
> *thread necromancy*
>
> Can we land this? It's the last struct_size() instance that the above
> Coccinelle script flags.
>
> Reviewed-by: Kees Cook <[email protected]>
>
I'm happy to send a v2 if we need.
Thanks,
Jake
On Tue, Feb 20, 2024 at 01:27:45PM -0800, Jacob Keller wrote:
>
>
> On 2/18/2024 9:04 PM, Kees Cook wrote:
> > On Mon, Feb 27, 2023 at 12:28:43PM -0800, Jesse Brandeburg wrote:
> >> On 2/22/2023 5:42 PM, Jacob Keller wrote:
> >>> The mux_chip structure size is over allocated to additionally include both
> >>> the array of mux controllers as well as a device specific private area.
> >>> The controllers array is then pointed to by assigning mux_chip->mux to the
> >>> first block of extra memory, while the private area is extracted via
> >>> mux_chip_priv() and points to the area just after the controllers.
> >>>
> >>> The size of the mux_chip allocation uses direct multiplication and addition
> >>> rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
> >>> struct member wastes space by having to store the pointer as part of the
> >>> structures.
> >>>
> >>> Convert struct mux_chip to use a flexible array member for the mux
> >>> controller array. Use struct_size() and size_add() to compute the size of
> >>> the structure while protecting against overflow.
> >>>
> >>> After converting the mux pointer, notice that two 4-byte holes remain in
> >>> the structure layout due to the alignment requirements for the dev
> >>> sub-structure and the ops pointer.
> >>>
> >>> These can be easily fixed through re-ordering the id field to the 4-byte
> >>> hole just after the controllers member.
> >>
> >> Looks good to me (just a driver dev, not a mux dev!). Also added
> >> linux-i2c mailing list and a couple others for more review.
> >>
> >> Reviewed-by: Jesse Brandeburg <[email protected]>
> >>
> >> related thread (cocci script) at [1]
> >>
> >> [1]
> >> https://lore.kernel.org/all/[email protected]/
> >
> > *thread necromancy*
> >
> > Can we land this? It's the last struct_size() instance that the above
> > Coccinelle script flags.
> >
> > Reviewed-by: Kees Cook <[email protected]>
> >
>
> I'm happy to send a v2 if we need.
Since it's been a while, yeah, can you send a v2?
Thanks!
-Kees
--
Kees Cook