drivers/mux/core.c | 7 +++---- include/linux/mux/driver.h | 10 +++++----- 2 files changed, 8 insertions(+), 9 deletions(-)
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 <jacob.e.keller@intel.com>
---
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.gd4c25cc71f83On 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 <jesse.brandeburg@intel.com> related thread (cocci script) at [1] [1] https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/
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 <jesse.brandeburg@intel.com> > > related thread (cocci script) at [1] > > [1] > https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/ *thread necromancy* Can we land this? It's the last struct_size() instance that the above Coccinelle script flags. Reviewed-by: Kees Cook <keescook@chromium.org> -- 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 <jesse.brandeburg@intel.com> >> >> related thread (cocci script) at [1] >> >> [1] >> https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/ > > *thread necromancy* > > Can we land this? It's the last struct_size() instance that the above > Coccinelle script flags. > > Reviewed-by: Kees Cook <keescook@chromium.org> > 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 <jesse.brandeburg@intel.com> > >> > >> related thread (cocci script) at [1] > >> > >> [1] > >> https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/ > > > > *thread necromancy* > > > > Can we land this? It's the last struct_size() instance that the above > > Coccinelle script flags. > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > 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
© 2016 - 2025 Red Hat, Inc.