Re: [Xen-devel] On unions usage, specifically arch.{hvm,pv}

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.10.2019 02:38, Marek Marczykowski-Górecki  wrote:
> To be honest, I think unions are very scary from security point of view.
> It's quite easy to use a field that in given context have very different
> meaning and easily results in security issue. In the most cases,
> compiler can't help you here. And seeing "IOMMU: add missing HVM check"
> patch recently, fixing a but that was there for a year, I think my point
> is valid.
> There are multiple unions in the Xen code base, but I'd start with the
> one in x86's struct arch_domain: {pv,hvm}. In some cases (like the above
> _patched_ one), it is obvious that using arch.pv or arch.hvm in given
> context is valid. But in some it is very much not obvious, like usage of
> d->arch.hvm.dirty_vram in _sh_propagate()
> (xen/arch/x86/mm/shadow/multi.c) - at least one caller seems to deal
> also with PV vcpus
> (sh_page_fault->shadow_get_and_create_l1e->l2e_propagate_from_guest).
> Maybe I'm missing something, or maybe I've just found a bug, I don't
> know, that's my point. And this is after casual grep for arch.hvm and
> picking random file (took like 1 minute).

Yes, this is a (latent) issue; we're being saved here merely by the
fact that the field lives far enough into the structure that there's
no overlap with the PV side of the union, and hence the NULL checks
of the pointer guard the code paths de-referencing it from getting
entered for PV. I do have a patch to address this (in the context of
arranging to compile out the whole dirty-VRAM handling code for !HVM
builds) already.

> I propose to implement some measures to make similar bugs less likely.
> Some ideas:
> 1. Add asserts for guest type, if the check isn't visible in obvious
> place, near arch.pv / arch.hvm usage.
> 
> or maybe even better:
> 
> 2. Add wrappers (inline, #define, whatever) that perform the check
> before accessing those fields. And forbid accessing those (and maybe
> later others too?) unions directly, so it would be trivial to verify.
> There could be multiple wrappers for most commons code patterns. For
> example one for combined is_hvm_domain(d) &&
> some-check-on-arch.hvm-field. Or another one just adding an ASSERT() /
> BUG_ON().
> 
> Ideally such check should be part of a release build (IMO it's better to
> crash early with clear message, instead of crashing later in mysterious
> way or having privilege escalation bug - if that's the alternative). But
> having those checks just in debug build would be an improvement already.

Adding checks where context is not obvious is certainly a good idea.
As always we want to be as non-destructive as possible, i.e. avoid
crashing the host at least in release builds when some alternative
solution is viable. But just like for finding such issues, given the
state of things it's a matter of auditing all uses to find where
such checks may want adding, which someone would need to find time
for.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel