On Mon, 8 May 2023 at 22:42, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 20, 2023 at 11:05 AM Christian Göttsche
> <cgzones@xxxxxxxxxxxxxx> wrote:
> >
> > In case mls_context_cpy() fails due to OOM set the free'd pointer in
> > context_cpy() to NULL to avoid it potentially being dereferenced or
> > free'd again in future. Freeing a NULL pointer is well-defined and a
> > hard NULL dereference crash is at least not exploitable and should give
> > a workable stack trace.
> >
> > Fixes: 12b29f34558b ("selinux: support deferred mapping of contexts")
> > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> > ---
> > security/selinux/ss/context.h | 1 +
> > 1 file changed, 1 insertion(+)
>
> Merged into selinux/next.
>
> Did you actually run into a problem where the system
> crashed/panic'd/etc. due to this?
No, no actual crash, just came to mind while looking over the code.
My understanding of the sidtab code is only superficial, so maybe
Stephen can correct my analysis:
There are two callers of context_cpy(): sidtab_set_initial() and
sidtab_context_to_sid():
sidtab_set_initial() is only called in policydb_load_isids(). If
context_cpy() fails on `&isid->entry.context` `isid->set` is not set
to 1 and thus the cleanup via sidtab_destroy() called in
policydb_load_isids() causes no use-after-free, since sidtab_destroy()
calls sidtab_destroy_entry() only on `if (s->isids[i].set)`.
In sidtab_context_to_sid(), if context_cpy() failed, the sidtab count
remains unchanged, since `smp_store_release(&s->count, count + 1);` is
skipped. So simple lookups via sidtab_lookup() should not access the
zombie context since the count is checked. But I think
sidtab_destroy(), e.g via a policy load on the old policy,
unconditionally calls, via sidtab_destroy_tree(),
sidtab_destroy_entry() on all fixed sized array elements, which leads
to a call of context_destroy() leading to a double-free.
Even if this analysis is wrong the change seems to me a good
defence-in-depth measure, e.g. for future refactorings.
Also the cost of a pointer assigmnment in an OOM error branch seems negligible.
I'll probably send a patch to set `dst->len` to 0 as well, since both
members are coupled.
> I'll leave the fixes tag on this
> since it is pretty minor, but generally I think it is best to reserve
> the fixes tag for problems that can be triggered as a fixes tag
> generally results in a stable backport, regardless of it is marked for
> stable or not.
>
> --
> paul-moore.com
