| Message ID | 20230420150503.22227-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [1/6] selinux: do not leave dangling pointer behind | expand |
On Thu, Apr 20, 2023 at 11:05 AM Christian Göttsche <cgzones@googlemail.com> 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@googlemail.com> > --- > 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? 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.
On Mon, 8 May 2023 at 22:42, Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Apr 20, 2023 at 11:05 AM Christian Göttsche > <cgzones@googlemail.com> 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@googlemail.com> > > --- > > 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
diff --git a/security/selinux/ss/context.h b/security/selinux/ss/context.h index eda32c3d4c0a..44179977f434 100644 --- a/security/selinux/ss/context.h +++ b/security/selinux/ss/context.h @@ -167,6 +167,7 @@ static inline int context_cpy(struct context *dst, const struct context *src) rc = mls_context_cpy(dst, src); if (rc) { kfree(dst->str); + dst->str = NULL; return rc; } return 0;
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@googlemail.com> --- security/selinux/ss/context.h | 1 + 1 file changed, 1 insertion(+)