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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- 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