linux-kernel - Re: [PATCH] workqueue: fix null-ptr-deref on __alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zr69QOysEfYXkMwb@DUT025-TGLU.fm.intel.com>
Date: Fri, 16 Aug 2024 02:45:20 +0000
From: Matthew Brost <matthew.brost@...el.com>
To: Sergey Senozhatsky <senozhatsky@...omium.org>
CC: Tejun Heo <tj@...nel.org>, Lai Jiangshan <jiangshanlai@...il.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] workqueue: fix null-ptr-deref on __alloc_workqueue()
 error

On Fri, Aug 16, 2024 at 11:38:31AM +0900, Sergey Senozhatsky wrote:
> Hi Matthew,
> 
> On (24/08/15 16:24), Matthew Brost wrote:
> [..]
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index 8ccbf510880b..5e818eae092d 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -534,7 +534,7 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> >   * @fmt: printf format for the name of the workqueue
> >   * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
> >   * @lockdep_map: user-defined lockdep_map
> > - * @args: args for @fmt
> > + * @...: args for @fmt
> >   *
> >   * Same as alloc_ordered_workqueue but with the a user-define lockdep_map.
> >   * Useful for workqueues created with the same purpose and to avoid leaking a
> > @@ -543,20 +543,9 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> >   * RETURNS:
> >   * Pointer to the allocated workqueue on success, %NULL on failure.
> >   */
> > -__printf(1, 4) static inline struct workqueue_struct *
> > -alloc_ordered_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> > -                                   struct lockdep_map *lockdep_map, ...)
> > -{
> > -       struct workqueue_struct *wq;
> > -       va_list args;
> > -
> > -       va_start(args, lockdep_map);
> > -       wq = alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | flags,
> > -                                        1, lockdep_map, args);
> > -       va_end(args);
> > +#define alloc_ordered_workqueue_lockdep_map(fmt, flags, lockdep_map, args...)  \
> > +       alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, lockdep_map, ##args)
> > 
> > -       return wq;
> > -}
> >  #endif
> 
> Oh, I haven't checked the workqueue header.  Yes, you are right.
> A macro should work.
> 

To be clear we your change to __alloc_workqueue in workqueue.c AND my
change to a macro in workqueue.h. In both cases a call chain of
multiple va_start / va_end happens which from what I read up on is not
allowed or undefined behavior depending on the compiler / platform
target.

> 
> Tejun, how do you plan to handle this?  Would it be possible to
> drop current series from your tree so that Matthew can send an
> updated version (with all the fixes squashed)?

Tejun, yes let me know how to move forward with this as it is highly
desired for Intel Xe team to get this into 6.12.

Matt