futex question

>
>(Cc:-ed more futex folks.)
>

That is great but I am wondering, what's your opinion on this Ingo?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

> Should we not just clear the pointer (and
>> it's compat version) within do_execve()?


In our private repository, applying the following patch resolved the
issues I mentioned. I no longer see messages like this:

[futex] ("ifconfig")(pid=2509) exit_robust_list:unable to fetch robust
entry. uaddr=0x000000002abbc4f0

from my instrumented kernel within exit_robust_list(). My
instrumentation looked something like this:

if (fetch_robust_entry(...)) {
printk(...);
return;
}

Just tossing the patch in the community in case someone is interested
....



Signed-off-by: Anirban Sinha <asinha DeleteThis @zeugmasystems.com>
---
fs/compat.c | 3 +++
fs/exec.c | 3 +++
2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 6d6f98f..c3d117c 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1510,6 +1510,9 @@ int compat_do_execve(char * filename,
if (retval)
goto out_file;

+#ifdef CONFIG_FUTEX
+ current->compat_robust_list = NULL;
+#endif
bprm->argc = compat_count(argv, MAX_ARG_STRINGS);
if ((retval = bprm->argc) < 0)
goto out;
diff --git a/fs/exec.c b/fs/exec.c
index 172ceb6..e9334b8 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1323,6 +1323,9 @@ int do_execve(char * filename,
retval = bprm_mm_init(bprm);
if (retval)
goto out_file;
+#ifdef CONFIG_FUTEX
+ current->robust_list = NULL;
+#endif

bprm->argc = count(argv, MAX_ARG_STRINGS);
if ((retval = bprm->argc) < 0)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Anirban Sinha wrote:
>> Should we not just clear the pointer (and
>>> it's compat version) within do_execve()?
>
>
> In our private repository, applying the following patch resolved the
> issues I mentioned. I no longer see messages like this:
>
> [futex] ("ifconfig")(pid=2509) exit_robust_list:unable to fetch robust
> entry. uaddr=0x000000002abbc4f0
>
> from my instrumented kernel within exit_robust_list(). My
> instrumentation looked something like this:

>
> if (fetch_robust_entry(...)) {
> printk(...);
> return;
> }
>
> Just tossing the patch in the community in case someone is interested


Thanks for sending the patch. I'm looking into it now. Couple questions:

1) What caused you to instrument this path in the first place? Were you
seeing some unexpected behavior?

2) I wonder why we would need to clear the robust list, but I don't see
other things like pi_blocked_on, etc. in execve being cleared. I'm
looking into this now (perhaps we don't do the same cleanup, need to
check).... have to get on the plane...

--
Darren Hart

> ...
>
>
>
> Signed-off-by: Anirban Sinha <asinha.RemoveThis@zeugmasystems.com>
> ---
> fs/compat.c | 3 +++
> fs/exec.c | 3 +++
> 2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/fs/compat.c b/fs/compat.c
> index 6d6f98f..c3d117c 100644
> --- a/fs/compat.c
> +++ b/fs/compat.c
> @@ -1510,6 +1510,9 @@ int compat_do_execve(char * filename,
> if (retval)
> goto out_file;
>
> +#ifdef CONFIG_FUTEX
> + current->compat_robust_list = NULL;
> +#endif
> bprm->argc = compat_count(argv, MAX_ARG_STRINGS);
> if ((retval = bprm->argc) < 0)
> goto out;
> diff --git a/fs/exec.c b/fs/exec.c
> index 172ceb6..e9334b8 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1323,6 +1323,9 @@ int do_execve(char * filename,
> retval = bprm_mm_init(bprm);
> if (retval)
> goto out_file;
> +#ifdef CONFIG_FUTEX
> + current->robust_list = NULL;
> +#endif
>
> bprm->argc = count(argv, MAX_ARG_STRINGS);
> if ((retval = bprm->argc) < 0)
>
>


--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

>
>
>Thanks for sending the patch. I'm looking into it now. Couple
questions:
>
>1) What caused you to instrument this path in the first place? Were
you
>seeing some unexpected behavior?

Basically, all this started as a means to aid debug or at least isolate
cases of memory corruption. When a process holding a futex died, the
robust futex cleanup operation can be foiled if there are any memory
corruptions in the user land. The "carefully inspecting the user land
linked list" part would bail out silently. So no process would get
EOWNERDEAD and wake up. So we decided to add printks so that we can
track these silent return cases.

We thought that actual number of cases of silently bailing out would be
rare so we did not expect any of those logs in the kernel buffer under
regular circumstances. To our surprise, we found lots of those logs!
This puzzled us. I looked at the code again and again but it deed some
seem to have any issues. Then it occurred to us (kaz) that an execve()
call can also cause invalid pointer values to remain in the task
structure. I did some testing and it seemed to indicate that this was
indeed the case.

There is a discussion on this by Kaz on the linux mips mailing list:

http://www.linux-mips.org/archives/linux-mips/2009-09/msg00130.html



>
>2) I wonder why we would need to clear the robust list, but I don't see
>other things like pi_blocked_on, etc. in execve being cleared.

Yea, may be. We don't use pi-futexes, so not too much confident of the
pi futex codebase there. You can look into those too.


I'm
>looking into this now (perhaps we don't do the same cleanup, need to
>check).... have to get on the plane...

Have a good trip and thanks for looking into it.

Ani

>>
>>
>> Signed-off-by: Anirban Sinha <asinha.TakeThisOut@zeugmasystems.com>
>> ---
>> fs/compat.c | 3 +++
>> fs/exec.c | 3 +++
>> 2 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/compat.c b/fs/compat.c
>> index 6d6f98f..c3d117c 100644
>> --- a/fs/compat.c
>> +++ b/fs/compat.c
>> @@ -1510,6 +1510,9 @@ int compat_do_execve(char * filename,
>> if (retval)
>> goto out_file;
>>
>> +#ifdef CONFIG_FUTEX
>> + current->compat_robust_list = NULL;
>> +#endif
>> bprm->argc = compat_count(argv, MAX_ARG_STRINGS);
>> if ((retval = bprm->argc) < 0)
>> goto out;
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 172ceb6..e9334b8 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1323,6 +1323,9 @@ int do_execve(char * filename,
>> retval = bprm_mm_init(bprm);
>> if (retval)
>> goto out_file;
>> +#ifdef CONFIG_FUTEX
>> + current->robust_list = NULL;
>> +#endif
>>
>> bprm->argc = count(argv, MAX_ARG_STRINGS);
>> if ((retval = bprm->argc) < 0)
>>
>>
>
>
>--
>Darren Hart
>IBM Linux Technology Center
>Real-Time Linux Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Anirban Sinha a écrit :
>>
>> Thanks for sending the patch. I'm looking into it now. Couple
> questions:
>> 1) What caused you to instrument this path in the first place? Were
> you
>> seeing some unexpected behavior?
>
> Basically, all this started as a means to aid debug or at least isolate
> cases of memory corruption. When a process holding a futex died, the
> robust futex cleanup operation can be foiled if there are any memory
> corruptions in the user land. The "carefully inspecting the user land
> linked list" part would bail out silently. So no process would get
> EOWNERDEAD and wake up. So we decided to add printks so that we can
> track these silent return cases.
>
> We thought that actual number of cases of silently bailing out would be
> rare so we did not expect any of those logs in the kernel buffer under
> regular circumstances. To our surprise, we found lots of those logs!
> This puzzled us. I looked at the code again and again but it deed some
> seem to have any issues. Then it occurred to us (kaz) that an execve()
> call can also cause invalid pointer values to remain in the task
> structure. I did some testing and it seemed to indicate that this was
> indeed the case.
>
> There is a discussion on this by Kaz on the linux mips mailing list:
>
> http://www.linux-mips.org/archives/linux-mips/2009-09/msg00130.html

This exactly looks like what I discovered a while ago about futex used
for pthread management. Anirban, this is a real security flaw and this
should be fixed as fast as possible

Commit 9c8a8228d0827e0d91d28527209988f672f97d28
author Eric Dumazet <eric.dumazet.RemoveThis@gmail.com>
Thu, 6 Aug 2009 22:09:28 +0000 (15:09 -0700)
execve: must clear current->clear_child_tid

While looking at Jens Rosenboom bug report
(http://lkml.org/lkml/2009/7/27/35) about strange sys_futex call done from
a dying "ps" program, we found following problem.

clone() syscall has special support for TID of created threads. This
support includes two features.

One (CLONE_CHILD_SETTID) is to set an integer into user memory with the
TID value.

One (CLONE_CHILD_CLEARTID) is to clear this same integer once the created
thread dies.

The integer location is a user provided pointer, provided at clone()
time.

kernel keeps this pointer value into current->clear_child_tid.

At execve() time, we should make sure kernel doesnt keep this user
provided pointer, as full user memory is replaced by a new one.

As glibc fork() actually uses clone() syscall with CLONE_CHILD_SETTID and
CLONE_CHILD_CLEARTID set, chances are high that we might corrupt user
memory in forked processes.

Following sequence could happen:

1) bash (or any program) starts a new process, by a fork() call that
glibc maps to a clone( ... CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID
...) syscall

2) When new process starts, its current->clear_child_tid is set to a
location that has a meaning only in bash (or initial program) context
(&THREAD_SELF->tid)

3) This new process does the execve() syscall to start a new program.
current->clear_child_tid is left unchanged (a non NULL value)

4) If this new program creates some threads, and initial thread exits,
kernel will attempt to clear the integer pointed by
current->clear_child_tid from mm_release() :

if (tsk->clear_child_tid
&& !(tsk->flags & PF_SIGNALED)
&& atomic_read(&mm->mm_users) > 1) {
u32 __user * tidptr = tsk->clear_child_tid;
tsk->clear_child_tid = NULL;

/*
* We don't check the error code - if userspace has
* not set up a proper pointer then tough luck.
*/
<< here >> put_user(0, tidptr);
sys_futex(tidptr, FUTEX_WAKE, 1, NULL, NULL, 0);
}

5) OR : if new program is not multi-threaded, but spied by /proc/pid
users (ps command for example), mm_users > 1, and the exiting program
could corrupt 4 bytes in a persistent memory area (shm or memory mapped
file)

If current->clear_child_tid points to a writeable portion of memory of the
new program, kernel happily and silently corrupts 4 bytes of memory, with
unexpected effects.

Fix is straightforward and should not break any sane program.

Reported-by: Jens Rosenboom <jens.RemoveThis@mcbone.net>
Acked-by: Linus Torvalds <torvalds.RemoveThis@linux-foundation.org>
Signed-off-by: Eric Dumazet <eric.dumazet.RemoveThis@gmail.com>
Signed-off-by: Oleg Nesterov <oleg.RemoveThis@redhat.com>
Cc: Peter Zijlstra <peterz.RemoveThis@infradead.org>
Cc: Sonny Rao <sonnyrao.RemoveThis@us.ibm.com>
Cc: Ingo Molnar <mingo.RemoveThis@elte.hu>
Cc: Thomas Gleixner <tglx.RemoveThis@linutronix.de>
Cc: Ulrich Drepper <drepper.RemoveThis@redhat.com>
Cc: Oleg Nesterov <oleg.RemoveThis@redhat.com>
Cc: <stable.RemoveThis@kernel.org>
Signed-off-by: Andrew Morton <akpm.RemoveThis@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds.RemoveThis@linux-foundation.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, 2 Oct 2009, Darren Hart wrote:
> Anirban Sinha wrote:
> > > Should we not just clear the pointer (and
> > > > it's compat version) within do_execve()?
> >
> >
> > In our private repository, applying the following patch resolved the
> > issues I mentioned. I no longer see messages like this:
> >
> > [futex] ("ifconfig")(pid=2509) exit_robust_list:unable to fetch robust
> > entry. uaddr=0x000000002abbc4f0
> >
> > from my instrumented kernel within exit_robust_list(). My
> > instrumentation looked something like this:
>
> >
> > if (fetch_robust_entry(...)) {
> > printk(...);
> > return;
> > }
> >
> > Just tossing the patch in the community in case someone is interested
>
>
> Thanks for sending the patch. I'm looking into it now. Couple questions:
>
> 1) What caused you to instrument this path in the first place? Were you
> seeing some unexpected behavior?
>
> 2) I wonder why we would need to clear the robust list, but I don't see other
> things like pi_blocked_on, etc. in execve being cleared. I'm looking into
> this now (perhaps we don't do the same cleanup, need to check).... have to get
> on the plane...

Hmm, just setting the robust list pointer to NULL fixes the problem at
hand, but I wonder whether we need to call exit_robust_list() as
well.

Vs. pi_blocked_on: If that's != NULL then you are not executing that
code path because you hang in the scheduler waiting for the lock.

The interesting question is whether we need to call
exit_pi_state_list() to fix up held locks.

Thanks,

tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

>> 1) What caused you to instrument this path in the first place? Were you
>> seeing some unexpected behavior?
>>
>> 2) I wonder why we would need to clear the robust list, but I don't see other
>> things like pi_blocked_on, etc. in execve being cleared. I'm looking into
>> this now (perhaps we don't do the same cleanup, need to check).... have to get
>> on the plane...
>
> Hmm, just setting the robust list pointer to NULL fixes the problem at
> hand, but I wonder whether we need to call exit_robust_list() as
> well.

hmm. That is an interesting thought. But I wonder if acquiring a lock and then exec()ing in the critical section is a legal thing to do. It does not feel right. Currently, with or without my change, such a thing would indefinitely block other waiters on the same futex.

Ani
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sun, 4 Oct 2009, Anirban Sinha wrote:

> >> 1) What caused you to instrument this path in the first place? Were you
> >> seeing some unexpected behavior?
> >>
> >> 2) I wonder why we would need to clear the robust list, but I don't see other
> >> things like pi_blocked_on, etc. in execve being cleared. I'm looking into
> >> this now (perhaps we don't do the same cleanup, need to check).... have to get
> >> on the plane...
> >
> > Hmm, just setting the robust list pointer to NULL fixes the problem at
> > hand, but I wonder whether we need to call exit_robust_list() as
> > well.

> hmm. That is an interesting thought. But I wonder if acquiring a
> lock and then exec()ing in the critical section is a legal thing to

It's definitely legal. There is no law which forbids to do that.

> do. It does not feel right. Currently, with or without my change,
> such a thing would indefinitely block other waiters on the same
> futex.

Right. Which completely defeats the purpose of the robust list. Will
have a look tomorrow.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sun, 2009-10-04 at 18:59 +0200, Thomas Gleixner wrote:

> > do. It does not feel right. Currently, with or without my change,
> > such a thing would indefinitely block other waiters on the same
> > futex.
>
> Right. Which completely defeats the purpose of the robust list. Will
> have a look tomorrow.

Right, so mm_release() which is meant to destroy the old mm context
actually does exit_robust_list(), but the problem is that it does so on
the new mm, not the old one that got passed down to mm_release().

The other detail is that exit_robust_list() doesn't clear
current->robust_list.

The problem with the patch send my Ani is that it clears the robust
lists before the point of no return, so on a failing execve() we'd have
messed up the state.

Making exit_robust_list() deal with an mm that is not the current mm is
interesting indeed.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Peter,

On Mon, 5 Oct 2009, Peter Zijlstra wrote:

> On Sun, 2009-10-04 at 18:59 +0200, Thomas Gleixner wrote:
>
> > > do. It does not feel right. Currently, with or without my change,
> > > such a thing would indefinitely block other waiters on the same
> > > futex.
> >
> > Right. Which completely defeats the purpose of the robust list. Will
> > have a look tomorrow.
>
> Right, so mm_release() which is meant to destroy the old mm context
> actually does exit_robust_list(), but the problem is that it does so on
> the new mm, not the old one that got passed down to mm_release().
>
> The other detail is that exit_robust_list() doesn't clear
> current->robust_list.

I know.

> The problem with the patch send my Ani is that it clears the robust
> lists before the point of no return, so on a failing execve() we'd have
> messed up the state.

Right. We need to do that at the latest possible point.

Looking more into that I think we should check whether the robust list
has an entry (lock held) in do_execve() and return -EWOULDBLOCK to
luser space. Same if pi_waiters is not empty. Holding a lock and
calling execve() is simply broken.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Mon, 2009-10-05 at 12:56 +0200, Thomas Gleixner wrote:
>
> Looking more into that I think we should check whether the robust list
> has an entry (lock held) in do_execve() and return -EWOULDBLOCK to
> luser space. Same if pi_waiters is not empty. Holding a lock and
> calling execve() is simply broken.

Fine by me

something like the below?

The question is of course what Ani was doing that triggered this in the
first place and if he can live with this..

---
fs/exec.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index d49be6b..0812ba6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1295,6 +1295,22 @@ int do_execve(char * filename,
bool clear_in_exec;
int retval;

+ retval = -EWOULDBLOCK;
+#ifdef CONFIG_FUTEX
+ if (unlikely(current->robust_list))
+ goto out_ret;
+#ifdef CONFIG_COMPAT
+ if (unlikely(current->compat_robust_list))
+ goto out_ret;
+#endif
+ spin_lock_irq(&current->pi_lock);
+ if (!list_empty(&current->pi_state_list)) {
+ spin_unlock_irq(&current->pi_lock);
+ goto out_ret;
+ }
+ spin_unlock_irq(&current->pi_lock);
+#endif
+
retval = unshare_files(&displaced);
if (retval)
goto out_ret;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

* Peter Zijlstra <a.p.zijlstra RemoveThis @chello.nl> wrote:

> diff --git a/fs/exec.c b/fs/exec.c
> index d49be6b..0812ba6 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1295,6 +1295,22 @@ int do_execve(char * filename,
> bool clear_in_exec;
> int retval;
>
> + retval = -EWOULDBLOCK;
> +#ifdef CONFIG_FUTEX
> + if (unlikely(current->robust_list))
> + goto out_ret;
> +#ifdef CONFIG_COMPAT
> + if (unlikely(current->compat_robust_list))
> + goto out_ret;
> +#endif
> + spin_lock_irq(&current->pi_lock);
> + if (!list_empty(&current->pi_state_list)) {
> + spin_unlock_irq(&current->pi_lock);
> + goto out_ret;
> + }
> + spin_unlock_irq(&current->pi_lock);
> +#endif

i suspect this should have the form of:

retval = can_exec_robust_futexes();
if (retval)
goto out_ret

retval = unshare_files(&displaced);
if (retval)
goto out_ret;

...

but ... shouldnt we just do what exec normally does and zap any state
that shouldnt be carried over into the new context - instead of denying
the exec? Am i missing something?

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Mon, 5 Oct 2009, Ingo Molnar wrote:
> * Peter Zijlstra <a.p.zijlstra.DeleteThis@chello.nl> wrote:
>
> > diff --git a/fs/exec.c b/fs/exec.c
> > index d49be6b..0812ba6 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1295,6 +1295,22 @@ int do_execve(char * filename,
> > bool clear_in_exec;
> > int retval;
> >
> > + retval = -EWOULDBLOCK;
> > +#ifdef CONFIG_FUTEX
> > + if (unlikely(current->robust_list))
> > + goto out_ret;
> > +#ifdef CONFIG_COMPAT
> > + if (unlikely(current->compat_robust_list))
> > + goto out_ret;
> > +#endif
> > + spin_lock_irq(&current->pi_lock);
> > + if (!list_empty(&current->pi_state_list)) {
> > + spin_unlock_irq(&current->pi_lock);
> > + goto out_ret;
> > + }
> > + spin_unlock_irq(&current->pi_lock);
> > +#endif
>
> i suspect this should have the form of:
>
> retval = can_exec_robust_futexes();
> if (retval)
> goto out_ret

Yes.

> retval = unshare_files(&displaced);
> if (retval)
> goto out_ret;
>
> ...
>
> but ... shouldnt we just do what exec normally does and zap any state
> that shouldnt be carried over into the new context - instead of denying
> the exec? Am i missing something?

We want to check whether the robust list is empty. If it's not empty
then we deny the exec instead of silently releasing the futexes or
just ignoring the robust list entirely. Same applies for the pi
waiters list.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/