Re: [PATCH 11/11] kobject: Require explicit initialization of struct kobj_type::release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sysfs: Use-After-Free in kobject delayed release via devm/memset
- [Low] sysfs: incorrect documentation for dss_overlay_kobj_uninit() cleanup
--

commit 25c0a8c68e5a115989252d41a0d4731867415d79
Author: Ian Rogers <irogers@google.com>
Subject: kobject: Require explicit initialization of struct kobj_type::release

Annotate struct kobj_type::release with __require_explicit_initialization
so the compiler diagnoses any struct kobj_type initializer that omits
.release. For the remaining legacy struct kobj_type definitions where the
kobject is static, per-CPU, or embedded inside a devm- or parent-managed
structure rather than freed by kobject_put(), explicitly initialize
.release = NULL.

Link: https://lpc.events/event/20/contributions/2392/

> diff --git a/drivers/dma/ioat/sysfs.c b/drivers/dma/ioat/sysfs.c
> index e796ddb5383f8..7fdd0ca36b98a 100644
> --- a/drivers/dma/ioat/sysfs.c
> +++ b/drivers/dma/ioat/sysfs.c
> @@ -167,6 +167,8 @@ static const struct attribute *const ioat_attrs[] = {
>  ATTRIBUTE_GROUPS(ioat);
>  
>  const struct kobj_type ioat_ktype = {
> +	/* ioatdma_chan is allocated via devm_kzalloc() rather than freed by kobject_put(). */
> +	.release = NULL,

[Severity: High]
Does this setup cause a use-after-free when CONFIG_DEBUG_KOBJECT_RELEASE is
enabled?

When kobject_put() is called, it queues a delayed_work to execute
kobject_cleanup() later. Because the struct is managed by devm_kzalloc(),
the memory is freed immediately after unbind, leaving an active delayed_work
pointing to the freed memory.

[ ... ]

> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index 0cc2953d59a54..5d083d2b72caa 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -1038,6 +1038,8 @@ static struct attribute *hccs_port_default_attrs[] = {
>  ATTRIBUTE_GROUPS(hccs_port_default);
>  
>  static const struct kobj_type hccs_port_type = {
> +	/* hccs_port_info is allocated via devm_kcalloc() rather than freed by kobject_put(). */
> +	.release = NULL,

[Severity: High]
Can this devm allocation cause a use-after-free for the delayed_work?

Similar to the ioat_ktype case, the devm_kcalloc allocation means the struct
containing the kobject is freed upon driver unbind immediately after
kobject_put() executes, leaving a pending delayed_work on freed memory.

[ ... ]

> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/display-sysfs.c b/drivers/video/fbdev/omap2/omapfb/dss/display-sysfs.c
> index ae937854403b1..3d787245f5b51 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/display-sysfs.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/display-sysfs.c
> @@ -304,6 +304,8 @@ static const struct sysfs_ops display_sysfs_ops = {
>  };
>  
>  static struct kobj_type display_ktype = {
> +	/* dssdev->kobj is embedded in struct omap_dss_device and zeroed in display_uninit_sysfs(). */
> +	.release = NULL,

[Severity: High]
Does this zeroing pattern corrupt the active workqueue when
CONFIG_DEBUG_KOBJECT_RELEASE is enabled?

Looking at display_uninit_sysfs(), the code calls kobject_put() followed
immediately by memset:

void display_uninit_sysfs(struct omap_dss_device *dssdev)
{
	...
	kobject_del(&dssdev->kobj);
	kobject_put(&dssdev->kobj);

	memset(&dssdev->kobj, 0, sizeof(dssdev->kobj));
}

Since kobject_put() schedules a delayed_work, wouldn't the memset
synchronously destroy the active work struct before it can run?

[ ... ]

> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/overlay-sysfs.c b/drivers/video/fbdev/omap2/omapfb/dss/overlay-sysfs.c
> index 1da4fb1c77b4a..9aa16b3dee5ef 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/overlay-sysfs.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/overlay-sysfs.c
> @@ -429,6 +429,8 @@ static const struct sysfs_ops overlay_sysfs_ops = {
>  };
>  
>  static struct kobj_type overlay_ktype = {
> +	/* ovl->kobj is embedded in static overlays[]; dss_overlay_kobj_uninit() only calls kobject_del(). */
> +	.release = NULL,

[Severity: Low]
Is this comment accurate?

Looking at dss_overlay_kobj_uninit() in the same file:

void dss_overlay_kobj_uninit(struct omap_overlay *ovl)
{
	kobject_del(&ovl->kobj);
	kobject_put(&ovl->kobj);
}

The implementation contradicts the comment by explicitly calling
kobject_put().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013223946.1836757-1-irogers@google.com?part=11