diff mbox series

[RESEND,v2] params: Annotate struct module_param_attrs with __counted_by()

Message ID 20240909162725.1805-2-thorsten.blum@toblux.com (mailing list archive)
State In Next
Commit 7125cb974977aa80959817548b5221314fe46bc0
Headers show
Series [RESEND,v2] params: Annotate struct module_param_attrs with __counted_by() | expand

Commit Message

Thorsten Blum Sept. 9, 2024, 4:27 p.m. UTC 
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.blum@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.blum@toblux.com/
---
 kernel/params.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

Comments

Luis Chamberlain Sept. 11, 2024, 5:59 p.m. UTC | #1 
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.blum@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.blum@toblux.com/

Applied and pushed to modules-next, thanks!

  Luis
Nathan Chancellor Sept. 13, 2024, 4:46 p.m. UTC | #2 
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.blum@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.blum@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
  ...
  [    0.623959] ------------[ cut here ]------------
  [    0.624158] WARNING: CPU: 0 PID: 1 at lib/string_helpers.c:1030 __fortify_report+0x64/0x6c
  [    0.625050] memset: detected buffer overflow: 32 byte write of buffer size 0
  [    0.625297] Modules linked in:
  [    0.625712] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-rc4+ #1
  [    0.626008] Hardware name: Generic DT based system
  [    0.626274] Call trace:
  [    0.626342]  unwind_backtrace from show_stack+0x28/0x2c
  [    0.626690]  show_stack from dump_stack_lvl+0x34/0x70
  [    0.626936]  dump_stack_lvl from __warn+0xe0/0x200
  [    0.627100]  __warn from warn_slowpath_fmt+0xc8/0x194
  [    0.627335]  warn_slowpath_fmt from __fortify_report+0x64/0x6c
  [    0.627551]  __fortify_report from __fortify_panic+0x8/0xc
  [    0.627739]  __fortify_panic from add_sysfs_param+0x224/0x228
  [    0.627927]  add_sysfs_param from kernel_add_sysfs_param+0x5c/0xa0
  [    0.628206]  kernel_add_sysfs_param from param_sysfs_builtin+0xf4/0x130
  [    0.628412]  param_sysfs_builtin from param_sysfs_builtin_init+0x30/0x38
  [    0.628649]  param_sysfs_builtin_init from do_one_initcall+0xd0/0x2d4
  [    0.628869]  do_one_initcall from do_initcall_level+0x9c/0x104
  [    0.629068]  do_initcall_level from do_initcalls+0x74/0xb0
  [    0.629254]  do_initcalls from kernel_init_freeable+0x10c/0x158
  [    0.629452]  kernel_init_freeable from kernel_init+0x20/0x1cc
  [    0.629654]  kernel_init from ret_from_fork+0x14/0x28
  [    0.629872] Exception stack(0xe080dfb0 to 0xe080dff8)
  [    0.630108] dfa0:                                     00000000 00000000 00000000 00000000
  [    0.630391] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  [    0.630656] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
  [    0.631064] ---[ end trace 0000000000000000 ]---
  [    0.631800] ------------[ cut here ]------------
  [    0.631989] kernel BUG at lib/string_helpers.c:1037!
  [    0.632175] Internal error: Oops - BUG: 0 [#1] SMP ARM
  [    0.632472] Modules linked in:
  [    0.632761] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.11.0-rc4+ #1
  [    0.633077] Tainted: [W]=WARN
  [    0.633186] Hardware name: Generic DT based system
  [    0.633357] PC is at __fortify_panic+0x8/0xc
  [    0.633521] LR is at __warn+0x16c/0x200
  [    0.633668] pc : [<c0f6daa8>]    lr : [<c035300c>]    psr: 60000013
  [    0.633878] sp : e080db98  ip : 00000000  fp : e080dbb8
  [    0.634059] r10: 6e72656b  r9 : 006c656e  r8 : c1000009
  [    0.634240] r7 : fffffff4  r6 : c135947c  r5 : c13d5ebc  r4 : c2466f00
  [    0.634470] r3 : 00000200  r2 : 00000000  r1 : 244cee50  r0 : 244cee50
  [    0.634738] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
  [    0.634987] Control: 10c5387d  Table: 423ec06a  DAC: 00000051
  [    0.635206] Register r0 information: non-paged memory
  [    0.635559] Register r1 information: non-paged memory
  [    0.635745] Register r2 information: NULL pointer
  [    0.635917] Register r3 information: non-paged memory
  [    0.636100] Register r4 information: slab kmalloc-64 start c2466f00 pointer offset 0 size 64
  [    0.636772] Register r5 information: non-slab/vmalloc memory
  [    0.637001] Register r6 information: non-slab/vmalloc memory
  [    0.637205] Register r7 information: non-paged memory
  [    0.637385] Register r8 information: non-slab/vmalloc memory
  [    0.637597] Register r9 information: non-paged memory
  [    0.637791] Register r10 information: non-paged memory
  [    0.637977] Register r11 information: 2-page vmalloc region starting at 0xe080c000 allocated at copy_process+0x17c/0xf18
  [    0.638382] Register r12 information: NULL pointer
  [    0.638569] Process swapper/0 (pid: 1, stack limit = 0x8a022d6d)
  [    0.639040] Stack: (0xe080db98 to 0xe080e000)
  [    0.639194] db80:                                                       e080dbb8 c037f5f8
  [    0.639462] dba0: c2466f00 c13d5ebc 00000000 c1000009 e080dbd8 6e72656b e080dbd0 c151d860
  [    0.639728] dbc0: c13d5ebc c13d6dbc 00000000 6e72656b e080dc38 c151d6d8 6e72656b 00006c65
  [    0.639993] dbe0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  [    0.640261] dc00: 00000000 00000000 00000000 00000000 00000000 00000000 244cee50 c1655918
  [    0.640531] dc20: c215c200 c21f9300 c170f458 c151d4f8 00000000 00000000 e080dc40 c151d528
  [    0.640795] dc40: e080df40 c0302398 5e4e6cd4 ef580029 00000000 00000000 00000000 00000000
  [    0.641067] dc60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  [    0.641330] dc80: 00000000 00000000 00000000 00000000 ffffffff 00000000 c18926ac 00000080
  [    0.641599] dca0: 000000b0 c061cb68 debbd794 debbd794 00000000 c21fd000 e080dce8 c061cb68
  [    0.641863] dcc0: 244cee50 00000001 00000801 debbd794 00000000 00000801 c1892640 00000000
  [    0.642126] dce0: c1893440 e080ddd0 e080ddb0 c061e2ec deb63e40 deb63e5c 00000000 00052cc0
  [    0.642390] dd00: 00000001 00000001 000000b0 00000001 00000000 00000000 00000000 00000064
  [    0.642657] dd20: 0000006c 00000080 00000001 e080ddd0 00000000 00000000 00000000 00000001
  [    0.642923] dd40: 00000000 00000000 00000000 00000001 00000000 00000000 00000001 00000000
  [    0.643187] dd60: c1892930 00000000 00000000 244cee50 00000000 00000000 00000000 00000000
  [    0.643451] dd80: 00000801 7ffe0000 c1892640 c0b3c530 244cee50 00000000 00052cc0 00000001
  [    0.643724] dda0: 00000008 00000001 00052cc0 00000000 e080de08 c061cddc 00000820 c0775f50
  [    0.643992] ddc0: 00000000 c215c200 00000000 00052cc0 c1893440 00000000 c1893440 244cee50
  [    0.644259] dde0: 00000020 debbd794 7ffe0000 c062c97c c21329c0 debbd794 0000000e 00000020
  [    0.644532] de00: 00000080 7ffe0000 e080de40 c062c97c c20a763c 00000020 e080de28 c0f74fc0
  [    0.644800] de20: 244cee50 00000000 00000080 c2133e80 00000000 7fff0000 c21329c0 deb64100
  [    0.645066] de40: e080dea0 c062a584 c21c1180 c17cb818 0fffffff 00000001 e080ded4 c0f655d8
  [    0.645330] de60: 00000000 00000001 c2132a48 ffffffff 00000000 c0f43544 ffff0a01 c0f71f18
  [    0.645592] de80: e080df20 c0f71f18 244cee50 c21c1180 00000cc0 e080ded4 00000079 00000000
  [    0.645857] dea0: 0fffffff 00000001 00000000 c0f43544 c071ca3c 00000003 00000dc0 00000000
  [    0.646121] dec0: 00000cc0 00000000 0003ffff 80000053 c21fd66c c17cb818 00000000 00000000
  [    0.646121] dec0: 00000cc0 00000000 0003ffff 80000053 c21fd66c c17cb818 00000000 00000000
  [    0.646391] dee0: 00000000 00000000 000000c0 c13d6dbc 00000000 c037e2ec 000000c0 c037e328
  [    0.646663] df00: e080df40 c037e328 c21fd600 00000000 c12e093d c13d5ebc 00000000 244cee50
  [    0.646929] df20: 244cee50 c1655918 c1655aa0 c21f9300 deffff00 c13578a8 00000000 00000000
  [    0.647197] df40: e080df68 c1501898 00000007 00000007 00000000 c1501900 00000019 c21f9300
  [    0.647462] df60: 00000007 00000000 e080df88 c15017c0 c2143a00 c215c200 00000000 00000000
  [    0.647736] df80: 00000000 00000000 e080df98 c150163c 00000000 c0f6fa60 e080dfa8 c0f6fa80
  [    0.648006] dfa0: 00000000 00000000 00000000 c030016c 00000000 00000000 00000000 00000000
  [    0.648273] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  [    0.648536] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
  [    0.648801] Call trace:
  [    0.648840]  __fortify_panic from add_sysfs_param+0x224/0x228
  [    0.649129]  add_sysfs_param from kernel_add_sysfs_param+0x5c/0xa0
  [    0.649348]  kernel_add_sysfs_param from param_sysfs_builtin+0xf4/0x130
  [    0.649572]  param_sysfs_builtin from param_sysfs_builtin_init+0x30/0x38
  [    0.649801]  param_sysfs_builtin_init from do_one_initcall+0xd0/0x2d4
  [    0.650021]  do_one_initcall from do_initcall_level+0x9c/0x104
  [    0.650223]  do_initcall_level from do_initcalls+0x74/0xb0
  [    0.650412]  do_initcalls from kernel_init_freeable+0x10c/0x158
  [    0.650612]  kernel_init_freeable from kernel_init+0x20/0x1cc
  [    0.650810]  kernel_init from ret_from_fork+0x14/0x28
  [    0.650984] Exception stack(0xe080dfb0 to 0xe080dff8)
  [    0.651164] dfa0:                                     00000000 00000000 00000000 00000000
  [    0.651427] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  [    0.651697] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
  [    0.652032] Code: e8bd4bf0 eae457a7 e92d4800 ebe4aaef (e7f001f2)
  [    0.652584] ---[ end trace 0000000000000000 ]---
  [    0.652825] note: swapper/0[1] exited with irqs disabled
  [    0.653314] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
  [    0.654033] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

I haven't had a chance to really look over this too much but I figured I
would get the report out sooner rather than later, especially with the
merge window being around the corner. If there is any other information
I can provide or patches I can test, I am more than happy to do so.

Cheers,
Nathan

# bad: [57f962b956f1d116cd64d5c406776c4975de549d] Add linux-next specific files for 20240912
# good: [77f587896757708780a7e8792efe62939f25a5ab] Merge tag 'arm-fixes-6.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
git bisect start '57f962b956f1d116cd64d5c406776c4975de549d' '77f587896757708780a7e8792efe62939f25a5ab'
# good: [5e6a31306949eb18be8d6dc7ad6c42f8bea8e743] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 5e6a31306949eb18be8d6dc7ad6c42f8bea8e743
# bad: [9802faa85f4ccbc8cc0893fe76b939a9c56b9e62] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git
git bisect bad 9802faa85f4ccbc8cc0893fe76b939a9c56b9e62
# good: [793a135214a81488066fb8ccecbd4f182c416326] drm/xe/mmio: Drop compatibility macros
git bisect good 793a135214a81488066fb8ccecbd4f182c416326
# good: [d8bfeb4282b1c55342d2dd946eaf00f4f13785db] Merge branch 'for-next' of https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git
git bisect good d8bfeb4282b1c55342d2dd946eaf00f4f13785db
# bad: [f5315deb662e813be5fa93f2b67d46fe9fcd383c] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
git bisect bad f5315deb662e813be5fa93f2b67d46fe9fcd383c
# good: [4b1d9019b26fd654ebc2d0d2e100ed56ef1821f0] ASoC: dt-bindings: amlogic,axg-sound-card: document clocks property
git bisect good 4b1d9019b26fd654ebc2d0d2e100ed56ef1821f0
# bad: [2ca98b409c36e91bb359a2997f4a7810e799bd62] Merge branch 'modules-next' of git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
git bisect bad 2ca98b409c36e91bb359a2997f4a7810e799bd62
# good: [d39b510a145a4766906cd49177695913d6e6bfd2] Add audio support for the MediaTek Genio 350-evk
git bisect good d39b510a145a4766906cd49177695913d6e6bfd2
# good: [43fbb2c702663d7af844ee952aec224a0e059e26] ASoC: loongson: Simplify code formatting
git bisect good 43fbb2c702663d7af844ee952aec224a0e059e26
# good: [0b0aa67baa8904e3c1e13be48a2ca125f59ead3d] ASoC: amd: acp: remove MODULE_ALIAS for sof based generic machine driver
git bisect good 0b0aa67baa8904e3c1e13be48a2ca125f59ead3d
# good: [8905d39085c60a13d47bd07d2766bfdde933cc8d] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
git bisect good 8905d39085c60a13d47bd07d2766bfdde933cc8d
# good: [907fa79d787f6f762c0afb67b8b02be8ed1f7bd2] MAINTAINERS: scale modules with more reviewers
git bisect good 907fa79d787f6f762c0afb67b8b02be8ed1f7bd2
# bad: [e1d90ff7bc4b93c0b6c4821bef71b51ce145d7a7] module: abort module loading when sysfs setup suffer errors
git bisect bad e1d90ff7bc4b93c0b6c4821bef71b51ce145d7a7
# bad: [7125cb974977aa80959817548b5221314fe46bc0] params: Annotate struct module_param_attrs with __counted_by()
git bisect bad 7125cb974977aa80959817548b5221314fe46bc0
# first bad commit: [7125cb974977aa80959817548b5221314fe46bc0] params: Annotate struct module_param_attrs with __counted_by()
Luis Chamberlain Sept. 13, 2024, 4:55 p.m. UTC | #3 
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.blum@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.blum@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
Andy Shevchenko Sept. 13, 2024, 6:40 p.m. UTC | #4 
On Fri, Sep 13, 2024 at 09:46:30AM -0700, Nathan Chancellor wrote:
> 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.

> >  	/* Fix up all the pointers, since krealloc can move us */
> >  	for (i = 0; i < mk->mp->num; i++)

Shouldn't this for loop and followed by assignment also be -1:ed?
Thorsten Blum Sept. 13, 2024, 7:03 p.m. UTC | #5 
On 13. Sep 2024, at 20:40, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Sep 13, 2024 at 09:46:30AM -0700, Nathan Chancellor wrote:
>> 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.
> 
>>> /* Fix up all the pointers, since krealloc can move us */
>>> for (i = 0; i < mk->mp->num; i++)
> 
> Shouldn't this for loop and followed by assignment also be -1:ed?

That should be fine as mk->mp->num was already incremented before the
for-loop.
Andy Shevchenko Sept. 13, 2024, 7:09 p.m. UTC | #6 
On Fri, Sep 13, 2024 at 09:03:06PM +0200, Thorsten Blum wrote:
> On 13. Sep 2024, at 20:40, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 13, 2024 at 09:46:30AM -0700, Nathan Chancellor wrote:
> >> 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.
> > 
> >>> /* Fix up all the pointers, since krealloc can move us */
> >>> for (i = 0; i < mk->mp->num; i++)
> > 
> > Shouldn't this for loop and followed by assignment also be -1:ed?
> 
> That should be fine as mk->mp->num was already incremented before the
> for-loop.

Exactly my point. This is behavioural change AFAICS as the original code used
the old (-1:ed) value, no?
Andy Shevchenko Sept. 13, 2024, 7:12 p.m. UTC | #7 
On Fri, Sep 13, 2024 at 10:09:25PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 09:03:06PM +0200, Thorsten Blum wrote:
> > On 13. Sep 2024, at 20:40, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 13, 2024 at 09:46:30AM -0700, Nathan Chancellor wrote:
> > >> 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.
> > > 
> > >>> /* Fix up all the pointers, since krealloc can move us */
> > >>> for (i = 0; i < mk->mp->num; i++)
> > > 
> > > Shouldn't this for loop and followed by assignment also be -1:ed?
> > 
> > That should be fine as mk->mp->num was already incremented before the
> > for-loop.
> 
> Exactly my point. This is behavioural change AFAICS as the original code used
> the old (-1:ed) value, no?

Ah, I see now. It was in the original code, but closer to that loop...
Thorsten Blum Sept. 13, 2024, 11:32 p.m. UTC | #8 
Hi Nathan,

> On 13. Sep 2024, at 18:46, Nathan Chancellor <nathan@kernel.org> 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.blum@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.blum@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
> ...
> [    0.623959] ------------[ cut here ]------------
> [    0.624158] WARNING: CPU: 0 PID: 1 at lib/string_helpers.c:1030 __fortify_report+0x64/0x6c
> [    0.625050] memset: detected buffer overflow: 32 byte write of buffer size 0
> [    0.625297] Modules linked in:
> [    0.625712] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-rc4+ #1
> [    0.626008] Hardware name: Generic DT based system
> [    0.626274] Call trace:
> [    0.626342]  unwind_backtrace from show_stack+0x28/0x2c
> [    0.626690]  show_stack from dump_stack_lvl+0x34/0x70
> [    0.626936]  dump_stack_lvl from __warn+0xe0/0x200
> [    0.627100]  __warn from warn_slowpath_fmt+0xc8/0x194
> [    0.627335]  warn_slowpath_fmt from __fortify_report+0x64/0x6c
> [    0.627551]  __fortify_report from __fortify_panic+0x8/0xc
> [    0.627739]  __fortify_panic from add_sysfs_param+0x224/0x228
> [    0.627927]  add_sysfs_param from kernel_add_sysfs_param+0x5c/0xa0
> [    0.628206]  kernel_add_sysfs_param from param_sysfs_builtin+0xf4/0x130
> [    0.628412]  param_sysfs_builtin from param_sysfs_builtin_init+0x30/0x38
> [    0.628649]  param_sysfs_builtin_init from do_one_initcall+0xd0/0x2d4
> [    0.628869]  do_one_initcall from do_initcall_level+0x9c/0x104
> [    0.629068]  do_initcall_level from do_initcalls+0x74/0xb0
> [    0.629254]  do_initcalls from kernel_init_freeable+0x10c/0x158
> [    0.629452]  kernel_init_freeable from kernel_init+0x20/0x1cc
> [    0.629654]  kernel_init from ret_from_fork+0x14/0x28
> [    0.629872] Exception stack(0xe080dfb0 to 0xe080dff8)
> [    0.630108] dfa0:                                     00000000 00000000 00000000 00000000
> [    0.630391] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.630656] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    0.631064] ---[ end trace 0000000000000000 ]---
> [    0.631800] ------------[ cut here ]------------
> [    0.631989] kernel BUG at lib/string_helpers.c:1037!
> [    0.632175] Internal error: Oops - BUG: 0 [#1] SMP ARM
> [    0.632472] Modules linked in:
> [    0.632761] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.11.0-rc4+ #1
> [    0.633077] Tainted: [W]=WARN
> [    0.633186] Hardware name: Generic DT based system
> [    0.633357] PC is at __fortify_panic+0x8/0xc
> [    0.633521] LR is at __warn+0x16c/0x200
> [    0.633668] pc : [<c0f6daa8>]    lr : [<c035300c>]    psr: 60000013
> [    0.633878] sp : e080db98  ip : 00000000  fp : e080dbb8
> [    0.634059] r10: 6e72656b  r9 : 006c656e  r8 : c1000009
> [    0.634240] r7 : fffffff4  r6 : c135947c  r5 : c13d5ebc  r4 : c2466f00
> [    0.634470] r3 : 00000200  r2 : 00000000  r1 : 244cee50  r0 : 244cee50
> [    0.634738] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [    0.634987] Control: 10c5387d  Table: 423ec06a  DAC: 00000051
> [    0.635206] Register r0 information: non-paged memory
> [    0.635559] Register r1 information: non-paged memory
> [    0.635745] Register r2 information: NULL pointer
> [    0.635917] Register r3 information: non-paged memory
> [    0.636100] Register r4 information: slab kmalloc-64 start c2466f00 pointer offset 0 size 64
> [    0.636772] Register r5 information: non-slab/vmalloc memory
> [    0.637001] Register r6 information: non-slab/vmalloc memory
> [    0.637205] Register r7 information: non-paged memory
> [    0.637385] Register r8 information: non-slab/vmalloc memory
> [    0.637597] Register r9 information: non-paged memory
> [    0.637791] Register r10 information: non-paged memory
> [    0.637977] Register r11 information: 2-page vmalloc region starting at 0xe080c000 allocated at copy_process+0x17c/0xf18
> [    0.638382] Register r12 information: NULL pointer
> [    0.638569] Process swapper/0 (pid: 1, stack limit = 0x8a022d6d)
> [    0.639040] Stack: (0xe080db98 to 0xe080e000)
> [    0.639194] db80:                                                       e080dbb8 c037f5f8
> [    0.639462] dba0: c2466f00 c13d5ebc 00000000 c1000009 e080dbd8 6e72656b e080dbd0 c151d860
> [    0.639728] dbc0: c13d5ebc c13d6dbc 00000000 6e72656b e080dc38 c151d6d8 6e72656b 00006c65
> [    0.639993] dbe0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.640261] dc00: 00000000 00000000 00000000 00000000 00000000 00000000 244cee50 c1655918
> [    0.640531] dc20: c215c200 c21f9300 c170f458 c151d4f8 00000000 00000000 e080dc40 c151d528
> [    0.640795] dc40: e080df40 c0302398 5e4e6cd4 ef580029 00000000 00000000 00000000 00000000
> [    0.641067] dc60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.641330] dc80: 00000000 00000000 00000000 00000000 ffffffff 00000000 c18926ac 00000080
> [    0.641599] dca0: 000000b0 c061cb68 debbd794 debbd794 00000000 c21fd000 e080dce8 c061cb68
> [    0.641863] dcc0: 244cee50 00000001 00000801 debbd794 00000000 00000801 c1892640 00000000
> [    0.642126] dce0: c1893440 e080ddd0 e080ddb0 c061e2ec deb63e40 deb63e5c 00000000 00052cc0
> [    0.642390] dd00: 00000001 00000001 000000b0 00000001 00000000 00000000 00000000 00000064
> [    0.642657] dd20: 0000006c 00000080 00000001 e080ddd0 00000000 00000000 00000000 00000001
> [    0.642923] dd40: 00000000 00000000 00000000 00000001 00000000 00000000 00000001 00000000
> [    0.643187] dd60: c1892930 00000000 00000000 244cee50 00000000 00000000 00000000 00000000
> [    0.643451] dd80: 00000801 7ffe0000 c1892640 c0b3c530 244cee50 00000000 00052cc0 00000001
> [    0.643724] dda0: 00000008 00000001 00052cc0 00000000 e080de08 c061cddc 00000820 c0775f50
> [    0.643992] ddc0: 00000000 c215c200 00000000 00052cc0 c1893440 00000000 c1893440 244cee50
> [    0.644259] dde0: 00000020 debbd794 7ffe0000 c062c97c c21329c0 debbd794 0000000e 00000020
> [    0.644532] de00: 00000080 7ffe0000 e080de40 c062c97c c20a763c 00000020 e080de28 c0f74fc0
> [    0.644800] de20: 244cee50 00000000 00000080 c2133e80 00000000 7fff0000 c21329c0 deb64100
> [    0.645066] de40: e080dea0 c062a584 c21c1180 c17cb818 0fffffff 00000001 e080ded4 c0f655d8
> [    0.645330] de60: 00000000 00000001 c2132a48 ffffffff 00000000 c0f43544 ffff0a01 c0f71f18
> [    0.645592] de80: e080df20 c0f71f18 244cee50 c21c1180 00000cc0 e080ded4 00000079 00000000
> [    0.645857] dea0: 0fffffff 00000001 00000000 c0f43544 c071ca3c 00000003 00000dc0 00000000
> [    0.646121] dec0: 00000cc0 00000000 0003ffff 80000053 c21fd66c c17cb818 00000000 00000000
> [    0.646121] dec0: 00000cc0 00000000 0003ffff 80000053 c21fd66c c17cb818 00000000 00000000
> [    0.646391] dee0: 00000000 00000000 000000c0 c13d6dbc 00000000 c037e2ec 000000c0 c037e328
> [    0.646663] df00: e080df40 c037e328 c21fd600 00000000 c12e093d c13d5ebc 00000000 244cee50
> [    0.646929] df20: 244cee50 c1655918 c1655aa0 c21f9300 deffff00 c13578a8 00000000 00000000
> [    0.647197] df40: e080df68 c1501898 00000007 00000007 00000000 c1501900 00000019 c21f9300
> [    0.647462] df60: 00000007 00000000 e080df88 c15017c0 c2143a00 c215c200 00000000 00000000
> [    0.647736] df80: 00000000 00000000 e080df98 c150163c 00000000 c0f6fa60 e080dfa8 c0f6fa80
> [    0.648006] dfa0: 00000000 00000000 00000000 c030016c 00000000 00000000 00000000 00000000
> [    0.648273] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.648536] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [    0.648801] Call trace:
> [    0.648840]  __fortify_panic from add_sysfs_param+0x224/0x228
> [    0.649129]  add_sysfs_param from kernel_add_sysfs_param+0x5c/0xa0
> [    0.649348]  kernel_add_sysfs_param from param_sysfs_builtin+0xf4/0x130
> [    0.649572]  param_sysfs_builtin from param_sysfs_builtin_init+0x30/0x38
> [    0.649801]  param_sysfs_builtin_init from do_one_initcall+0xd0/0x2d4
> [    0.650021]  do_one_initcall from do_initcall_level+0x9c/0x104
> [    0.650223]  do_initcall_level from do_initcalls+0x74/0xb0
> [    0.650412]  do_initcalls from kernel_init_freeable+0x10c/0x158
> [    0.650612]  kernel_init_freeable from kernel_init+0x20/0x1cc
> [    0.650810]  kernel_init from ret_from_fork+0x14/0x28
> [    0.650984] Exception stack(0xe080dfb0 to 0xe080dff8)
> [    0.651164] dfa0:                                     00000000 00000000 00000000 00000000
> [    0.651427] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.651697] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    0.652032] Code: e8bd4bf0 eae457a7 e92d4800 ebe4aaef (e7f001f2)
> [    0.652584] ---[ end trace 0000000000000000 ]---
> [    0.652825] note: swapper/0[1] exited with irqs disabled
> [    0.653314] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    0.654033] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
> 
> I haven't had a chance to really look over this too much but I figured I
> would get the report out sooner rather than later, especially with the
> merge window being around the corner. If there is any other information
> I can provide or patches I can test, I am more than happy to do so.
> 
> Cheers,
> Nathan
> 
> # bad: [57f962b956f1d116cd64d5c406776c4975de549d] Add linux-next specific files for 20240912
> # good: [77f587896757708780a7e8792efe62939f25a5ab] Merge tag 'arm-fixes-6.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
> git bisect start '57f962b956f1d116cd64d5c406776c4975de549d' '77f587896757708780a7e8792efe62939f25a5ab'
> # good: [5e6a31306949eb18be8d6dc7ad6c42f8bea8e743] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git bisect good 5e6a31306949eb18be8d6dc7ad6c42f8bea8e743
> # bad: [9802faa85f4ccbc8cc0893fe76b939a9c56b9e62] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git
> git bisect bad 9802faa85f4ccbc8cc0893fe76b939a9c56b9e62
> # good: [793a135214a81488066fb8ccecbd4f182c416326] drm/xe/mmio: Drop compatibility macros
> git bisect good 793a135214a81488066fb8ccecbd4f182c416326
> # good: [d8bfeb4282b1c55342d2dd946eaf00f4f13785db] Merge branch 'for-next' of https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git
> git bisect good d8bfeb4282b1c55342d2dd946eaf00f4f13785db
> # bad: [f5315deb662e813be5fa93f2b67d46fe9fcd383c] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
> git bisect bad f5315deb662e813be5fa93f2b67d46fe9fcd383c
> # good: [4b1d9019b26fd654ebc2d0d2e100ed56ef1821f0] ASoC: dt-bindings: amlogic,axg-sound-card: document clocks property
> git bisect good 4b1d9019b26fd654ebc2d0d2e100ed56ef1821f0
> # bad: [2ca98b409c36e91bb359a2997f4a7810e799bd62] Merge branch 'modules-next' of git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
> git bisect bad 2ca98b409c36e91bb359a2997f4a7810e799bd62
> # good: [d39b510a145a4766906cd49177695913d6e6bfd2] Add audio support for the MediaTek Genio 350-evk
> git bisect good d39b510a145a4766906cd49177695913d6e6bfd2
> # good: [43fbb2c702663d7af844ee952aec224a0e059e26] ASoC: loongson: Simplify code formatting
> git bisect good 43fbb2c702663d7af844ee952aec224a0e059e26
> # good: [0b0aa67baa8904e3c1e13be48a2ca125f59ead3d] ASoC: amd: acp: remove MODULE_ALIAS for sof based generic machine driver
> git bisect good 0b0aa67baa8904e3c1e13be48a2ca125f59ead3d
> # good: [8905d39085c60a13d47bd07d2766bfdde933cc8d] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
> git bisect good 8905d39085c60a13d47bd07d2766bfdde933cc8d
> # good: [907fa79d787f6f762c0afb67b8b02be8ed1f7bd2] MAINTAINERS: scale modules with more reviewers
> git bisect good 907fa79d787f6f762c0afb67b8b02be8ed1f7bd2
> # bad: [e1d90ff7bc4b93c0b6c4821bef71b51ce145d7a7] module: abort module loading when sysfs setup suffer errors
> git bisect bad e1d90ff7bc4b93c0b6c4821bef71b51ce145d7a7
> # bad: [7125cb974977aa80959817548b5221314fe46bc0] params: Annotate struct module_param_attrs with __counted_by()
> git bisect bad 7125cb974977aa80959817548b5221314fe46bc0
> # first bad commit: [7125cb974977aa80959817548b5221314fe46bc0] params: Annotate struct module_param_attrs with __counted_by()

Thanks for reporting this.

Changing

 memset(&mk->mp->attrs[mk->mp->num - 1], 0, sizeof(mk->mp->attrs[0]));

to

 memset(mk->mp->attrs + mk->mp->num - 1, 0, sizeof(mk->mp->attrs[0]));

fixes the false-positive warning

 memset: detected buffer overflow: 32 byte write of buffer size 0

even though the pointers have the same value. Does anyone know why?

Thanks,
Thorsten
Nathan Chancellor Sept. 13, 2024, 11:44 p.m. UTC | #9 
On Sat, Sep 14, 2024 at 01:32:19AM +0200, Thorsten Blum wrote:
> Thanks for reporting this.
> 
> Changing
> 
>  memset(&mk->mp->attrs[mk->mp->num - 1], 0, sizeof(mk->mp->attrs[0]));
> 
> to
> 
>  memset(mk->mp->attrs + mk->mp->num - 1, 0, sizeof(mk->mp->attrs[0]));
> 
> fixes the false-positive warning
> 
>  memset: detected buffer overflow: 32 byte write of buffer size 0
> 
> even though the pointers have the same value. Does anyone know why?

Might be a good question for Bill? The full context is available
starting at:

  https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/

I wonder if the krealloc() has something to do with it? I should try GCC
but I don't have a tip of tree copy handy at the moment and I am also
rushing at the end of my day to pack for my travels to LPC :)

Cheers,
Nathan
Thorsten Blum Sept. 14, 2024, 12:23 a.m. UTC | #10 
On 14. Sep 2024, at 01:44, Nathan Chancellor <nathan@kernel.org> wrote:
> On Sat, Sep 14, 2024 at 01:32:19AM +0200, Thorsten Blum wrote:
>> Thanks for reporting this.
>> 
>> Changing
>> 
>> memset(&mk->mp->attrs[mk->mp->num - 1], 0, sizeof(mk->mp->attrs[0]));
>> 
>> to
>> 
>> memset(mk->mp->attrs + mk->mp->num - 1, 0, sizeof(mk->mp->attrs[0]));
>> 
>> fixes the false-positive warning
>> 
>> memset: detected buffer overflow: 32 byte write of buffer size 0
>> 
>> even though the pointers have the same value. Does anyone know why?
> 
> Might be a good question for Bill? The full context is available
> starting at:
> 
> https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/
> 
> I wonder if the krealloc() has something to do with it? I should try GCC
> but I don't have a tip of tree copy handy at the moment and I am also
> rushing at the end of my day to pack for my travels to LPC :)

I think the problem is with __builtin_dynamic_object_size().

memset(p,,) calls __struct_size(p), which calls
__builtin_dynamic_object_size(p, 0) and this behaves weirdly:

 __builtin_dynamic_object_size(&mk->mp->attrs[mk->mp->num - 1], 0);

evaluates to 0, but

 __builtin_dynamic_object_size(mk->mp->attrs + mk->mp->num - 1, 0);

evaluates to 4294967295.

Both values are wrong, but the latter doesn't trigger the
false-positive warning.
Bill Wendling Sept. 16, 2024, 9:45 a.m. UTC | #11 
On Fri, Sep 13, 2024 at 5:23 PM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>
> On 14. Sep 2024, at 01:44, Nathan Chancellor <nathan@kernel.org> wrote:
> > On Sat, Sep 14, 2024 at 01:32:19AM +0200, Thorsten Blum wrote:
> >> Thanks for reporting this.
> >>
> >> Changing
> >>
> >> memset(&mk->mp->attrs[mk->mp->num - 1], 0, sizeof(mk->mp->attrs[0]));
> >>
> >> to
> >>
> >> memset(mk->mp->attrs + mk->mp->num - 1, 0, sizeof(mk->mp->attrs[0]));
> >>
> >> fixes the false-positive warning
> >>
> >> memset: detected buffer overflow: 32 byte write of buffer size 0
> >>
> >> even though the pointers have the same value. Does anyone know why?
> >
> > Might be a good question for Bill? The full context is available
> > starting at:
> >
> > https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/
> >
> > I wonder if the krealloc() has something to do with it? I should try GCC
> > but I don't have a tip of tree copy handy at the moment and I am also
> > rushing at the end of my day to pack for my travels to LPC :)
>
> I think the problem is with __builtin_dynamic_object_size().
>
> memset(p,,) calls __struct_size(p), which calls
> __builtin_dynamic_object_size(p, 0) and this behaves weirdly:
>
>  __builtin_dynamic_object_size(&mk->mp->attrs[mk->mp->num - 1], 0);
>
> evaluates to 0, but
>
>  __builtin_dynamic_object_size(mk->mp->attrs + mk->mp->num - 1, 0);
>
> evaluates to 4294967295.
>
> Both values are wrong, but the latter doesn't trigger the
> false-positive warning.

The 4294967295 simply means "I don't know." There's probably a bug in
the size calculation. I'll look into it.

-bw
Kees Cook Sept. 17, 2024, 11:43 a.m. UTC | #12 
On Mon, Sep 16, 2024 at 02:45:47AM -0700, Bill Wendling wrote:
> The 4294967295 simply means "I don't know." There's probably a bug in
> the size calculation. I'll look into it.

I was able to build a minimized PoC, if that's helpful:

https://godbolt.org/z/qohGd5xh1


#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

int main(int argc, char *argv[])
{
        struct bucket *p;
        struct variable *v;

        p = malloc(sizeof(*p));
        v = malloc(sizeof(*p->growable) + sizeof(*p->growable->array) * 32);
        v->length = 32;


        printf("%zu\n", __builtin_dynamic_object_size(v->array, 1));

        p->growable = v;
        printf("%zu\n", __builtin_dynamic_object_size(p->growable->array, 1));

        return 0;
}


GCC shows 64 64, but Clang shows 64 0.
diff mbox series

Patch

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++)