Re: [PATCH 5.4] selinux: fix race condition when computing ocontext SIDs

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Dec 13, 2021 at 11:56:51PM -0800, Vijay Balakrishna wrote:
> From: Ondrej Mosnacek <omosnace@redhat.com>
> 
> commit cbfcd13be5cb2a07868afe67520ed181956579a7 upstream.
> 
> Current code contains a lot of racy patterns when converting an
> ocontext's context structure to an SID. This is being done in a "lazy"
> fashion, such that the SID is looked up in the SID table only when it's
> first needed and then cached in the "sid" field of the ocontext
> structure. However, this is done without any locking or memory barriers
> and is thus unsafe.
> 
> Between commits 24ed7fdae669 ("selinux: use separate table for initial
> SID lookup") and 66f8e2f03c02 ("selinux: sidtab reverse lookup hash
> table"), this race condition lead to an actual observable bug, because a
> pointer to the shared sid field was passed directly to
> sidtab_context_to_sid(), which was using this location to also store an
> intermediate value, which could have been read by other threads and
> interpreted as an SID. In practice this caused e.g. new mounts to get a
> wrong (seemingly random) filesystem context, leading to strange denials.
> This bug has been spotted in the wild at least twice, see [1] and [2].
> 
> Fix the race condition by making all the racy functions use a common
> helper that ensures the ocontext::sid accesses are made safely using the
> appropriate SMP constructs.
> 
> Note that security_netif_sid() was populating the sid field of both
> contexts stored in the ocontext, but only the first one was actually
> used. The SELinux wiki's documentation on the "netifcon" policy
> statement [3] suggests that using only the first context is intentional.
> I kept only the handling of the first context here, as there is really
> no point in doing the SID lookup for the unused one.
> 
> I wasn't able to reproduce the bug mentioned above on any kernel that
> includes commit 66f8e2f03c02, even though it has been reported that the
> issue occurs with that commit, too, just less frequently. Thus, I wasn't
> able to verify that this patch fixes the issue, but it makes sense to
> avoid the race condition regardless.
> 
> [1] https://github.com/containers/container-selinux/issues/89
> [2] https://lists.fedoraproject.org/archives/list/selinux@lists.fedoraproject.org/thread/6DMTAMHIOAOEMUAVTULJD45JZU7IBAFM/
> [3] https://selinuxproject.org/page/NetworkStatements#netifcon
> 
> Cc: stable@vger.kernel.org
> Cc: Xinjie Zheng <xinjie@google.com>
> Reported-by: Sujithra Periasamy <sujithra@google.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> (cherry picked from commit cbfcd13be5cb2a07868afe67520ed181956579a7)
> [vijayb: Backport contextual differences are due to v5.10 RCU related
>  changes are not in 5.4]
> Signed-off-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
> ---
> 
> We have kernel crashes with stack traces related to selinux security
> context to sid in 5.4 --
> https://lore.kernel.org/all/af058f59-ce8a-7648-25e8-f8b8a2dbb0ba@linux.microsoft.com/#t
> Unfortunately we don't have a on-demand repro.  We are hoping this
> patch would help in addressing a possible race in 5.4.

Now queued up.

greg k-h