Re: [PATCH net 6/7] netfilter: xt_IDLETIMER: allocate timer with kzalloc()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: xt_IDLETIMER: allocate timer with kzalloc()

This switches the struct idletimer_tg allocation in
idletimer_tg_create_v1() from kmalloc_obj() to kzalloc_obj() so the
embedded struct device_attribute does not carry uninitialized function
pointers.  The v0 path already used kzalloc_obj(), so this aligns the
two.

> diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
> index fe7d8d19629b4..8a1d95d219917 100644
> --- a/net/netfilter/xt_IDLETIMER.c
> +++ b/net/netfilter/xt_IDLETIMER.c
> @@ -196,7 +196,7 @@ static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
>  {
>  	int ret;
>  
> -	info->timer = kmalloc_obj(*info->timer);
> +	info->timer = kzalloc_obj(*info->timer);
>  	if (!info->timer) {
>  		ret = -ENOMEM;
>  		goto out;

[Severity: Medium]
The change itself looks right, and zeroing does remove the uninitialized
attr.store / attr.store_const pointers, but does the mechanism described in
the commit message actually work?

The message says:

  "When the sysfs file is created, a process with the DAC_OVERRIDE
  capability can bypass the 0444 mode restriction and write to the file.
  This causes dev_attr_store() to execute the uninitialized store pointer,
  leading to arbitrary kernel code execution."

sysfs creates its kernfs root with the extra open permission check:

fs/sysfs/mount.c:sysfs_init() {
	sysfs_root = kernfs_create_root(NULL, KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
					NULL);
	...
}

and kernfs_fop_open() rejects the write open before any ->store dispatch:

fs/kernfs/file.c:kernfs_fop_open() {
	...
	if (root->flags & KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK) {
		if ((file->f_mode & FMODE_WRITE) &&
		    (!(inode->i_mode & S_IWUGO) || !has_write))
			goto err_out;
	...
}

idletimer_tg_create_v1() sets attr.attr.mode = 0444, and
sysfs_add_file_mode_ns() installs that mode verbatim via mode & 0777, so
S_IWUGO is clear on the inode and the write open fails with -EACCES
regardless of CAP_DAC_OVERRIDE.

Reaching the garbage pointer seems to require a privileged chmod of the
sysfs file first (permitted by kernfs_iop_setattr()), which is a different
and much narrower trigger than the capability bypass described.

Could the commit message be reworded to describe the actual trigger, given
that the Fixes: tag and the "arbitrary kernel code execution" wording will
drive stable backports?