Re: [PATCH 0/2] proc: protect ptrace_may_access() with exec_update_lock

[Eric W. Biederman]

I have added a couple more people who might be interested.

Kees Cook because as you have structured this it is an exec problem.

Oleg Nesterov as he is knowledgable about ptrace.

Jann Horn <jannh@xxxxxxxxxx> writes:

> My understanding is that procfs is effectively maintained by the VFS
> maintainers (though scripts/get_maintainer.pl claims that there are
> no maintainers for procfs because the VFS entry only claims files
> directly in fs/, and the procfs entry has no maintainers listed on
> it).
>
> In procfs, most uses of ptrace_may_access() should use
> exec_update_lock to avoid TOCTOU issues with concurrent privileged
> execve() (like setuid binary execution).
>
> This series doesn't fix all the remaining issues in procfs, but it fixes
> the easy cases for now; I will probably follow up with fixes for the
> gnarlier cases later unless someone else wants to do that.
>
> I have checked that procfs files still work with these changes and that
> CONFIG_PROVE_LOCKING=y doesn't generate any warnings.
>
> (checkpatch complains about missing argument names in
> proc_op::proc_get_link, but that was already the case before my
> patch.)


I think I finally have my context paged back in so I can intelligently
say something about this series.

The scenario you are worried about is when exec gains privileges,
and we read through proc and authenticate with the old credentials
instead of the new credentials.

Question 1.

Assuming the executable is world readable (which they generally are)
is there anything that becomes accessible in that race that was
not already accessible?

Question 2.

How does this race compare to racing with setresuid?
Do we need to fix the setresuid case as well?

Question 3.
Do we care about the case when a privileged process calls a setuid
process and drops privileges?

Question 4.
Is it possible to use a seq_lock instead of reader writer semaphore?
Or is that only for non-sleeping readers?

There have been a number of nasty cases lurking in the background
involving seccomp filters, PTRACE_EVENT_EXIT, de_thread and the like.

Blocking locks, especially ones that get widely used, just scare me in
this area.  Being able to see that something happened between start and
finish and say -EAGAIN or retrying internally seems like it would be
much less prone to weirdness.

The ugly with PTRACE_EVENT_EXIT as I recall is that if ptrace stops one
of the threads (not the one calling exec) at PTRACE_EVENT_EXIT it can
block de_thread, which blocks the rest of exec.  But there is something
in there where the ptracer hangs waiting for the exec to complete.  So
everything just stalls.  The ptracer waiting for exec the exec waiting
for the ptracer.  SIGKILL can get you out of that mess last I looked.
Still it is an ugly mess.

Getting everything away from that mess is why we have exec_update_lock
instead of just cred_guard_mutex.


I would really appreciate hearing the scenarios you are aiming to fix
and how this fixes them.  There are enough races and special cases
I don't feel comfortable reading that we just need exec_update_lock
around ptrace_may_access.  It is not clear to me that is sufficient
to close the small races we are worried about here.

If I could trace through someone else's logic I could be convinced
and the next people to deal with the code could look at it and see
ah.  That is the detail that was missed when it has to be fixed again.

Eric


> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> ---
> Jann Horn (2):
>       proc: protect ptrace_may_access() with exec_update_lock (part 1)
>       proc: protect ptrace_may_access() with exec_update_lock (FD links)
>
>  fs/proc/array.c      |   6 ++
>  fs/proc/base.c       | 159 ++++++++++++++++++++++-----------------------------
>  fs/proc/fd.c         |  27 ++++-----
>  fs/proc/internal.h   |   2 +-
>  fs/proc/namespaces.c |  12 ++++
>  5 files changed, 97 insertions(+), 109 deletions(-)
> ---
> base-commit: 5200f5f493f79f14bbdc349e402a40dfb32f23c8
> change-id: 20260518-procfs-lockfix-part1-5dca2d95bc12
>
> --  
> Jann Horn <jannh@xxxxxxxxxx>