| Message ID | 20250302230220.245739-3-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] mux: Convert mux_control_ops to a flex array member in mux_chip | expand |
On Mon, Mar 03, 2025 at 12:02:22AM +0100, Thorsten Blum wrote: > Convert mux_control_ops to a flexible array member at the end of the > mux_chip struct and add the __counted_by() compiler attribute to > improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > CONFIG_FORTIFY_SOURCE. > > Use struct_size() to calculate the number of bytes to allocate for a new > mux chip and to remove the following Coccinelle/coccicheck warning: > > WARNING: Use struct_size > > Use size_add() to safely add any extra bytes. > > Compile-tested only. I believe this will fail at runtime. Note that sizeof_priv follows the allocation, so at the very least, you'd need to update: static inline void *mux_chip_priv(struct mux_chip *mux_chip) { return &mux_chip->mux[mux_chip->controllers]; } to not use the mux array itself as a location reference because it will be seen as out of bounds. To deal with this, the location will need to be calculated using mux_chip as the base, not mux_chip->mux as the base. For example, see commit 838ae9f45c4e ("nouveau/gsp: Avoid addressing beyond end of rpc->entries") -Kees > > Link: https://github.com/KSPP/linux/issues/83 > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> > --- > drivers/mux/core.c | 7 +++---- > include/linux/mux/driver.h | 4 ++-- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/mux/core.c b/drivers/mux/core.c > index 02be4ba37257..a3840fe0995f 100644 > --- a/drivers/mux/core.c > +++ b/drivers/mux/core.c > @@ -98,13 +98,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..e58e59354e23 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. > * @ops: Mux controller operations. > + * @mux: Array of mux controllers that are handled. > */ > struct mux_chip { > unsigned int controllers; > - struct mux_control *mux; > struct device dev; > int id; > > const struct mux_control_ops *ops; > + struct mux_control mux[] __counted_by(controllers); > }; > > #define to_mux_chip(x) container_of((x), struct mux_chip, dev) > -- > 2.48.1 > >
On 3. Mar 2025, at 19:44, Kees Cook wrote: > On Mon, Mar 03, 2025 at 12:02:22AM +0100, Thorsten Blum wrote: >> Convert mux_control_ops to a flexible array member at the end of the >> mux_chip struct and add the __counted_by() compiler attribute to >> improve access bounds-checking via CONFIG_UBSAN_BOUNDS and >> CONFIG_FORTIFY_SOURCE. >> >> Use struct_size() to calculate the number of bytes to allocate for a new >> mux chip and to remove the following Coccinelle/coccicheck warning: >> >> WARNING: Use struct_size >> >> Use size_add() to safely add any extra bytes. >> >> Compile-tested only. > > I believe this will fail at runtime. Note that sizeof_priv follows the > allocation, so at the very least, you'd need to update: > > static inline void *mux_chip_priv(struct mux_chip *mux_chip) > { > return &mux_chip->mux[mux_chip->controllers]; > } > > to not use the mux array itself as a location reference because it will > be seen as out of bounds. Getting the address doesn't fail at runtime, does it? For this example it works, but maybe I'm missing some compiler flag? https://godbolt.org/z/qTEdqn9WW Thanks, Thorsten
On Tue, Mar 04, 2025 at 09:58:21AM +0100, Thorsten Blum wrote: > On 3. Mar 2025, at 19:44, Kees Cook wrote: > > On Mon, Mar 03, 2025 at 12:02:22AM +0100, Thorsten Blum wrote: > >> Convert mux_control_ops to a flexible array member at the end of the > >> mux_chip struct and add the __counted_by() compiler attribute to > >> improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > >> CONFIG_FORTIFY_SOURCE. > >> > >> Use struct_size() to calculate the number of bytes to allocate for a new > >> mux chip and to remove the following Coccinelle/coccicheck warning: > >> > >> WARNING: Use struct_size > >> > >> Use size_add() to safely add any extra bytes. > >> > >> Compile-tested only. > > > > I believe this will fail at runtime. Note that sizeof_priv follows the > > allocation, so at the very least, you'd need to update: > > > > static inline void *mux_chip_priv(struct mux_chip *mux_chip) > > { > > return &mux_chip->mux[mux_chip->controllers]; > > } > > > > to not use the mux array itself as a location reference because it will > > be seen as out of bounds. > > Getting the address doesn't fail at runtime, does it? For this example > it works, but maybe I'm missing some compiler flag? > > https://godbolt.org/z/qTEdqn9WW Uhn. I can't explain that. :( We've seen this calculation get tripped in the real world, though: https://git.kernel.org/linus/a26a5107bc52 But yeah, when I build local test cases, grabbing an integral trips it, but taking an address does not, as your godbolt shows. This makes no sense to me at all. Here's my version, doing a direct comparison of int to *(int *) ... https://godbolt.org/z/e1bKGz739 #include <stdlib.h> #include <stdio.h> struct foo { int count; int array[] __attribute__((__counted_by__(count))); }; int main(int argc, char *argv[]) { int num_elems = 2 + argc; struct foo *p = malloc(sizeof(*p) + num_elems * sizeof(*p->array) + sizeof(int)); p->count = num_elems; // this correctly trips sanitizer: int val = p->array[num_elems]; printf("%d\n", val); // this does not?! int *valp = &p->array[num_elems]; printf("%p %d\n", valp, *valp); return 0; } Qing and Bill, are you able to explain this? If I set p->count = 0, 1, or 2, this trips. Is this somehow an off-by-one error in both GCC and Clang?
> On Mar 4, 2025, at 23:57, Kees Cook <kees@kernel.org> wrote: > > #include <stdlib.h> > #include <stdio.h> > > struct foo { > int count; > int array[] __attribute__((__counted_by__(count))); > }; > > int main(int argc, char *argv[]) { > int num_elems = 2 + argc; > > struct foo *p = malloc(sizeof(*p) + num_elems * sizeof(*p->array) + sizeof(int)); > p->count = num_elems; > > // this correctly trips sanitizer: > int val = p->array[num_elems]; > printf("%d\n", val); > > // this does not?! > int *valp = &p->array[num_elems]; > printf("%p %d\n", valp, *valp); > > return 0; > }
> On Mar 4, 2025, at 23:57, Kees Cook <kees@kernel.org> wrote: > > On Tue, Mar 04, 2025 at 09:58:21AM +0100, Thorsten Blum wrote: >> On 3. Mar 2025, at 19:44, Kees Cook wrote: >>> On Mon, Mar 03, 2025 at 12:02:22AM +0100, Thorsten Blum wrote: >>>> Convert mux_control_ops to a flexible array member at the end of the >>>> mux_chip struct and add the __counted_by() compiler attribute to >>>> improve access bounds-checking via CONFIG_UBSAN_BOUNDS and >>>> CONFIG_FORTIFY_SOURCE. >>>> >>>> Use struct_size() to calculate the number of bytes to allocate for a new >>>> mux chip and to remove the following Coccinelle/coccicheck warning: >>>> >>>> WARNING: Use struct_size >>>> >>>> Use size_add() to safely add any extra bytes. >>>> >>>> Compile-tested only. >>> >>> I believe this will fail at runtime. Note that sizeof_priv follows the >>> allocation, so at the very least, you'd need to update: >>> >>> static inline void *mux_chip_priv(struct mux_chip *mux_chip) >>> { >>> return &mux_chip->mux[mux_chip->controllers]; >>> } >>> >>> to not use the mux array itself as a location reference because it will >>> be seen as out of bounds. >> >> Getting the address doesn't fail at runtime, does it? For this example >> it works, but maybe I'm missing some compiler flag? >> >> https://godbolt.org/z/qTEdqn9WW > > Uhn. I can't explain that. :( We've seen this calculation get tripped > in the real world, though: > > https://git.kernel.org/linus/a26a5107bc52 > > But yeah, when I build local test cases, grabbing an integral trips it, > but taking an address does not, as your godbolt shows. This makes no > sense to me at all. > > Here's my version, doing a direct comparison of int to *(int *) ... > https://godbolt.org/z/e1bKGz739 > > #include <stdlib.h> > #include <stdio.h> > > struct foo { > int count; > int array[] __attribute__((__counted_by__(count))); > }; > > int main(int argc, char *argv[]) { > int num_elems = 2 + argc; > > struct foo *p = malloc(sizeof(*p) + num_elems * sizeof(*p->array) + sizeof(int)); > p->count = num_elems; > > // this correctly trips sanitizer: > int val = p->array[num_elems]; > printf("%d\n", val); > > // this does not?! > int *valp = &p->array[num_elems]; > printf("%p %d\n", valp, *valp); > > return 0; > } > > Qing and Bill, are you able to explain this? If I set p->count = 0, 1, or > 2, this trips. Is this somehow an off-by-one error in both GCC and Clang? This does look like a bug in the compiler, could you please file a bug against GCC on this? thanks. Qing > > -- > Kees Cook
On Wed, Mar 05, 2025 at 05:31:57PM +0000, Qing Zhao wrote:
> This does look like a bug in the compiler, could you please file a bug against GCC on this?
Okay, thanks for taking a looking!
GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119132
Clang: https://github.com/llvm/llvm-project/issues/129951
-Kees
On 3. Mar 2025, at 19:44, Kees Cook wrote: > On Mon, Mar 03, 2025 at 12:02:22AM +0100, Thorsten Blum wrote: >> Convert mux_control_ops to a flexible array member at the end of the >> mux_chip struct and add the __counted_by() compiler attribute to >> improve access bounds-checking via CONFIG_UBSAN_BOUNDS and >> CONFIG_FORTIFY_SOURCE. >> >> Use struct_size() to calculate the number of bytes to allocate for a new >> mux chip and to remove the following Coccinelle/coccicheck warning: >> >> WARNING: Use struct_size >> >> Use size_add() to safely add any extra bytes. >> >> Compile-tested only. > > I believe this will fail at runtime. Note that sizeof_priv follows the > allocation, so at the very least, you'd need to update: > > static inline void *mux_chip_priv(struct mux_chip *mux_chip) > { > return &mux_chip->mux[mux_chip->controllers]; > } > > to not use the mux array itself as a location reference because it will > be seen as out of bounds. > > To deal with this, the location will need to be calculated using > mux_chip as the base, not mux_chip->mux as the base. For example, see > commit 838ae9f45c4e ("nouveau/gsp: Avoid addressing beyond end of rpc->entries") Since this should work and is well-defined C code according to [1][2], could you give this patch another look or should I still change it and submit a v2? Thanks, Thorsten [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119132 [2] https://github.com/llvm/llvm-project/issues/129951
diff --git a/drivers/mux/core.c b/drivers/mux/core.c index 02be4ba37257..a3840fe0995f 100644 --- a/drivers/mux/core.c +++ b/drivers/mux/core.c @@ -98,13 +98,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..e58e59354e23 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. * @ops: Mux controller operations. + * @mux: Array of mux controllers that are handled. */ struct mux_chip { unsigned int controllers; - struct mux_control *mux; struct device dev; int id; const struct mux_control_ops *ops; + struct mux_control mux[] __counted_by(controllers); }; #define to_mux_chip(x) container_of((x), struct mux_chip, dev)
Convert mux_control_ops to a flexible array member at the end of the mux_chip struct and add the __counted_by() compiler attribute to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. Use struct_size() to calculate the number of bytes to allocate for a new mux chip and to remove the following Coccinelle/coccicheck warning: WARNING: Use struct_size Use size_add() to safely add any extra bytes. Compile-tested only. Link: https://github.com/KSPP/linux/issues/83 Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> --- drivers/mux/core.c | 7 +++---- include/linux/mux/driver.h | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-)