Re: [PATCH] workqueue: fix devm_alloc_workqueue() va_list misuse

[Breno Leitao <leitao@debian.org>]

On Mon, Apr 27, 2026 at 08:51:22PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 27, 2026 at 09:47:44AM -0700, Breno Leitao wrote:
> > devm_alloc_workqueue() builds a va_list from its own variadic
> > arguments and then passes that va_list as a single positional
> > argument to the variadic alloc_workqueue() macro:
> > 
> > 	va_start(args, max_active);
> > 	wq = alloc_workqueue(fmt, flags, max_active, args);
> > 	va_end(args);
> > 
> > alloc_workqueue() expands to alloc_workqueue_noprof(), which
> > performs its own va_start() over its ... parameters. The inner
> > vsnprintf(wq->name, sizeof(wq->name), fmt, args) inside
> > __alloc_workqueue() therefore receives the outer va_list object
> > as the first variadic slot, not the caller's actual format
> > arguments. On x86-64 SysV a "%s" conversion dereferences the
> > pointer inside that va_list as a char * and copies arbitrary
> > stack bytes from devm_alloc_workqueue()'s register-save area
> > into wq->name. The write is bounded by WQ_NAME_LEN but the
> > read is not, so this is an out-of-bounds stack read whose result
> > also surfaces via show_all_workqueues(), print_worker_info(),
> > the WQ_NAME_LEN pr_warn_once(), and lockdep lock names.
> > 
> > C does not allow forwarding a va_list through a ... parameter;
> > a v-style entry point is required. __alloc_workqueue() is
> > exactly that and is already used by alloc_workqueue_noprof().
> > Split devm_alloc_workqueue() into devm_alloc_workqueue_noprof()
> > plus an alloc_hooks() macro wrapper, mirroring the
> > alloc_workqueue / alloc_workqueue_noprof split. The _noprof
> > function calls __alloc_workqueue() directly with the va_list we
> > already hold, and runs wq_init_lockdep(wq) afterwards to mirror
> > alloc_workqueue_noprof() (otherwise wq->lockdep_map stays NULL
> > and __flush_workqueue()'s on-stack
> > COMPLETION_INITIALIZER_ONSTACK_MAP would NULL-deref it).
> > 
> > Keeping the alloc_hooks() wrapper preserves memory allocation
> > profiling attribution under CONFIG_MEM_ALLOC_PROFILING=y: the
> > kzalloc_noprof() and alloc_workqueue_attrs_noprof() inside
> > __alloc_workqueue() are charged to the driver call site rather
> > than to a single line in kernel/workqueue.c.
> > 
> > No caller changes are required. Drivers continue to write
> > devm_alloc_workqueue() and devm_alloc_ordered_workqueue() as
> > before; the new macro forwards through alloc_hooks() into the
> > renamed _noprof function transparently. Out-of-tree modules
> > need only be recompiled against the updated header to pick up
> > the renamed export, mirroring the precedent set by the existing
> > alloc_workqueue / alloc_workqueue_noprof split.
> > 
> > devm_alloc_ordered_workqueue() in include/linux/workqueue.h is
> > a macro that forwards to devm_alloc_workqueue(), so it inherits
> > this fix automatically. Two in-tree callers actively trigger
> > the broken path on every probe:
> > 
> >   drivers/power/supply/mt6370-charger.c:889
> >   drivers/power/supply/max77705_charger.c:649
> > 
> > both of which use devm_alloc_ordered_workqueue(dev, "%s", 0,
> > dev_name(dev)).
> > 
> > A standalone reproducer module is available at[1]:
> > 
> > Link: https://github.com/leitao/debug/blob/main/workqueue/valist/wq_va_test.c [1]
> 
> > 
> 
> There shouldn't be blank line(s) in the tag block.

Ack, I will remove it in the respin.

> 
> > Fixes: 1dfc9d60a69e ("workqueue: devres: Add device-managed allocate workqueue")
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Otherwise second Tejun's "nice catch"!