On Fri, Sep 13, 2024 at 09:46:30AM -0700, Nathan Chancellor wrote: > Hi Thorsten, > > On Mon, Sep 09, 2024 at 06:27:26PM +0200, Thorsten Blum wrote: > > Add the __counted_by compiler attribute to the flexible array member > > attrs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > > CONFIG_FORTIFY_SOURCE. > > > > Increment num before adding a new param_attribute to the attrs array and > > adjust the array index accordingly. Increment num immediately after the > > first reallocation such that the reallocation for the NULL terminator > > only needs to add 1 (instead of 2) to mk->mp->num. > > > > Use struct_size() instead of manually calculating the size for the > > reallocation. > > > > Use krealloc_array() for the additional NULL terminator. > > > > Signed-off-by: Thorsten Blum <thorsten.b...@toblux.com> > > --- > > Changes in v2: > > - Use krealloc_array() as suggested by Andy Shevchenko > > - Link to v1: > > https://lore.kernel.org/linux-kernel/20240823123300.37574-1-thorsten.b...@toblux.com/ > > --- > > kernel/params.c | 29 +++++++++++++---------------- > > 1 file changed, 13 insertions(+), 16 deletions(-) > > > > diff --git a/kernel/params.c b/kernel/params.c > > index 2e447f8ae183..5f6643676697 100644 > > --- a/kernel/params.c > > +++ b/kernel/params.c > > @@ -551,7 +551,7 @@ struct module_param_attrs > > { > > unsigned int num; > > struct attribute_group grp; > > - struct param_attribute attrs[]; > > + struct param_attribute attrs[] __counted_by(num); > > }; > > > > #ifdef CONFIG_SYSFS > > @@ -651,35 +651,32 @@ static __modinit int add_sysfs_param(struct > > module_kobject *mk, > > } > > > > /* Enlarge allocations. */ > > - new_mp = krealloc(mk->mp, > > - sizeof(*mk->mp) + > > - sizeof(mk->mp->attrs[0]) * (mk->mp->num + 1), > > + new_mp = krealloc(mk->mp, struct_size(mk->mp, attrs, mk->mp->num + 1), > > GFP_KERNEL); > > if (!new_mp) > > return -ENOMEM; > > mk->mp = new_mp; > > + mk->mp->num++; > > > > /* Extra pointer for NULL terminator */ > > - new_attrs = krealloc(mk->mp->grp.attrs, > > - sizeof(mk->mp->grp.attrs[0]) * (mk->mp->num + 2), > > - GFP_KERNEL); > > + new_attrs = krealloc_array(mk->mp->grp.attrs, mk->mp->num + 1, > > + sizeof(mk->mp->grp.attrs[0]), GFP_KERNEL); > > if (!new_attrs) > > return -ENOMEM; > > mk->mp->grp.attrs = new_attrs; > > > > /* Tack new one on the end. */ > > - memset(&mk->mp->attrs[mk->mp->num], 0, sizeof(mk->mp->attrs[0])); > > - sysfs_attr_init(&mk->mp->attrs[mk->mp->num].mattr.attr); > > - mk->mp->attrs[mk->mp->num].param = kp; > > - mk->mp->attrs[mk->mp->num].mattr.show = param_attr_show; > > + memset(&mk->mp->attrs[mk->mp->num - 1], 0, sizeof(mk->mp->attrs[0])); > > + sysfs_attr_init(&mk->mp->attrs[mk->mp->num - 1].mattr.attr); > > + mk->mp->attrs[mk->mp->num - 1].param = kp; > > + mk->mp->attrs[mk->mp->num - 1].mattr.show = param_attr_show; > > /* Do not allow runtime DAC changes to make param writable. */ > > if ((kp->perm & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0) > > - mk->mp->attrs[mk->mp->num].mattr.store = param_attr_store; > > + mk->mp->attrs[mk->mp->num - 1].mattr.store = param_attr_store; > > else > > - mk->mp->attrs[mk->mp->num].mattr.store = NULL; > > - mk->mp->attrs[mk->mp->num].mattr.attr.name = (char *)name; > > - mk->mp->attrs[mk->mp->num].mattr.attr.mode = kp->perm; > > - mk->mp->num++; > > + mk->mp->attrs[mk->mp->num - 1].mattr.store = NULL; > > + mk->mp->attrs[mk->mp->num - 1].mattr.attr.name = (char *)name; > > + mk->mp->attrs[mk->mp->num - 1].mattr.attr.mode = kp->perm; > > > > /* Fix up all the pointers, since krealloc can move us */ > > for (i = 0; i < mk->mp->num; i++) > > -- > > 2.46.0 > > > > I just bisected this change to a fortify failure that I see with a > couple different ARM configurations when built with a version of clang > that supports __counted_by(). I can reproduce this with: > > $ curl -LSso .config > https://gist.github.com/nathanchance/6df4bd2ab4c4418020b3ed5417ef4f80/raw/758abf666dfe8c76e0f16735f336849ea47ef791/.config > > $ make -skj"$(nproc)" ARCH=arm LLVM=1 olddefconfig zImage > > $ curl -LSs > https://github.com/ClangBuiltLinux/boot-utils/releases/download/20230707-182910/arm-rootfs.cpio.zst > | zstd -d >rootfs.cpio > > $ qemu-system-arm \ > -display none \ > -nodefaults \ > -no-reboot \ > -machine virt \ > -append 'console=ttyAMA0 earlycon' \ > -kernel arch/arm/boot/zImage \ > -initrd rootfs.cpio \ > -m 512m \ > -serial mon:stdio > [ 0.000000] Booting Linux on physical CPU 0x0 > [ 0.000000] Linux version 6.11.0-rc4+ (nathan@thelio-3990X) > (ClangBuiltLinux clang version 19.1.0-rc4 > (https://github.com/llvm/llvm-project.git > 0c641568515a797473394694f05937e1f1913d87), ClangBuiltLinux LLD 19.1.0 > (https://github.com/llvm/llvm-project.git > 0c641568515a797473394694f05937e1f1913d87)) #1 SMP Fri Sep 13 09:36:38 MST 2024 > ...
Thanks, I've dropped this patch from modules-next for now. Luis
