Re: [PATCH v1 1/5] dmaengine: add support for custom per-channel sysfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Embedding a raw `kobject` into `struct dma_chan` without governing the channel's lifecycle creates an unavoidable Use-After-Free vulnerability due to a strict kobject lifecycle mismatch.
- [High] The `name` parameter is passed directly as the `fmt` string to `kobject_init_and_add`, leading to a format string vulnerability.
- [Medium] `dma_chan_kobject_add` calculates an invalid parent pointer if called before the channel is fully registered.
- [Low] Typographical errors in the kobject initialization failure warning message.
--

--- Patch [1]: [PATCH v1 1/5] dmaengine: add support for custom per-channel sysfs attributes ---
commit e1443713f7d441022da01ca370f8ab1129a41d63
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: add support for custom per-channel sysfs attributes

This commit adds specific support for adding sysfs attributes to DMA channels.
It is intended to be used to replace similar functionality in ioat so it can
be utilized in other drivers.

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 9049171df8578..7ea2a8709c078 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
[ ... ]
> @@ -125,6 +125,63 @@ static int dmaengine_summary_show(struct seq_file *s, void *data)
[ ... ]
> +void dma_chan_kobject_add(struct dma_device *dev, const struct kobj_type *type,
> +			  const char *name)
> +{
> +	struct dma_chan *chan;
> +	int err;
> +
> +	list_for_each_entry(chan, &dev->channels, device_node) {
> +		chan->kobj_used = true;
> +		err = kobject_init_and_add(&chan->kobj, type,
> +					   &chan->dev->device.kobj, name);

[Severity: High]
Could this introduce a format string vulnerability? kobject_init_and_add()
takes a format string as its fourth argument, and passing 'name' directly
means any format specifiers in the driver-provided name could cause
unpredictable behavior. Should this be passed safely as "%s", name?

[Severity: Medium]
Will this calculate an invalid parent pointer if a driver calls this function
before the channel is fully registered? 

Since chan->dev is dynamically allocated during dma_async_device_register(), 
wouldn't this perform pointer arithmetic on a NULL pointer if called during a 
driver's probe routine prior to registration?

> +		if (err) {
> +			dev_warn(dev->dev,
> +				 "sysis init error(%d), continuinng...\n", err);

[Severity: Low]
This isn't a bug, but there are typos in the warning message ("sysis" instead
of "sysfs" and "continuinng" instead of "continuing").

> +			kobject_put(&chan->kobj);
> +			chan->kobj_used = false;

[Severity: High]
Does this error handling path introduce a use-after-free? 

The kobject lifecycle API mandates that the release callback must free the 
structure containing the kobject. If the driver complies and frees 'chan' 
during kobject_put(), won't setting chan->kobj_used and the subsequent list 
iteration dereference freed memory?

> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(dma_chan_kobject_add);
[ ... ]
> @@ -1143,6 +1200,11 @@ static void __dma_async_device_channel_unregister(struct dma_device *device,
>  	if (chan->local == NULL)
>  		return;
>  
> +	if (chan->kobj_used) {
> +		kobject_del(&chan->kobj);
> +		kobject_put(&chan->kobj);

[Severity: High]
If a driver does not free 'chan' in the kobject release callback to avoid the
error path use-after-free in dma_chan_kobject_add(), does this create a 
different use-after-free here?

If userspace holds an open file descriptor to the sysfs attributes, the
kobject reference count remains above zero after kobject_put(). The driver
will proceed to free the dma_chan memory during unloading, leaving sysfs with
a reference to freed memory.

> +	}
> +
>  	WARN_ONCE(!device->device_release && chan->client_count,
[ ... ]
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b3d251c9734e9..852943ef46b8e 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
[ ... ]
> @@ -358,6 +358,9 @@ struct dma_chan {
>  	struct dma_router *router;
>  	void *route_data;
>  
> +	bool kobj_used;
> +	struct kobject kobj;

[Severity: High]
Does embedding a raw kobject without governing the channel's lifecycle create
an unavoidable use-after-free vulnerability? 

This structural approach forces a mismatch between the kobject lifecycle and 
the dma_chan lifecycle, triggering one of the two use-after-free scenarios 
noted above.

> +
>  	void *private;
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707162045.23910-1-logang@deltatee.com?part=1