This is an attempt to do a 'post-mortem' on XSA-283, to find out how
the error came about, and consider what changes we could make to code
/ processes to prevent such errors from happening in the future. The
Security Team hopes to make it a habit to perform such analyses in the
future.
As it happened, there XSA-283 was not a security issue; but this was due
to an accident of the implementation; a change to the implementation may
have become a security issue at any point.
This approach is very much a work in progress; and this is my own work
done without consultation from the rest of the XenProject Security
Team. Feedback / improvements welcome.
# XSA-283: The issue
Interrupt remapping maps a multi-page datastructure shared between
hardware and the OS.
The datastructrue must be page-aligned, meaning that the lower 12 bits
of the address of the structure would always be 0. The hardware
register pointing to this (DMAR_IRTRA_REG) uses these "extra" bits to
store other flags.
This address is also stored in a per-iommu-domain data structrue
(struct ir_ctrl->iremap_maddr); but the extra hardware bits were
*also* stored in this data structure, in a way that meant "garbage"
bits were stored in the offset. It was only an accident of the way
the calculation was handled that avoided this resulting in accesses
past the end of the buffer.
# Archaeology
GET_IREMAP_ENTRY() was introduced by d210b9fc623 in 2009. This mainly
refactored a number of open-coded versions of the same algorithm which
already existed.
At that point, ir_ctrl->iremap_maddr was already being filled with the
contents of DMAR_IRTA_REG, rather than actually being a machine
address.
Various other changesets affecting lines or-ing in the values:
cda2f6c7613 2009-09-07 // Just slightly changes condition for calculation
1e9712f180a 2008-10-17 // Replaces magic constants with macros
12697426971 2008-04-10 // This is the culprit
Changeset 12697426971, in 2008, changes the structure from holding a
long-term pointer allocated from the xenheap, and adding "extra"
values into a local variable, to holding an maddr allocated from
domheap, and adding "extra" values into the maddr stored in the
structure.
It was posted to xen-devel in a thread starting with message-id
<08df4d958216244799fc84f3514d70f0013cd...@pdsmsx415.ccr.corp.intel.com> .
# Analysis
There was no public review of this patch; the commit contains no R-b
or A-b. Keir's only comment on the patch before committing it was to
note that it conflicted with a different patch and ask for a rebase.
Fundamentally, both the name of the new structure element
("iremap_maddr") and the name of the local variable it was replacing
("paddr") were wrong. "addr" should imply *only* an address; to
contain other information it should have been named "ctrl" instead.
Furthermore, map_vtd_domain_page() silently discards non-aligned bits
of addressed passed to it.
# Potential changes that could have prevented this
This could have been caught with a more careful review.
We could consider adding an entry to the style guide speciying that
names based on the word 'address' must *only* be an address, and must
not contain other extraneous information; and if such information
needs to be stored to reflect the contents of a hardware register,
then the name should be based on the word 'register' instead.
If map_vtd_domain_page() ASSERT()ed that addresses passed to it were
page-aligned, then this issue would have been caught probably before
the patch was sent.
A more rigorous change could be to create a special type to store
addresses, with access macros to set or get the address, which would
make it difficult to do this sort of casual or'ing.
# Recommendations
Look into ASSERTing that map_vtd_domain_page() is passed a
page-aligned address.
Consider adding a style recommendation in line with the address /
register discussion above.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
