Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 15:03:38 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Jorge Merlino <jorge.merlino@canonical.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Eric Biederman <ebiederm@xmission.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Fix race condition when exec'ing setuid files
Message-ID: <202209131456.76A13BC5E4@keescook>
References: <20220910211215.140270-1-jorge.merlino@canonical.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220910211215.140270-1-jorge.merlino@canonical.com>
Precedence: bulk

On Sat, Sep 10, 2022 at 06:12:14PM -0300, Jorge Merlino wrote:
> This patch fixes a race condition in check_unsafe_exec when a heavily
> threaded program tries to exec a setuid file. check_unsafe_exec counts the
> number of threads sharing the same fs_struct and compares it to the total
> number of users of the fs_struct by looking at its users counter. If there
> are more users than process threads using it the setuid exec fails with
> LSM_UNSAFE_SHARE. The problem is that, during the kernel_clone code
> execution, the fs_struct users counter is incremented before the new thread
> is added to the thread_group list. So there is a race when the counter has
> been incremented but the thread is not visible to the while_each_tread loop
> in check_unsafe_exec.

Thanks for reporting this and for having a reproducer!

It looks like this is "failing safe", in the sense that the bug causes
an exec of a setuid binary to not actually change the euid. Is that an
accurate understanding here?

> This patch sort of fixes this by setting a process flag to the parent
> process during the time this race is possible. Thus, if a process is
> forking, it counts an extra user fo the fs_struct as the counter might be
> incremented before the thread is visible. But this is not great as this
> could generate the opposite problem as there may be an external process
> sharing the fs_struct that is masked by some thread that is being counted
> twice. I submit this patch just as an idea but mainly I want to introduce
> this issue and see if someone comes up with a better solution.

I'll want to spend some more time studying this race, but yes, it looks
like it should get fixed. I'm curious, though, how did you find this
problem? It seems quite unusual to have a high-load heavily threaded
process decide to exec.

-Kees

-- 
Kees Cook