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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2188 bytes --]

Hi all,

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).

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.

Thoughts?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel