[PATCH 0/1] wait: using uninitialized member of wait queue

[PATCH 0/1] wait: using uninitialized member of wait queue

[Evgeny Kuznetsov]

Hi,

Here is patch which fixes bug in /include/linux/wait.h file.
Member "flags" of "wait_queue_t" struct  is used in several places in
kernel code without beeing initialized. "flags" is used in bitwise
operations. If it contain dummy data then incorrect flags may be 
used later in code.

Thanks,
Best Regards,
Evgeny

Evgeny Kuznetsov (1):
  wait: using uninitialized member of wait queue

 include/linux/wait.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

[PATCH 1/1] wait: using uninitialized member of wait queue

[Evgeny Kuznetsov]

From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>

Member "flags" of "wait_queue_t" struct  is used in several places in
kernel code without beeing initialized. "flags" is used in bitwise operations.
If "flags" not initialized then unexpected behaviour may have place.
Incorrect flags maybe used later in code.
Struct "wait_queue_t" is initialized in function "init_wait()". But
"init_wait()" do not initialize "flag" member.
Added initialization of "wait_queue_t.flags" with zero value into "init_wait".

Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
---
 include/linux/wait.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 0836ccc..3efc9f3 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -614,6 +614,7 @@ int wake_bit_function(wait_queue_t *wait, unsigned mode, int sync, void *key);
 		(wait)->private = current;				\
 		(wait)->func = autoremove_wake_function;		\
 		INIT_LIST_HEAD(&(wait)->task_list);			\
+		(wait)->flags = 0;					\
 	} while (0)
 
 /**
-- 
1.6.3.3

Re: [PATCH 1/1] wait: using uninitialized member of wait queue

[Michał Nazarewicz]

On Tue, 05 Oct 2010 10:47:57 +0200, Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com> wrote:
> Member "flags" of "wait_queue_t" struct  is used in several places in
> kernel code without beeing initialized. "flags" is used in bitwise operations.
                       ^^^^^^ -- "being"

> If "flags" not initialized then unexpected behaviour may have place.
> Incorrect flags maybe used later in code.
> Struct "wait_queue_t" is initialized in function "init_wait()". But
> "init_wait()" do not initialize "flag" member.
                 ^^ -- does         ^^^^ -- "flags"

> Added initialization of "wait_queue_t.flags" with zero value into "init_wait".

> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 0836ccc..3efc9f3 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -614,6 +614,7 @@ int wake_bit_function(wait_queue_t *wait, unsigned mode, int sync, void *key);
>  		(wait)->private = current;				\
>  		(wait)->func = autoremove_wake_function;		\
>  		INIT_LIST_HEAD(&(wait)->task_list);			\
> +		(wait)->flags = 0;					\
>  	} while (0)
> /**

I'd turn init_wait() into a static inline.  Otherwise looks good to me.
(Interestingly, init_wait() is used only in 3 places in the kernel and
none uses flags.)

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 1/1] wait: using uninitialized member of wait queue

[Evgeny Kuznetsov]

Hi,

'wait_queue_t' is passed to prepare_to_wait() function where 'flags' is
used, e.g:
File: /mm/mempool.c
void * mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
{       .....
         .....
	init_wait(&wait);
	prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE);
	.....
}

Best Regards,
Evgeny

On Tue, 2010-10-05 at 12:41 +0200, ext Michał Nazarewicz wrote:
> On Tue, 05 Oct 2010 10:47:57 +0200, Evgeny Kuznetsov
> <EXT-Eugeny.Kuznetsov@nokia.com> wrote:
> > Member "flags" of "wait_queue_t" struct  is used in several places
> in
> > kernel code without beeing initialized. "flags" is used in bitwise
> operations.
>                        ^^^^^^ -- "being"
> 
> > If "flags" not initialized then unexpected behaviour may have place.
> > Incorrect flags maybe used later in code.
> > Struct "wait_queue_t" is initialized in function "init_wait()". But
> > "init_wait()" do not initialize "flag" member.
>                  ^^ -- does         ^^^^ -- "flags"
> 
> > Added initialization of "wait_queue_t.flags" with zero value into
> "init_wait".
> 
> > diff --git a/include/linux/wait.h b/include/linux/wait.h
> > index 0836ccc..3efc9f3 100644
> > --- a/include/linux/wait.h
> > +++ b/include/linux/wait.h
> > @@ -614,6 +614,7 @@ int wake_bit_function(wait_queue_t *wait,
> unsigned mode, int sync, void *key);
> >               (wait)->private =
> current;                              \
> >               (wait)->func =
> autoremove_wake_function;                \
> >               INIT_LIST_HEAD(&(wait)->task_list);                     \
> > +             (wait)->flags =
> 0;                                      \
> >       } while (0)
> > /**
> 
> I'd turn init_wait() into a static inline.  Otherwise looks good to
> me.
> (Interestingly, init_wait() is used only in 3 places in the kernel and
> none uses flags.)
> 
> -- 
> Best regards,                                        _     _
> | Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
> | Computer Science,  Michał "mina86" Nazarewicz       (o o)
> +----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo-- 

Re: [PATCH 1/1] wait: using uninitialized member of wait queue

[Michał Nazarewicz]

> On Tue, 2010-10-05 at 12:41 +0200, ext Michał Nazarewicz wrote:
>> (Interestingly, init_wait() is used only in 3 places in the kernel and
>> none uses flags.)

On Tue, 05 Oct 2010 13:00:38 +0200, Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com> wrote:
> 'wait_queue_t' is passed to prepare_to_wait() function where 'flags' is
> used, e.g:
> File: /mm/mempool.c
> void * mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> {       .....
>          .....
> 	init_wait(&wait);
> 	prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE);
> 	.....
> }

I meant that none of the callers initialized flags.  I was initially concerned
whether some caller that used init_wait() could set flags and expect that they
won't be changed by the call to init_wait() -- this turned out not to be the
case.

So essentially, I'm supporting your point.  Sorry about the confusion.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 0/1] wait: using uninitialized member of wait queue

[Linus Torvalds]

On Tue, Oct 5, 2010 at 1:47 AM, Evgeny Kuznetsov
<EXT-Eugeny.Kuznetsov@nokia.com> wrote:
>
> Here is patch which fixes bug in /include/linux/wait.h file.
> Member "flags" of "wait_queue_t" struct  is used in several places in
> kernel code without beeing initialized. "flags" is used in bitwise
> operations. If it contain dummy data then incorrect flags may be
> used later in code.

The patch looks fine, but do you have any case of this actually
causing problems? Afaik, we do end up initializing the part of the
flags field we care about (WQ_FLAG_EXCLUSIVE) in prepare_to_wait()
(and add_wait_queue()), so it looks - from an admittedly very cursory
read - as if this is a good cleanup but shouldn't actually be the
cause of any actual bugs.

So I'll apply it, but I just wondered if you had actually seen any
semantic changes from it?

               Linus

Re: [PATCH 0/1] wait: using uninitialized member of wait queue

[Evgeny Kuznetsov]

On Tue, 2010-10-05 at 08:43 -0700, ext Linus Torvalds wrote:
> On Tue, Oct 5, 2010 at 1:47 AM, Evgeny Kuznetsov
> <EXT-Eugeny.Kuznetsov@nokia.com> wrote:
> >
> > Here is patch which fixes bug in /include/linux/wait.h file.
> > Member "flags" of "wait_queue_t" struct  is used in several places in
> > kernel code without beeing initialized. "flags" is used in bitwise
> > operations. If it contain dummy data then incorrect flags may be
> > used later in code.
> 
> The patch looks fine, but do you have any case of this actually
> causing problems? Afaik, we do end up initializing the part of the
> flags field we care about (WQ_FLAG_EXCLUSIVE) in prepare_to_wait()
> (and add_wait_queue()), so it looks - from an admittedly very cursory
> read - as if this is a good cleanup but shouldn't actually be the
> cause of any actual bugs.
> 
> So I'll apply it, but I just wondered if you had actually seen any
> semantic changes from it?
> 
>                Linus
I did not see any problems caused by uninitialized flag. But this is
potential unsafe place (in case of future changes).
Also there is initialization of all members but not 'flags' in
init_wait() function. It is logical to initialize all data members.

Thanks,
Regards,
Evgeny