-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2024-45818 / XSA-463
version 2
Deadlock in x86 HVM standard VGA handling
UPDATES IN VERSION 2
====================
Public release.
ISSUE DESCRIPTION
=================
The hypervisor contains code to accelerate VGA memory accesses for HVM
guests, when the (virtual) VGA is in "standard" mode. Locking involved
there has an unusual discipline, leaving a lock acquired past the
return from the function that acquired it. This behavior results in a
problem when emulating an instruction with two memory accesses, both of
which touch VGA memory (plus some further constraints which aren't
relevant here). When emulating the 2nd access, the lock that is already
being held would be attempted to be re-acquired, resulting in a
deadlock.
This deadlock was already found when the code was first introduced, but
was analysed incorrectly and the fix was incomplete. Analysis in light
of the new finding cannot find a way to make the existing locking
discipline work.
In staging, this logic has all been removed because it was discovered
to be accidentally disabled since Xen 4.7. Therefore, we are fixing the
locking problem by backporting the removal of most of the feature. Note
that even with the feature disabled, the lock would still be acquired
for any accesses to the VGA MMIO region.
IMPACT
======
A (not necessarily malicious) HVM guest kernel can lock up the entire
host.
VULNERABLE SYSTEMS
==================
Xen versions 4.6 through 4.19 are vulnerable. Staging (4.20 dev) is
not vulnerable; as noted above, the functionality was already removed
prior to the discovery of this issue.
Only x86 systems running HVM guests are vulnerable. Architectures other
than x86 are not vulnerable.
Only HVM guests can leverage the vulnerability. PVH and PV guests
cannot leverage the vulnerability.
MITIGATION
==========
Running only PV or PVH guests will avoid this vulnerability.
CREDITS
=======
This issue was discovered by Manuel Andreas of Technical University of
Munich.
RESOLUTION
==========
Applying the appropriate set of attached patches resolves this issue.
Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball. Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.
xsa463/xsa463-4.19-??.patch Xen 4.19.x
xsa463/xsa463-4.18-??.patch Xen 4.18.x
xsa463/xsa463-4.17-??.patch Xen 4.17.x
xsa463/xsa463-4.16-??.patch Xen 4.16.x
$ sha256sum xsa463*/*
405655548529a52ced40b42341a7e991945ff5f7851709b60d85a1d1a03a0f7f xsa463/xsa463-4.16-01.patch
6bf659c0cec609f79101b237b6ac07274b4f08b20ac24d4b399732d6c3dbae3b xsa463/xsa463-4.16-02.patch
2ae4413ac1781506353a34b6a3b5836bc426ceae789696e9c1fe6274d67e073d xsa463/xsa463-4.16-03.patch
87d2515141f6900c2f20ed449cf86cc82928fcf2b0424c2faa9b8cfa49940447 xsa463/xsa463-4.16-04.patch
227bf1f048dfa389b04d8208f0003f77c006340d9258906ce21bf3f5fa226a67 xsa463/xsa463-4.16-05.patch
13d17ec4ce9c4844ddf33d2f034f354e9b9907425b98d242e0472021d3ae2c4a xsa463/xsa463-4.16-06.patch
ad0c25dcf768dfe88084f34f15e9b9bc91f32f3c8c281b172da673aadc568b54 xsa463/xsa463-4.16-07.patch
065395ea7b4eb9ca26c48a110ea03ceac243a4c9390e25e044d134408ddc88e0 xsa463/xsa463-4.16-08.patch
6c4ad589e979e200dee6ec060832489a841b91751801e42f63d8e7aa2f7bc1f5 xsa463/xsa463-4.16-09.patch
3ce5056d796811c930b6b6453a18fd285d50e871135b22dacbed845b9e2e6fd7 xsa463/xsa463-4.16-10.patch
f6be5907e34c9bdceafd56b413b26b9dea44faae6db49ee15c0866056880a615 xsa463/xsa463-4.17-01.patch
1907bf226db3f67be539ae70c581969011ee77e4161993e8e68aef66625f62c9 xsa463/xsa463-4.17-02.patch
6c51a337b50cba01b5255c12276ed63ab771ddb12a104e468c436328fab27ca6 xsa463/xsa463-4.17-03.patch
efc532d8382d8d765e43cfd72edc3d0d4555a3a9faa019f77ef85979f241c265 xsa463/xsa463-4.17-04.patch
3b7cf5ff13e7a6080b16d56fe713369cc027928f00dc85508593b549f2195e09 xsa463/xsa463-4.17-05.patch
a2f1000352d3f426bb6a07ffb0e9fb942975b49da1c6dd1109bce167acd37778 xsa463/xsa463-4.17-06.patch
544d02f57b7f2c4101a3ccd3f4fc5a68e168bcf7806f19677c1675f96d16ce34 xsa463/xsa463-4.17-07.patch
93d422d0e143884ffd51ecebd0bad08a75002e61b0d5af4b38ac0d190b29039a xsa463/xsa463-4.17-08.patch
759ea086c438f9fb7dedb0b60b5455b81d0452c693f88c270e3bd3911ea26a43 xsa463/xsa463-4.17-09.patch
94162a21fa97f041abf94a32b9999d98d16055b1f6e2e4230d123f4c8ef202fb xsa463/xsa463-4.17-10.patch
a19b7675c633f5d33fc611a1a258c2a1f579e170245cf021662b056c144ce6b1 xsa463/xsa463-4.18-01.patch
61db94f3e856593c1a58e1c32863ce4228ee2bedeac5f56ddd527b7fd4dd91ee xsa463/xsa463-4.18-02.patch
211df1a265f657d6a601fa56b592030b8db5856399531cf825f2e74f10e6f054 xsa463/xsa463-4.18-03.patch
f1be6433e2223acd67301b78978d24114adc5021a67ed0874e57d70e05c05fab xsa463/xsa463-4.18-04.patch
79e050d6d9ed2cf5e3dbaff035d5cc00b727ccb5ddfcda0cf8e475f799f980bd xsa463/xsa463-4.18-05.patch
823f7214aa1ad325b34da965fef48c6b3bf805ecf41532cdf0ccd283d1bf1708 xsa463/xsa463-4.18-06.patch
4cad1195998178c771f960d435d4d60c40dfc311a71e1d0a0be2bc651dd97076 xsa463/xsa463-4.18-07.patch
569e555814834c99b0316c1c664cf08d16807ef7d08520d7ed9c01c914dbe1dc xsa463/xsa463-4.18-08.patch
8c28ba35b79648fde1290f307e34a6594ec091f38e8ec4d11f07377aeac82149 xsa463/xsa463-4.18-09.patch
82b4a135fe27bf581996c334a5d3306b5aecb1a5dbefcc5cd1f151bdf1237f22 xsa463/xsa463-4.19-01.patch
b158d65fe3775b84192c205adda5f461c82a76f6c58aa03e0d1958062023a4ad xsa463/xsa463-4.19-02.patch
53135fc79a440a03141dab05f2d02e784dfed226d0faadc014b2b14fb1e6bbbe xsa463/xsa463-4.19-03.patch
e54ea9cbf82881d21c2b5b407803eeae948b9f804e5c643431b95dc5430be345 xsa463/xsa463-4.19-04.patch
65195bc7a52aa3582fd94010b95fc39979f1aad5ac961fe625c89573276b4b40 xsa463/xsa463-4.19-05.patch
cf8e589067ee08c628fd65ee3726546a27f39dc3098283346126f6745cc2aa9a xsa463/xsa463-4.19-06.patch
b13f01ea76a54dd6b2290afeaf6f6790892050588f128a276e408340afd6eb6a xsa463/xsa463-4.19-07.patch
dfd48e8f925ff487c6b8c2aaeec58442a9b9bdef98461e59191cd20a2094bad9 xsa463/xsa463-4.19-08.patch
8c28ba35b79648fde1290f307e34a6594ec091f38e8ec4d11f07377aeac82149 xsa463/xsa463-4.19-09.patch
$
DEPLOYMENT DURING EMBARGO
=========================
Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.
But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).
Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.
(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable. This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)
For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----
iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmczQ/UMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZA/oH/iNqbCCBE9lP+Wao3Zkzuo+krvVfd3f6am/jzp/J
DT39YZpRsZPLKGE3NKf12y88A0aFpVq2ZBE4efubx3tS1hJqf4AmQ2AOlHAAkFa4
d966jzX/8wFmuO0HnRgHI5Z3mADQEQL8TLeb8rAuPuqguusX7RoG4vvLNQmy/S9T
50CYU86AN9Of3jaUVZQp10N6O9fnV/D3X6rKWoBTOSGDMnYf4mh6FgPcntg9sKjQ
QGzlZXpEi3Bjs37vtaDi7cgQ8TDGJGVAsBC1d4IUufvlqYycjAgZN0ukOAmn9mMl
6/mDQdgGhRMJMvho57jGnaNQsjtovA7eBjtiU0tABeyh9og=
=KD+y
-----END PGP SIGNATURE-----
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "cache" struct member
Since 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
guests"), HVM guests are built using XEN_DOMCTL_sethvmcontext, which
ends up disabling stdvga caching because of arch_hvm_load() being
involved in the processing of the request. With that the field is
useless, and can be dropped. Drop the helper functions manipulating /
checking as well right away, but leave the use sites of
stdvga_cache_is_enabled() with the hard-coded result the function would
have produced, to aid validation of subsequent dropping of further code.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 53b7246bdfb3c280adcdf714918e4decb7e108f4)
diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
index 86c82cbd7456..ad2594f95b3f 100644
--- a/xen/arch/x86/hvm/save.c
+++ b/xen/arch/x86/hvm/save.c
@@ -77,9 +77,6 @@ int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
/* Time when restore started */
d->arch.hvm.sync_tsc = rdtsc();
- /* VGA state is not saved/restored, so we nobble the cache. */
- d->arch.hvm.stdvga.cache = STDVGA_CACHE_DISABLED;
-
return 0;
}
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index ab9781d82a55..b9d7b5a4d9ff 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -101,37 +101,6 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
unmap_domain_page(p);
}
-static void stdvga_try_cache_enable(struct hvm_hw_stdvga *s)
-{
- /*
- * Caching mode can only be enabled if the the cache has
- * never been used before. As soon as it is disabled, it will
- * become out-of-sync with the VGA device model and since no
- * mechanism exists to acquire current VRAM state from the
- * device model, re-enabling it would lead to stale data being
- * seen by the guest.
- */
- if ( s->cache != STDVGA_CACHE_UNINITIALIZED )
- return;
-
- gdprintk(XENLOG_INFO, "entering caching mode\n");
- s->cache = STDVGA_CACHE_ENABLED;
-}
-
-static void stdvga_cache_disable(struct hvm_hw_stdvga *s)
-{
- if ( s->cache != STDVGA_CACHE_ENABLED )
- return;
-
- gdprintk(XENLOG_INFO, "leaving caching mode\n");
- s->cache = STDVGA_CACHE_DISABLED;
-}
-
-static bool_t stdvga_cache_is_enabled(const struct hvm_hw_stdvga *s)
-{
- return s->cache == STDVGA_CACHE_ENABLED;
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -171,7 +140,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
if ( !prev_stdvga && s->stdvga )
{
gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- stdvga_try_cache_enable(s);
}
else if ( prev_stdvga && !s->stdvga )
{
@@ -468,7 +436,7 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
};
struct ioreq_server *srv;
- if ( !stdvga_cache_is_enabled(s) || !s->stdvga )
+ if ( true || !s->stdvga )
goto done;
/* Intercept mmio write */
@@ -536,18 +504,12 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
* We cannot return X86EMUL_UNHANDLEABLE on anything other then the
* first cycle of an I/O. So, since we cannot guarantee to always be
* able to send buffered writes, we have to reject any multi-cycle
- * I/O and, since we are rejecting an I/O, we must invalidate the
- * cache.
- * Single-cycle write transactions are accepted even if the cache is
- * not active since we can assert, when in stdvga mode, that writes
- * to VRAM have no side effect and thus we can try to buffer them.
+ * I/O.
*/
- stdvga_cache_disable(s);
-
goto reject;
}
else if ( p->dir == IOREQ_READ &&
- (!stdvga_cache_is_enabled(s) || !s->stdvga) )
+ (true || !s->stdvga) )
goto reject;
/* s->lock intentionally held */
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 54e0161b492c..d92f87f83bb7 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -121,19 +121,12 @@ struct vpci_arch_msix_entry {
int pirq;
};
-enum stdvga_cache_state {
- STDVGA_CACHE_UNINITIALIZED,
- STDVGA_CACHE_ENABLED,
- STDVGA_CACHE_DISABLED
-};
-
struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
bool_t stdvga;
- enum stdvga_cache_state cache;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: properly reject "indirect" VRAM writes
While ->count will only be different from 1 for "indirect" (data in
guest memory) accesses, it being 1 does not exclude the request being an
"indirect" one. Check both to be on the safe side, and bring the ->count
part also in line with what ioreq_send_buffered() actually refuses to
handle.
This is part of XSA-463 / CVE-2024-45818
Fixes: 3bbaaec09b1b ("x86/hvm: unify stdvga mmio intercept with standard mmio intercept")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit eb7cd0593d88c4b967a24bca8bd30591966676cd)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index b9d7b5a4d9ff..11f2a92d12b9 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -498,13 +498,13 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
spin_lock(&s->lock);
- if ( p->dir == IOREQ_WRITE && p->count > 1 )
+ if ( p->dir == IOREQ_WRITE && (p->data_is_ptr || p->count != 1) )
{
/*
* We cannot return X86EMUL_UNHANDLEABLE on anything other then the
* first cycle of an I/O. So, since we cannot guarantee to always be
* able to send buffered writes, we have to reject any multi-cycle
- * I/O.
+ * or "indirect" I/O.
*/
goto reject;
}
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "stdvga" struct member
Two of its consumers are dead (in compile-time constant conditionals)
and the only remaining ones are merely controlling debug logging. Hence
the field is now pointless to set, which in particular allows to get rid
of the questionable conditional from which the field's value was
established (afaict 551ceee97513 ["x86, hvm: stdvga cache always on"]
had dropped too much of the earlier extra check that was there, and
quite likely further checks were missing).
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b740a9369e81bdda675a9780130ce2b9e75d4ec9)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 11f2a92d12b9..a5f1cff017ed 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -104,7 +104,7 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1, prev_stdvga = s->stdvga;
+ int rc = 1;
switch ( addr )
{
@@ -133,19 +133,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
break;
}
- /* When in standard vga mode, emulate here all writes to the vram buffer
- * so we can immediately satisfy reads without waiting for qemu. */
- s->stdvga = (s->sr[7] == 0x00);
-
- if ( !prev_stdvga && s->stdvga )
- {
- gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- }
- else if ( prev_stdvga && !s->stdvga )
- {
- gdprintk(XENLOG_INFO, "leaving stdvga mode\n");
- }
-
return rc;
}
@@ -425,7 +412,6 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
uint64_t addr, uint32_t size,
uint64_t data)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = addr,
@@ -436,8 +422,7 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
};
struct ioreq_server *srv;
- if ( true || !s->stdvga )
- goto done;
+ goto done;
/* Intercept mmio write */
switch ( size )
@@ -498,19 +483,14 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
spin_lock(&s->lock);
- if ( p->dir == IOREQ_WRITE && (p->data_is_ptr || p->count != 1) )
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
- * We cannot return X86EMUL_UNHANDLEABLE on anything other then the
- * first cycle of an I/O. So, since we cannot guarantee to always be
- * able to send buffered writes, we have to reject any multi-cycle
- * or "indirect" I/O.
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
*/
goto reject;
}
- else if ( p->dir == IOREQ_READ &&
- (true || !s->stdvga) )
- goto reject;
/* s->lock intentionally held */
return 1;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index d92f87f83bb7..d964f8efdfe2 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -126,7 +126,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- bool_t stdvga;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: remove unused MMIO handling code
All read accesses are rejected by the ->accept handler, while writes
bypass the bulk of the function body. Drop the dead code, leaving an
assertion in the read handler.
A number of other static items (and a macro) are then unreferenced and
hence also need (want) dropping. The same applies to the "latch" field
of the state structure.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 89108547af1f230b72893b48351f9c1106189649)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index a5f1cff017ed..138ced229d38 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -38,26 +38,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-#define PAT(x) (x)
-static const uint32_t mask16[16] = {
- PAT(0x00000000),
- PAT(0x000000ff),
- PAT(0x0000ff00),
- PAT(0x0000ffff),
- PAT(0x00ff0000),
- PAT(0x00ff00ff),
- PAT(0x00ffff00),
- PAT(0x00ffffff),
- PAT(0xff000000),
- PAT(0xff0000ff),
- PAT(0xff00ff00),
- PAT(0xff00ffff),
- PAT(0xffff0000),
- PAT(0xffff00ff),
- PAT(0xffffff00),
- PAT(0xffffffff),
-};
-
/* force some bits to zero */
static const uint8_t sr_mask[8] = {
(uint8_t)~0xfc,
@@ -82,25 +62,6 @@ static const uint8_t gr_mask[9] = {
(uint8_t)~0x00, /* 0x08 */
};
-static uint8_t *vram_getb(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 12) & 0x3f];
- uint8_t *p = __map_domain_page(pg);
- return &p[a & 0xfff];
-}
-
-static uint32_t *vram_getl(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 10) & 0x3f];
- uint32_t *p = __map_domain_page(pg);
- return &p[a & 0x3ff];
-}
-
-static void vram_put(struct hvm_hw_stdvga *s, void *p)
-{
- unmap_domain_page(p);
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -169,243 +130,12 @@ static int stdvga_intercept_pio(
return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
}
-static unsigned int stdvga_mem_offset(
- struct hvm_hw_stdvga *s, unsigned int mmio_addr)
-{
- unsigned int memory_map_mode = (s->gr[6] >> 2) & 3;
- unsigned int offset = mmio_addr & 0x1ffff;
-
- switch ( memory_map_mode )
- {
- case 0:
- break;
- case 1:
- if ( offset >= 0x10000 )
- goto fail;
- offset += 0; /* assume bank_offset == 0; */
- break;
- case 2:
- offset -= 0x10000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- default:
- case 3:
- offset -= 0x18000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- }
-
- return offset;
-
- fail:
- return ~0u;
-}
-
-#define GET_PLANE(data, p) (((data) >> ((p) * 8)) & 0xff)
-
-static uint8_t stdvga_mem_readb(uint64_t addr)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane;
- uint32_t ret, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return 0xff;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- vram_b = vram_getb(s, addr);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- vram_b = vram_getb(s, ((addr & ~1) << 1) | plane);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else
- {
- /* standard VGA latched access */
- vram_l = vram_getl(s, addr);
- s->latch = *vram_l;
- vram_put(s, vram_l);
-
- if ( !(s->gr[5] & 0x08) )
- {
- /* read mode 0 */
- plane = s->gr[4];
- ret = GET_PLANE(s->latch, plane);
- }
- else
- {
- /* read mode 1 */
- ret = (s->latch ^ mask16[s->gr[2]]) & mask16[s->gr[7]];
- ret |= ret >> 16;
- ret |= ret >> 8;
- ret = (~ret) & 0xff;
- }
- }
-
- return ret;
-}
-
static int stdvga_mem_read(const struct hvm_io_handler *handler,
uint64_t addr, uint32_t size, uint64_t *p_data)
{
- uint64_t data = ~0ul;
-
- switch ( size )
- {
- case 1:
- data = stdvga_mem_readb(addr);
- break;
-
- case 2:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- break;
-
- case 4:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- data |= stdvga_mem_readb(addr + 2) << 16;
- data |= (uint32_t)stdvga_mem_readb(addr + 3) << 24;
- break;
-
- case 8:
- data = (uint64_t)(stdvga_mem_readb(addr));
- data |= (uint64_t)(stdvga_mem_readb(addr + 1)) << 8;
- data |= (uint64_t)(stdvga_mem_readb(addr + 2)) << 16;
- data |= (uint64_t)(stdvga_mem_readb(addr + 3)) << 24;
- data |= (uint64_t)(stdvga_mem_readb(addr + 4)) << 32;
- data |= (uint64_t)(stdvga_mem_readb(addr + 5)) << 40;
- data |= (uint64_t)(stdvga_mem_readb(addr + 6)) << 48;
- data |= (uint64_t)(stdvga_mem_readb(addr + 7)) << 56;
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
-
- *p_data = data;
- return X86EMUL_OKAY;
-}
-
-static void stdvga_mem_writeb(uint64_t addr, uint32_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane, write_mode, b, func_select, mask;
- uint32_t write_mask, bit_mask, set_mask, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- plane = addr & 3;
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- addr = ((addr & ~1) << 1) | plane;
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else
- {
- write_mode = s->gr[5] & 3;
- switch ( write_mode )
- {
- default:
- case 0:
- /* rotate */
- b = s->gr[3] & 7;
- val = ((val >> b) | (val << (8 - b))) & 0xff;
- val |= val << 8;
- val |= val << 16;
-
- /* apply set/reset mask */
- set_mask = mask16[s->gr[1]];
- val = (val & ~set_mask) | (mask16[s->gr[0]] & set_mask);
- bit_mask = s->gr[8];
- break;
- case 1:
- val = s->latch;
- goto do_write;
- case 2:
- val = mask16[val & 0x0f];
- bit_mask = s->gr[8];
- break;
- case 3:
- /* rotate */
- b = s->gr[3] & 7;
- val = (val >> b) | (val << (8 - b));
-
- bit_mask = s->gr[8] & val;
- val = mask16[s->gr[0]];
- break;
- }
-
- /* apply logical operation */
- func_select = s->gr[3] >> 3;
- switch ( func_select )
- {
- case 0:
- default:
- /* nothing to do */
- break;
- case 1:
- /* and */
- val &= s->latch;
- break;
- case 2:
- /* or */
- val |= s->latch;
- break;
- case 3:
- /* xor */
- val ^= s->latch;
- break;
- }
-
- /* apply bit mask */
- bit_mask |= bit_mask << 8;
- bit_mask |= bit_mask << 16;
- val = (val & bit_mask) | (s->latch & ~bit_mask);
-
- do_write:
- /* mask data according to sr[2] */
- mask = s->sr[2];
- write_mask = mask16[mask];
- vram_l = vram_getl(s, addr);
- *vram_l = (*vram_l & ~write_mask) | (val & write_mask);
- vram_put(s, vram_l);
- }
+ ASSERT_UNREACHABLE();
+ *p_data = ~0;
+ return X86EMUL_UNHANDLEABLE;
}
static int stdvga_mem_write(const struct hvm_io_handler *handler,
@@ -420,47 +150,8 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
.dir = IOREQ_WRITE,
.data = data,
};
- struct ioreq_server *srv;
-
- goto done;
-
- /* Intercept mmio write */
- switch ( size )
- {
- case 1:
- stdvga_mem_writeb(addr, (data >> 0) & 0xff);
- break;
-
- case 2:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- break;
-
- case 4:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- break;
-
- case 8:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- stdvga_mem_writeb(addr+4, (data >> 32) & 0xff);
- stdvga_mem_writeb(addr+5, (data >> 40) & 0xff);
- stdvga_mem_writeb(addr+6, (data >> 48) & 0xff);
- stdvga_mem_writeb(addr+7, (data >> 56) & 0xff);
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
+ struct ioreq_server *srv = ioreq_server_select(current->domain, &p);
- done:
- srv = ioreq_server_select(current->domain, &p);
if ( !srv )
return X86EMUL_UNHANDLEABLE;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index d964f8efdfe2..7bd1fa113e39 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -126,7 +126,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "gr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static gr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b16c0966a17f19c0e55ed0b9baa28191d2590178)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 138ced229d38..edcc456d1f76 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -50,18 +50,6 @@ static const uint8_t sr_mask[8] = {
(uint8_t)~0x00,
};
-static const uint8_t gr_mask[9] = {
- (uint8_t)~0xf0, /* 0x00 */
- (uint8_t)~0xf0, /* 0x01 */
- (uint8_t)~0xf0, /* 0x02 */
- (uint8_t)~0xe0, /* 0x03 */
- (uint8_t)~0xfc, /* 0x04 */
- (uint8_t)~0x84, /* 0x05 */
- (uint8_t)~0xf0, /* 0x06 */
- (uint8_t)~0xf0, /* 0x07 */
- (uint8_t)~0x00, /* 0x08 */
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -83,12 +71,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->gr_index = val;
break;
- case 0x3cf: /* graphics data register */
- rc = (s->gr_index < sizeof(s->gr));
- if ( rc )
- s->gr[s->gr_index] = val & gr_mask[s->gr_index];
- break;
-
default:
rc = 0;
break;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 7bd1fa113e39..375c12a466f2 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -125,7 +125,6 @@ struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
- uint8_t gr[9];
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "sr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static sr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 7aba44bdd78aedb97703811948c3b69ccff85032)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index edcc456d1f76..70facdbd9602 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -38,18 +38,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-/* force some bits to zero */
-static const uint8_t sr_mask[8] = {
- (uint8_t)~0xfc,
- (uint8_t)~0xc2,
- (uint8_t)~0xf0,
- (uint8_t)~0xc0,
- (uint8_t)~0xf1,
- (uint8_t)~0xff,
- (uint8_t)~0xff,
- (uint8_t)~0x00,
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -61,12 +49,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->sr_index = val;
break;
- case 0x3c5: /* sequencer data register */
- rc = (s->sr_index < sizeof(s->sr));
- if ( rc )
- s->sr[s->sr_index] = val & sr_mask[s->sr_index] ;
- break;
-
case 0x3ce: /* graphics address register */
s->gr_index = val;
break;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 375c12a466f2..d17cf20fd4e1 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -123,7 +123,6 @@ struct vpci_arch_msix_entry {
struct hvm_hw_stdvga {
uint8_t sr_index;
- uint8_t sr[8];
uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "{g,s}r_index" struct members
No consumers are left, hence the producer and the fields themselves can
also go away. stdvga_outb() is then useless, rendering stdvga_out()
useless as well. Hence the entire I/O port intercept can go away.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 86c03372e107f5c18266a62281663861b1144929)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 70facdbd9602..c363669b4aaa 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -38,62 +38,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-static int stdvga_outb(uint64_t addr, uint8_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1;
-
- switch ( addr )
- {
- case 0x3c4: /* sequencer address register */
- s->sr_index = val;
- break;
-
- case 0x3ce: /* graphics address register */
- s->gr_index = val;
- break;
-
- default:
- rc = 0;
- break;
- }
-
- return rc;
-}
-
-static void stdvga_out(uint32_t port, uint32_t bytes, uint32_t val)
-{
- switch ( bytes )
- {
- case 1:
- stdvga_outb(port, val);
- break;
-
- case 2:
- stdvga_outb(port + 0, val >> 0);
- stdvga_outb(port + 1, val >> 8);
- break;
-
- default:
- break;
- }
-}
-
-static int stdvga_intercept_pio(
- int dir, unsigned int port, unsigned int bytes, uint32_t *val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- if ( dir == IOREQ_WRITE )
- {
- spin_lock(&s->lock);
- stdvga_out(port, bytes, *val);
- spin_unlock(&s->lock);
- }
-
- return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
-}
-
static int stdvga_mem_read(const struct hvm_io_handler *handler,
uint64_t addr, uint32_t size, uint64_t *p_data)
{
@@ -194,11 +138,6 @@ void stdvga_init(struct domain *d)
{
struct hvm_io_handler *handler;
- /* Sequencer registers. */
- register_portio_handler(d, 0x3c4, 2, stdvga_intercept_pio);
- /* Graphics registers. */
- register_portio_handler(d, 0x3ce, 2, stdvga_intercept_pio);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index d17cf20fd4e1..03a437fb8bf4 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -122,8 +122,6 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- uint8_t sr_index;
- uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "vram_page[]" struct member
No uses are left, hence its setup, teardown, and the field itself can
also go away. stdvga_deinit() is then empty and can be dropped as well.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 3beb4baf2a0a2eef40d39eb7e6eecbfd36da5d14)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 542b60bc90e4..a998aff1ce52 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -709,7 +709,6 @@ int hvm_domain_initialise(struct domain *d)
return 0;
fail2:
- stdvga_deinit(d);
vioapic_deinit(d);
fail1:
if ( is_hardware_domain(d) )
@@ -772,7 +771,6 @@ void hvm_domain_destroy(struct domain *d)
if ( hvm_funcs.domain_destroy )
alternative_vcall(hvm_funcs.domain_destroy, d);
- stdvga_deinit(d);
vioapic_deinit(d);
XFREE(d->arch.hvm.pl_time);
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index c363669b4aaa..3890d69c82c7 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -116,8 +116,7 @@ static const struct hvm_io_ops stdvga_mem_ops = {
void stdvga_init(struct domain *d)
{
struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- struct page_info *pg;
- unsigned int i;
+ struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
@@ -125,47 +124,15 @@ void stdvga_init(struct domain *d)
memset(s, 0, sizeof(*s));
spin_lock_init(&s->lock);
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
+ /* VGA memory */
+ handler = hvm_next_io_handler(d);
+ if ( handler )
{
- pg = alloc_domheap_page(d, MEMF_no_owner);
- if ( pg == NULL )
- break;
- s->vram_page[i] = pg;
- clear_domain_page(page_to_mfn(pg));
- }
-
- if ( i == ARRAY_SIZE(s->vram_page) )
- {
- struct hvm_io_handler *handler;
-
- /* VGA memory */
- handler = hvm_next_io_handler(d);
-
- if ( handler == NULL )
- return;
-
handler->type = IOREQ_TYPE_COPY;
handler->ops = &stdvga_mem_ops;
}
}
-void stdvga_deinit(struct domain *d)
-{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- int i;
-
- if ( !has_vvga(d) )
- return;
-
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
- {
- if ( s->vram_page[i] == NULL )
- continue;
- free_domheap_page(s->vram_page[i]);
- s->vram_page[i] = NULL;
- }
-}
-
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 03a437fb8bf4..2f5bfeccae39 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -122,12 +122,10 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
void stdvga_init(struct domain *d);
-void stdvga_deinit(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "lock" struct member
No state is left to protect. It being the last field, drop the struct
itself as well. Similarly for then ending up empty, drop the .complete
handler.
This is part of XSA-463 / CVE-2024-45818
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b180a50326c8a2c171f37c1940a0fbbdcad4be90)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 3890d69c82c7..3c5340ad9279 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -69,61 +69,35 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
const ioreq_t *p)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- /*
- * The range check must be done without taking the lock, to avoid
- * deadlock when hvm_mmio_internal() is called from
- * hvm_copy_to/from_guest_phys() in hvm_process_io_intercept().
- */
if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
return 0;
- spin_lock(&s->lock);
-
if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
* Only accept single direct writes, as that's the only thing we can
* accelerate using buffered ioreq handling.
*/
- goto reject;
+ return false;
}
- /* s->lock intentionally held */
- return 1;
-
- reject:
- spin_unlock(&s->lock);
- return 0;
-}
-
-static void stdvga_mem_complete(const struct hvm_io_handler *handler)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- spin_unlock(&s->lock);
+ return true;
}
static const struct hvm_io_ops stdvga_mem_ops = {
.accept = stdvga_mem_accept,
.read = stdvga_mem_read,
.write = stdvga_mem_write,
- .complete = stdvga_mem_complete
};
void stdvga_init(struct domain *d)
{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
- memset(s, 0, sizeof(*s));
- spin_lock_init(&s->lock);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
if ( handler )
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 698455444ea8..1e5d7db6ed09 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -83,7 +83,6 @@ struct hvm_domain {
struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */
struct hvm_vioapic **vioapic;
unsigned int nr_vioapics;
- struct hvm_hw_stdvga stdvga;
/*
* hvm_hw_pmtimer is a publicly-visible name. We will defer renaming
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 2f5bfeccae39..784bd0c83f2f 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -121,10 +121,6 @@ struct vpci_arch_msix_entry {
int pirq;
};
-struct hvm_hw_stdvga {
- spinlock_t lock;
-};
-
void stdvga_init(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: x86/hvm: Simplify stdvga_mem_accept() further
stdvga_mem_accept() is called on almost all IO emulations, and the
overwhelming likely answer is to reject the ioreq. Simply rearranging the
expression yields an improvement:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
Function old new delta
stdvga_mem_accept 109 52 -57
which is best explained looking at the disassembly:
Before: After:
f3 0f 1e fa endbr64 f3 0f 1e fa endbr64
0f b6 4e 1e movzbl 0x1e(%rsi),%ecx | 0f b6 46 1e movzbl 0x1e(%rsi),%eax
48 8b 16 mov (%rsi),%rdx | 31 d2 xor %edx,%edx
f6 c1 40 test $0x40,%cl | a8 30 test $0x30,%al
75 38 jne <stdvga_mem_accept+0x48> | 75 23 jne <stdvga_mem_accept+0x31>
31 c0 xor %eax,%eax <
48 81 fa ff ff 09 00 cmp $0x9ffff,%rdx <
76 26 jbe <stdvga_mem_accept+0x41> <
8b 46 14 mov 0x14(%rsi),%eax <
8b 7e 10 mov 0x10(%rsi),%edi <
48 0f af c7 imul %rdi,%rax <
48 8d 54 02 ff lea -0x1(%rdx,%rax,1),%rdx <
31 c0 xor %eax,%eax <
48 81 fa ff ff 0b 00 cmp $0xbffff,%rdx <
77 0c ja <stdvga_mem_accept+0x41> <
83 e1 30 and $0x30,%ecx <
75 07 jne <stdvga_mem_accept+0x41> <
83 7e 10 01 cmpl $0x1,0x10(%rsi) 83 7e 10 01 cmpl $0x1,0x10(%rsi)
0f 94 c0 sete %al | 75 1d jne <stdvga_mem_accept+0x31>
c3 ret | 48 8b 0e mov (%rsi),%rcx
66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) | 48 81 f9 ff ff 09 00 cmp $0x9ffff,%rcx
8b 46 10 mov 0x10(%rsi),%eax | 76 11 jbe <stdvga_mem_accept+0x31>
8b 7e 14 mov 0x14(%rsi),%edi | 8b 46 14 mov 0x14(%rsi),%eax
49 89 d0 mov %rdx,%r8 | 48 8d 44 01 ff lea -0x1(%rcx,%rax,1),%rax
48 83 e8 01 sub $0x1,%rax | 48 3d ff ff 0b 00 cmp $0xbffff,%rax
48 8d 54 3a ff lea -0x1(%rdx,%rdi,1),%rdx | 0f 96 c2 setbe %dl
48 0f af c7 imul %rdi,%rax | 89 d0 mov %edx,%eax
49 29 c0 sub %rax,%r8 <
31 c0 xor %eax,%eax <
49 81 f8 ff ff 09 00 cmp $0x9ffff,%r8 <
77 be ja <stdvga_mem_accept+0x2a> <
c3 ret c3 ret
By moving the "p->count != 1" check ahead of the
ioreq_mmio_{first,last}_byte() calls, both multiplies disappear along with a
lot of surrounding logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(cherry picked from commit 08ffd8705d36c7c445df3ecee8ad9b8f8d65fbe0)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 3c5340ad9279..fdb41ef305ef 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -69,18 +69,14 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
const ioreq_t *p)
{
- if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
+ /*
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
+ */
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 ||
+ (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
- return 0;
-
- if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
- {
- /*
- * Only accept single direct writes, as that's the only thing we can
- * accelerate using buffered ioreq handling.
- */
return false;
- }
return true;
}
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "cache" struct member
Since 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
guests"), HVM guests are built using XEN_DOMCTL_sethvmcontext, which
ends up disabling stdvga caching because of arch_hvm_load() being
involved in the processing of the request. With that the field is
useless, and can be dropped. Drop the helper functions manipulating /
checking as well right away, but leave the use sites of
stdvga_cache_is_enabled() with the hard-coded result the function would
have produced, to aid validation of subsequent dropping of further code.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 53b7246bdfb3c280adcdf714918e4decb7e108f4)
diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
index 86c82cbd7456..ad2594f95b3f 100644
--- a/xen/arch/x86/hvm/save.c
+++ b/xen/arch/x86/hvm/save.c
@@ -77,9 +77,6 @@ int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
/* Time when restore started */
d->arch.hvm.sync_tsc = rdtsc();
- /* VGA state is not saved/restored, so we nobble the cache. */
- d->arch.hvm.stdvga.cache = STDVGA_CACHE_DISABLED;
-
return 0;
}
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index be8200c8d072..bed25e3cff62 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -101,37 +101,6 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
unmap_domain_page(p);
}
-static void stdvga_try_cache_enable(struct hvm_hw_stdvga *s)
-{
- /*
- * Caching mode can only be enabled if the the cache has
- * never been used before. As soon as it is disabled, it will
- * become out-of-sync with the VGA device model and since no
- * mechanism exists to acquire current VRAM state from the
- * device model, re-enabling it would lead to stale data being
- * seen by the guest.
- */
- if ( s->cache != STDVGA_CACHE_UNINITIALIZED )
- return;
-
- gdprintk(XENLOG_INFO, "entering caching mode\n");
- s->cache = STDVGA_CACHE_ENABLED;
-}
-
-static void stdvga_cache_disable(struct hvm_hw_stdvga *s)
-{
- if ( s->cache != STDVGA_CACHE_ENABLED )
- return;
-
- gdprintk(XENLOG_INFO, "leaving caching mode\n");
- s->cache = STDVGA_CACHE_DISABLED;
-}
-
-static bool_t stdvga_cache_is_enabled(const struct hvm_hw_stdvga *s)
-{
- return s->cache == STDVGA_CACHE_ENABLED;
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -171,7 +140,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
if ( !prev_stdvga && s->stdvga )
{
gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- stdvga_try_cache_enable(s);
}
else if ( prev_stdvga && !s->stdvga )
{
@@ -469,7 +437,7 @@ static int cf_check stdvga_mem_write(
};
struct ioreq_server *srv;
- if ( !stdvga_cache_is_enabled(s) || !s->stdvga )
+ if ( true || !s->stdvga )
goto done;
/* Intercept mmio write */
@@ -537,18 +505,12 @@ static bool cf_check stdvga_mem_accept(
* We cannot return X86EMUL_UNHANDLEABLE on anything other then the
* first cycle of an I/O. So, since we cannot guarantee to always be
* able to send buffered writes, we have to reject any multi-cycle
- * I/O and, since we are rejecting an I/O, we must invalidate the
- * cache.
- * Single-cycle write transactions are accepted even if the cache is
- * not active since we can assert, when in stdvga mode, that writes
- * to VRAM have no side effect and thus we can try to buffer them.
+ * I/O.
*/
- stdvga_cache_disable(s);
-
goto reject;
}
else if ( p->dir == IOREQ_READ &&
- (!stdvga_cache_is_enabled(s) || !s->stdvga) )
+ (true || !s->stdvga) )
goto reject;
/* s->lock intentionally held */
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 54e0161b492c..d92f87f83bb7 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -121,19 +121,12 @@ struct vpci_arch_msix_entry {
int pirq;
};
-enum stdvga_cache_state {
- STDVGA_CACHE_UNINITIALIZED,
- STDVGA_CACHE_ENABLED,
- STDVGA_CACHE_DISABLED
-};
-
struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
bool_t stdvga;
- enum stdvga_cache_state cache;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: properly reject "indirect" VRAM writes
While ->count will only be different from 1 for "indirect" (data in
guest memory) accesses, it being 1 does not exclude the request being an
"indirect" one. Check both to be on the safe side, and bring the ->count
part also in line with what ioreq_send_buffered() actually refuses to
handle.
This is part of XSA-463 / CVE-2024-45818
Fixes: 3bbaaec09b1b ("x86/hvm: unify stdvga mmio intercept with standard mmio intercept")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit eb7cd0593d88c4b967a24bca8bd30591966676cd)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index bed25e3cff62..f5a4b1cfe9c2 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -499,13 +499,13 @@ static bool cf_check stdvga_mem_accept(
spin_lock(&s->lock);
- if ( p->dir == IOREQ_WRITE && p->count > 1 )
+ if ( p->dir == IOREQ_WRITE && (p->data_is_ptr || p->count != 1) )
{
/*
* We cannot return X86EMUL_UNHANDLEABLE on anything other then the
* first cycle of an I/O. So, since we cannot guarantee to always be
* able to send buffered writes, we have to reject any multi-cycle
- * I/O.
+ * or "indirect" I/O.
*/
goto reject;
}
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "stdvga" struct member
Two of its consumers are dead (in compile-time constant conditionals)
and the only remaining ones are merely controlling debug logging. Hence
the field is now pointless to set, which in particular allows to get rid
of the questionable conditional from which the field's value was
established (afaict 551ceee97513 ["x86, hvm: stdvga cache always on"]
had dropped too much of the earlier extra check that was there, and
quite likely further checks were missing).
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b740a9369e81bdda675a9780130ce2b9e75d4ec9)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index f5a4b1cfe9c2..3a84a7c35e9e 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -104,7 +104,7 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1, prev_stdvga = s->stdvga;
+ int rc = 1;
switch ( addr )
{
@@ -133,19 +133,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
break;
}
- /* When in standard vga mode, emulate here all writes to the vram buffer
- * so we can immediately satisfy reads without waiting for qemu. */
- s->stdvga = (s->sr[7] == 0x00);
-
- if ( !prev_stdvga && s->stdvga )
- {
- gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- }
- else if ( prev_stdvga && !s->stdvga )
- {
- gdprintk(XENLOG_INFO, "leaving stdvga mode\n");
- }
-
return rc;
}
@@ -426,7 +413,6 @@ static int cf_check stdvga_mem_write(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t data)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = addr,
@@ -437,8 +423,7 @@ static int cf_check stdvga_mem_write(
};
struct ioreq_server *srv;
- if ( true || !s->stdvga )
- goto done;
+ goto done;
/* Intercept mmio write */
switch ( size )
@@ -499,19 +484,14 @@ static bool cf_check stdvga_mem_accept(
spin_lock(&s->lock);
- if ( p->dir == IOREQ_WRITE && (p->data_is_ptr || p->count != 1) )
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
- * We cannot return X86EMUL_UNHANDLEABLE on anything other then the
- * first cycle of an I/O. So, since we cannot guarantee to always be
- * able to send buffered writes, we have to reject any multi-cycle
- * or "indirect" I/O.
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
*/
goto reject;
}
- else if ( p->dir == IOREQ_READ &&
- (true || !s->stdvga) )
- goto reject;
/* s->lock intentionally held */
return 1;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index d92f87f83bb7..d964f8efdfe2 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -126,7 +126,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- bool_t stdvga;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: remove unused MMIO handling code
All read accesses are rejected by the ->accept handler, while writes
bypass the bulk of the function body. Drop the dead code, leaving an
assertion in the read handler.
A number of other static items (and a macro) are then unreferenced and
hence also need (want) dropping. The same applies to the "latch" field
of the state structure.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 89108547af1f230b72893b48351f9c1106189649)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 3a84a7c35e9e..915dfc823079 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -38,26 +38,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-#define PAT(x) (x)
-static const uint32_t mask16[16] = {
- PAT(0x00000000),
- PAT(0x000000ff),
- PAT(0x0000ff00),
- PAT(0x0000ffff),
- PAT(0x00ff0000),
- PAT(0x00ff00ff),
- PAT(0x00ffff00),
- PAT(0x00ffffff),
- PAT(0xff000000),
- PAT(0xff0000ff),
- PAT(0xff00ff00),
- PAT(0xff00ffff),
- PAT(0xffff0000),
- PAT(0xffff00ff),
- PAT(0xffffff00),
- PAT(0xffffffff),
-};
-
/* force some bits to zero */
static const uint8_t sr_mask[8] = {
(uint8_t)~0xfc,
@@ -82,25 +62,6 @@ static const uint8_t gr_mask[9] = {
(uint8_t)~0x00, /* 0x08 */
};
-static uint8_t *vram_getb(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 12) & 0x3f];
- uint8_t *p = __map_domain_page(pg);
- return &p[a & 0xfff];
-}
-
-static uint32_t *vram_getl(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 10) & 0x3f];
- uint32_t *p = __map_domain_page(pg);
- return &p[a & 0x3ff];
-}
-
-static void vram_put(struct hvm_hw_stdvga *s, void *p)
-{
- unmap_domain_page(p);
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -169,244 +130,13 @@ static int cf_check stdvga_intercept_pio(
return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
}
-static unsigned int stdvga_mem_offset(
- struct hvm_hw_stdvga *s, unsigned int mmio_addr)
-{
- unsigned int memory_map_mode = (s->gr[6] >> 2) & 3;
- unsigned int offset = mmio_addr & 0x1ffff;
-
- switch ( memory_map_mode )
- {
- case 0:
- break;
- case 1:
- if ( offset >= 0x10000 )
- goto fail;
- offset += 0; /* assume bank_offset == 0; */
- break;
- case 2:
- offset -= 0x10000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- default:
- case 3:
- offset -= 0x18000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- }
-
- return offset;
-
- fail:
- return ~0u;
-}
-
-#define GET_PLANE(data, p) (((data) >> ((p) * 8)) & 0xff)
-
-static uint8_t stdvga_mem_readb(uint64_t addr)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane;
- uint32_t ret, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return 0xff;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- vram_b = vram_getb(s, addr);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- vram_b = vram_getb(s, ((addr & ~1) << 1) | plane);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else
- {
- /* standard VGA latched access */
- vram_l = vram_getl(s, addr);
- s->latch = *vram_l;
- vram_put(s, vram_l);
-
- if ( !(s->gr[5] & 0x08) )
- {
- /* read mode 0 */
- plane = s->gr[4];
- ret = GET_PLANE(s->latch, plane);
- }
- else
- {
- /* read mode 1 */
- ret = (s->latch ^ mask16[s->gr[2]]) & mask16[s->gr[7]];
- ret |= ret >> 16;
- ret |= ret >> 8;
- ret = (~ret) & 0xff;
- }
- }
-
- return ret;
-}
-
static int cf_check stdvga_mem_read(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t *p_data)
{
- uint64_t data = ~0ul;
-
- switch ( size )
- {
- case 1:
- data = stdvga_mem_readb(addr);
- break;
-
- case 2:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- break;
-
- case 4:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- data |= stdvga_mem_readb(addr + 2) << 16;
- data |= (uint32_t)stdvga_mem_readb(addr + 3) << 24;
- break;
-
- case 8:
- data = (uint64_t)(stdvga_mem_readb(addr));
- data |= (uint64_t)(stdvga_mem_readb(addr + 1)) << 8;
- data |= (uint64_t)(stdvga_mem_readb(addr + 2)) << 16;
- data |= (uint64_t)(stdvga_mem_readb(addr + 3)) << 24;
- data |= (uint64_t)(stdvga_mem_readb(addr + 4)) << 32;
- data |= (uint64_t)(stdvga_mem_readb(addr + 5)) << 40;
- data |= (uint64_t)(stdvga_mem_readb(addr + 6)) << 48;
- data |= (uint64_t)(stdvga_mem_readb(addr + 7)) << 56;
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
-
- *p_data = data;
- return X86EMUL_OKAY;
-}
-
-static void stdvga_mem_writeb(uint64_t addr, uint32_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane, write_mode, b, func_select, mask;
- uint32_t write_mask, bit_mask, set_mask, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- plane = addr & 3;
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- addr = ((addr & ~1) << 1) | plane;
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else
- {
- write_mode = s->gr[5] & 3;
- switch ( write_mode )
- {
- default:
- case 0:
- /* rotate */
- b = s->gr[3] & 7;
- val = ((val >> b) | (val << (8 - b))) & 0xff;
- val |= val << 8;
- val |= val << 16;
-
- /* apply set/reset mask */
- set_mask = mask16[s->gr[1]];
- val = (val & ~set_mask) | (mask16[s->gr[0]] & set_mask);
- bit_mask = s->gr[8];
- break;
- case 1:
- val = s->latch;
- goto do_write;
- case 2:
- val = mask16[val & 0x0f];
- bit_mask = s->gr[8];
- break;
- case 3:
- /* rotate */
- b = s->gr[3] & 7;
- val = (val >> b) | (val << (8 - b));
-
- bit_mask = s->gr[8] & val;
- val = mask16[s->gr[0]];
- break;
- }
-
- /* apply logical operation */
- func_select = s->gr[3] >> 3;
- switch ( func_select )
- {
- case 0:
- default:
- /* nothing to do */
- break;
- case 1:
- /* and */
- val &= s->latch;
- break;
- case 2:
- /* or */
- val |= s->latch;
- break;
- case 3:
- /* xor */
- val ^= s->latch;
- break;
- }
-
- /* apply bit mask */
- bit_mask |= bit_mask << 8;
- bit_mask |= bit_mask << 16;
- val = (val & bit_mask) | (s->latch & ~bit_mask);
-
- do_write:
- /* mask data according to sr[2] */
- mask = s->sr[2];
- write_mask = mask16[mask];
- vram_l = vram_getl(s, addr);
- *vram_l = (*vram_l & ~write_mask) | (val & write_mask);
- vram_put(s, vram_l);
- }
+ ASSERT_UNREACHABLE();
+ *p_data = ~0;
+ return X86EMUL_UNHANDLEABLE;
}
static int cf_check stdvga_mem_write(
@@ -421,47 +151,8 @@ static int cf_check stdvga_mem_write(
.dir = IOREQ_WRITE,
.data = data,
};
- struct ioreq_server *srv;
-
- goto done;
-
- /* Intercept mmio write */
- switch ( size )
- {
- case 1:
- stdvga_mem_writeb(addr, (data >> 0) & 0xff);
- break;
-
- case 2:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- break;
-
- case 4:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- break;
-
- case 8:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- stdvga_mem_writeb(addr+4, (data >> 32) & 0xff);
- stdvga_mem_writeb(addr+5, (data >> 40) & 0xff);
- stdvga_mem_writeb(addr+6, (data >> 48) & 0xff);
- stdvga_mem_writeb(addr+7, (data >> 56) & 0xff);
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
+ struct ioreq_server *srv = ioreq_server_select(current->domain, &p);
- done:
- srv = ioreq_server_select(current->domain, &p);
if ( !srv )
return X86EMUL_UNHANDLEABLE;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index d964f8efdfe2..7bd1fa113e39 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -126,7 +126,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "gr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static gr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b16c0966a17f19c0e55ed0b9baa28191d2590178)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 915dfc823079..9b7455c58b36 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -50,18 +50,6 @@ static const uint8_t sr_mask[8] = {
(uint8_t)~0x00,
};
-static const uint8_t gr_mask[9] = {
- (uint8_t)~0xf0, /* 0x00 */
- (uint8_t)~0xf0, /* 0x01 */
- (uint8_t)~0xf0, /* 0x02 */
- (uint8_t)~0xe0, /* 0x03 */
- (uint8_t)~0xfc, /* 0x04 */
- (uint8_t)~0x84, /* 0x05 */
- (uint8_t)~0xf0, /* 0x06 */
- (uint8_t)~0xf0, /* 0x07 */
- (uint8_t)~0x00, /* 0x08 */
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -83,12 +71,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->gr_index = val;
break;
- case 0x3cf: /* graphics data register */
- rc = (s->gr_index < sizeof(s->gr));
- if ( rc )
- s->gr[s->gr_index] = val & gr_mask[s->gr_index];
- break;
-
default:
rc = 0;
break;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 7bd1fa113e39..375c12a466f2 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -125,7 +125,6 @@ struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
- uint8_t gr[9];
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "sr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static sr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 7aba44bdd78aedb97703811948c3b69ccff85032)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 9b7455c58b36..818dba67d7b7 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -38,18 +38,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-/* force some bits to zero */
-static const uint8_t sr_mask[8] = {
- (uint8_t)~0xfc,
- (uint8_t)~0xc2,
- (uint8_t)~0xf0,
- (uint8_t)~0xc0,
- (uint8_t)~0xf1,
- (uint8_t)~0xff,
- (uint8_t)~0xff,
- (uint8_t)~0x00,
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -61,12 +49,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->sr_index = val;
break;
- case 0x3c5: /* sequencer data register */
- rc = (s->sr_index < sizeof(s->sr));
- if ( rc )
- s->sr[s->sr_index] = val & sr_mask[s->sr_index] ;
- break;
-
case 0x3ce: /* graphics address register */
s->gr_index = val;
break;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 375c12a466f2..d17cf20fd4e1 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -123,7 +123,6 @@ struct vpci_arch_msix_entry {
struct hvm_hw_stdvga {
uint8_t sr_index;
- uint8_t sr[8];
uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "{g,s}r_index" struct members
No consumers are left, hence the producer and the fields themselves can
also go away. stdvga_outb() is then useless, rendering stdvga_out()
useless as well. Hence the entire I/O port intercept can go away.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 86c03372e107f5c18266a62281663861b1144929)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 818dba67d7b7..029328b4b1eb 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -38,62 +38,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-static int stdvga_outb(uint64_t addr, uint8_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1;
-
- switch ( addr )
- {
- case 0x3c4: /* sequencer address register */
- s->sr_index = val;
- break;
-
- case 0x3ce: /* graphics address register */
- s->gr_index = val;
- break;
-
- default:
- rc = 0;
- break;
- }
-
- return rc;
-}
-
-static void stdvga_out(uint32_t port, uint32_t bytes, uint32_t val)
-{
- switch ( bytes )
- {
- case 1:
- stdvga_outb(port, val);
- break;
-
- case 2:
- stdvga_outb(port + 0, val >> 0);
- stdvga_outb(port + 1, val >> 8);
- break;
-
- default:
- break;
- }
-}
-
-static int cf_check stdvga_intercept_pio(
- int dir, unsigned int port, unsigned int bytes, uint32_t *val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- if ( dir == IOREQ_WRITE )
- {
- spin_lock(&s->lock);
- stdvga_out(port, bytes, *val);
- spin_unlock(&s->lock);
- }
-
- return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
-}
-
static int cf_check stdvga_mem_read(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t *p_data)
@@ -195,11 +139,6 @@ void stdvga_init(struct domain *d)
{
struct hvm_io_handler *handler;
- /* Sequencer registers. */
- register_portio_handler(d, 0x3c4, 2, stdvga_intercept_pio);
- /* Graphics registers. */
- register_portio_handler(d, 0x3ce, 2, stdvga_intercept_pio);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index d17cf20fd4e1..03a437fb8bf4 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -122,8 +122,6 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- uint8_t sr_index;
- uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "vram_page[]" struct member
No uses are left, hence its setup, teardown, and the field itself can
also go away. stdvga_deinit() is then empty and can be dropped as well.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 3beb4baf2a0a2eef40d39eb7e6eecbfd36da5d14)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6ae15e04f390..4454b158b698 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -677,7 +677,6 @@ int hvm_domain_initialise(struct domain *d,
return 0;
fail2:
- stdvga_deinit(d);
vioapic_deinit(d);
fail1:
if ( is_hardware_domain(d) )
@@ -740,7 +739,6 @@ void hvm_domain_destroy(struct domain *d)
if ( hvm_funcs.domain_destroy )
alternative_vcall(hvm_funcs.domain_destroy, d);
- stdvga_deinit(d);
vioapic_deinit(d);
XFREE(d->arch.hvm.pl_time);
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 029328b4b1eb..1d171cb42712 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -117,8 +117,7 @@ static const struct hvm_io_ops stdvga_mem_ops = {
void stdvga_init(struct domain *d)
{
struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- struct page_info *pg;
- unsigned int i;
+ struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
@@ -126,47 +125,15 @@ void stdvga_init(struct domain *d)
memset(s, 0, sizeof(*s));
spin_lock_init(&s->lock);
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
+ /* VGA memory */
+ handler = hvm_next_io_handler(d);
+ if ( handler )
{
- pg = alloc_domheap_page(d, MEMF_no_owner);
- if ( pg == NULL )
- break;
- s->vram_page[i] = pg;
- clear_domain_page(page_to_mfn(pg));
- }
-
- if ( i == ARRAY_SIZE(s->vram_page) )
- {
- struct hvm_io_handler *handler;
-
- /* VGA memory */
- handler = hvm_next_io_handler(d);
-
- if ( handler == NULL )
- return;
-
handler->type = IOREQ_TYPE_COPY;
handler->ops = &stdvga_mem_ops;
}
}
-void stdvga_deinit(struct domain *d)
-{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- int i;
-
- if ( !has_vvga(d) )
- return;
-
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
- {
- if ( s->vram_page[i] == NULL )
- continue;
- free_domheap_page(s->vram_page[i]);
- s->vram_page[i] = NULL;
- }
-}
-
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 03a437fb8bf4..2f5bfeccae39 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -122,12 +122,10 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
void stdvga_init(struct domain *d);
-void stdvga_deinit(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "lock" struct member
No state is left to protect. It being the last field, drop the struct
itself as well. Similarly for then ending up empty, drop the .complete
handler.
This is part of XSA-463 / CVE-2024-45818
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b180a50326c8a2c171f37c1940a0fbbdcad4be90)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 1d171cb42712..fd38fb48002c 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -70,61 +70,35 @@ static int cf_check stdvga_mem_write(
static bool cf_check stdvga_mem_accept(
const struct hvm_io_handler *handler, const ioreq_t *p)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- /*
- * The range check must be done without taking the lock, to avoid
- * deadlock when hvm_mmio_internal() is called from
- * hvm_copy_to/from_guest_phys() in hvm_process_io_intercept().
- */
if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
return 0;
- spin_lock(&s->lock);
-
if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
* Only accept single direct writes, as that's the only thing we can
* accelerate using buffered ioreq handling.
*/
- goto reject;
+ return false;
}
- /* s->lock intentionally held */
- return 1;
-
- reject:
- spin_unlock(&s->lock);
- return 0;
-}
-
-static void cf_check stdvga_mem_complete(const struct hvm_io_handler *handler)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- spin_unlock(&s->lock);
+ return true;
}
static const struct hvm_io_ops stdvga_mem_ops = {
.accept = stdvga_mem_accept,
.read = stdvga_mem_read,
.write = stdvga_mem_write,
- .complete = stdvga_mem_complete
};
void stdvga_init(struct domain *d)
{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
- memset(s, 0, sizeof(*s));
- spin_lock_init(&s->lock);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
if ( handler )
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index e8e59a72bccc..0b86329ea378 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -83,7 +83,6 @@ struct hvm_domain {
struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */
struct hvm_vioapic **vioapic;
unsigned int nr_vioapics;
- struct hvm_hw_stdvga stdvga;
/*
* hvm_hw_pmtimer is a publicly-visible name. We will defer renaming
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 2f5bfeccae39..784bd0c83f2f 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -121,10 +121,6 @@ struct vpci_arch_msix_entry {
int pirq;
};
-struct hvm_hw_stdvga {
- spinlock_t lock;
-};
-
void stdvga_init(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: x86/hvm: Simplify stdvga_mem_accept() further
stdvga_mem_accept() is called on almost all IO emulations, and the
overwhelming likely answer is to reject the ioreq. Simply rearranging the
expression yields an improvement:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
Function old new delta
stdvga_mem_accept 109 52 -57
which is best explained looking at the disassembly:
Before: After:
f3 0f 1e fa endbr64 f3 0f 1e fa endbr64
0f b6 4e 1e movzbl 0x1e(%rsi),%ecx | 0f b6 46 1e movzbl 0x1e(%rsi),%eax
48 8b 16 mov (%rsi),%rdx | 31 d2 xor %edx,%edx
f6 c1 40 test $0x40,%cl | a8 30 test $0x30,%al
75 38 jne <stdvga_mem_accept+0x48> | 75 23 jne <stdvga_mem_accept+0x31>
31 c0 xor %eax,%eax <
48 81 fa ff ff 09 00 cmp $0x9ffff,%rdx <
76 26 jbe <stdvga_mem_accept+0x41> <
8b 46 14 mov 0x14(%rsi),%eax <
8b 7e 10 mov 0x10(%rsi),%edi <
48 0f af c7 imul %rdi,%rax <
48 8d 54 02 ff lea -0x1(%rdx,%rax,1),%rdx <
31 c0 xor %eax,%eax <
48 81 fa ff ff 0b 00 cmp $0xbffff,%rdx <
77 0c ja <stdvga_mem_accept+0x41> <
83 e1 30 and $0x30,%ecx <
75 07 jne <stdvga_mem_accept+0x41> <
83 7e 10 01 cmpl $0x1,0x10(%rsi) 83 7e 10 01 cmpl $0x1,0x10(%rsi)
0f 94 c0 sete %al | 75 1d jne <stdvga_mem_accept+0x31>
c3 ret | 48 8b 0e mov (%rsi),%rcx
66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) | 48 81 f9 ff ff 09 00 cmp $0x9ffff,%rcx
8b 46 10 mov 0x10(%rsi),%eax | 76 11 jbe <stdvga_mem_accept+0x31>
8b 7e 14 mov 0x14(%rsi),%edi | 8b 46 14 mov 0x14(%rsi),%eax
49 89 d0 mov %rdx,%r8 | 48 8d 44 01 ff lea -0x1(%rcx,%rax,1),%rax
48 83 e8 01 sub $0x1,%rax | 48 3d ff ff 0b 00 cmp $0xbffff,%rax
48 8d 54 3a ff lea -0x1(%rdx,%rdi,1),%rdx | 0f 96 c2 setbe %dl
48 0f af c7 imul %rdi,%rax | 89 d0 mov %edx,%eax
49 29 c0 sub %rax,%r8 <
31 c0 xor %eax,%eax <
49 81 f8 ff ff 09 00 cmp $0x9ffff,%r8 <
77 be ja <stdvga_mem_accept+0x2a> <
c3 ret c3 ret
By moving the "p->count != 1" check ahead of the
ioreq_mmio_{first,last}_byte() calls, both multiplies disappear along with a
lot of surrounding logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(cherry picked from commit 08ffd8705d36c7c445df3ecee8ad9b8f8d65fbe0)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index fd38fb48002c..7554fa10cc96 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -70,18 +70,14 @@ static int cf_check stdvga_mem_write(
static bool cf_check stdvga_mem_accept(
const struct hvm_io_handler *handler, const ioreq_t *p)
{
- if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
+ /*
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
+ */
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 ||
+ (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
- return 0;
-
- if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
- {
- /*
- * Only accept single direct writes, as that's the only thing we can
- * accelerate using buffered ioreq handling.
- */
return false;
- }
return true;
}
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "cache" struct member
Since 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
guests"), HVM guests are built using XEN_DOMCTL_sethvmcontext, which
ends up disabling stdvga caching because of arch_hvm_load() being
involved in the processing of the request. With that the field is
useless, and can be dropped. Drop the helper functions manipulating /
checking as well right away, but leave the use sites of
stdvga_cache_is_enabled() with the hard-coded result the function would
have produced, to aid validation of subsequent dropping of further code.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 53b7246bdfb3c280adcdf714918e4decb7e108f4)
diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
index 79713cd6cad0..832a10f67e8e 100644
--- a/xen/arch/x86/hvm/save.c
+++ b/xen/arch/x86/hvm/save.c
@@ -64,9 +64,6 @@ int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
/* Time when restore started */
d->arch.hvm.sync_tsc = rdtsc();
- /* VGA state is not saved/restored, so we nobble the cache. */
- d->arch.hvm.stdvga.cache = STDVGA_CACHE_DISABLED;
-
return 0;
}
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 64192112661b..ac7056b00bc7 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -100,37 +100,6 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
unmap_domain_page(p);
}
-static void stdvga_try_cache_enable(struct hvm_hw_stdvga *s)
-{
- /*
- * Caching mode can only be enabled if the the cache has
- * never been used before. As soon as it is disabled, it will
- * become out-of-sync with the VGA device model and since no
- * mechanism exists to acquire current VRAM state from the
- * device model, re-enabling it would lead to stale data being
- * seen by the guest.
- */
- if ( s->cache != STDVGA_CACHE_UNINITIALIZED )
- return;
-
- gdprintk(XENLOG_INFO, "entering caching mode\n");
- s->cache = STDVGA_CACHE_ENABLED;
-}
-
-static void stdvga_cache_disable(struct hvm_hw_stdvga *s)
-{
- if ( s->cache != STDVGA_CACHE_ENABLED )
- return;
-
- gdprintk(XENLOG_INFO, "leaving caching mode\n");
- s->cache = STDVGA_CACHE_DISABLED;
-}
-
-static bool_t stdvga_cache_is_enabled(const struct hvm_hw_stdvga *s)
-{
- return s->cache == STDVGA_CACHE_ENABLED;
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -170,7 +139,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
if ( !prev_stdvga && s->stdvga )
{
gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- stdvga_try_cache_enable(s);
}
else if ( prev_stdvga && !s->stdvga )
{
@@ -468,7 +436,7 @@ static int cf_check stdvga_mem_write(
};
struct ioreq_server *srv;
- if ( !stdvga_cache_is_enabled(s) || !s->stdvga )
+ if ( true || !s->stdvga )
goto done;
/* Intercept mmio write */
@@ -536,18 +504,12 @@ static bool cf_check stdvga_mem_accept(
* We cannot return X86EMUL_UNHANDLEABLE on anything other then the
* first cycle of an I/O. So, since we cannot guarantee to always be
* able to send buffered writes, we have to reject any multi-cycle
- * or "indirect" I/O and, since we are rejecting an I/O, we must
- * invalidate the cache.
- * Single-cycle write transactions are accepted even if the cache is
- * not active since we can assert, when in stdvga mode, that writes
- * to VRAM have no side effect and thus we can try to buffer them.
+ * or "indirect" I/O.
*/
- stdvga_cache_disable(s);
-
goto reject;
}
else if ( p->dir == IOREQ_READ &&
- (!stdvga_cache_is_enabled(s) || !s->stdvga) )
+ (true || !s->stdvga) )
goto reject;
/* s->lock intentionally held */
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index e5225e75ef26..1abe1ab67bc3 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -110,19 +110,12 @@ struct vpci_arch_msix_entry {
int pirq;
};
-enum stdvga_cache_state {
- STDVGA_CACHE_UNINITIALIZED,
- STDVGA_CACHE_ENABLED,
- STDVGA_CACHE_DISABLED
-};
-
struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
bool_t stdvga;
- enum stdvga_cache_state cache;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "stdvga" struct member
Two of its consumers are dead (in compile-time constant conditionals)
and the only remaining ones are merely controlling debug logging. Hence
the field is now pointless to set, which in particular allows to get rid
of the questionable conditional from which the field's value was
established (afaict 551ceee97513 ["x86, hvm: stdvga cache always on"]
had dropped too much of the earlier extra check that was there, and
quite likely further checks were missing).
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b740a9369e81bdda675a9780130ce2b9e75d4ec9)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index ac7056b00bc7..76f0d83297cf 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -103,7 +103,7 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1, prev_stdvga = s->stdvga;
+ int rc = 1;
switch ( addr )
{
@@ -132,19 +132,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
break;
}
- /* When in standard vga mode, emulate here all writes to the vram buffer
- * so we can immediately satisfy reads without waiting for qemu. */
- s->stdvga = (s->sr[7] == 0x00);
-
- if ( !prev_stdvga && s->stdvga )
- {
- gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- }
- else if ( prev_stdvga && !s->stdvga )
- {
- gdprintk(XENLOG_INFO, "leaving stdvga mode\n");
- }
-
return rc;
}
@@ -425,7 +412,6 @@ static int cf_check stdvga_mem_write(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t data)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = addr,
@@ -436,8 +422,7 @@ static int cf_check stdvga_mem_write(
};
struct ioreq_server *srv;
- if ( true || !s->stdvga )
- goto done;
+ goto done;
/* Intercept mmio write */
switch ( size )
@@ -498,19 +483,14 @@ static bool cf_check stdvga_mem_accept(
spin_lock(&s->lock);
- if ( p->dir == IOREQ_WRITE && (p->data_is_ptr || p->count != 1) )
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
- * We cannot return X86EMUL_UNHANDLEABLE on anything other then the
- * first cycle of an I/O. So, since we cannot guarantee to always be
- * able to send buffered writes, we have to reject any multi-cycle
- * or "indirect" I/O.
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
*/
goto reject;
}
- else if ( p->dir == IOREQ_READ &&
- (true || !s->stdvga) )
- goto reject;
/* s->lock intentionally held */
return 1;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 1abe1ab67bc3..28dbaf2e1ba6 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -115,7 +115,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- bool_t stdvga;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: remove unused MMIO handling code
All read accesses are rejected by the ->accept handler, while writes
bypass the bulk of the function body. Drop the dead code, leaving an
assertion in the read handler.
A number of other static items (and a macro) are then unreferenced and
hence also need (want) dropping. The same applies to the "latch" field
of the state structure.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 89108547af1f230b72893b48351f9c1106189649)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 76f0d83297cf..0f0bd100681b 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -37,26 +37,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-#define PAT(x) (x)
-static const uint32_t mask16[16] = {
- PAT(0x00000000),
- PAT(0x000000ff),
- PAT(0x0000ff00),
- PAT(0x0000ffff),
- PAT(0x00ff0000),
- PAT(0x00ff00ff),
- PAT(0x00ffff00),
- PAT(0x00ffffff),
- PAT(0xff000000),
- PAT(0xff0000ff),
- PAT(0xff00ff00),
- PAT(0xff00ffff),
- PAT(0xffff0000),
- PAT(0xffff00ff),
- PAT(0xffffff00),
- PAT(0xffffffff),
-};
-
/* force some bits to zero */
static const uint8_t sr_mask[8] = {
(uint8_t)~0xfc,
@@ -81,25 +61,6 @@ static const uint8_t gr_mask[9] = {
(uint8_t)~0x00, /* 0x08 */
};
-static uint8_t *vram_getb(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 12) & 0x3f];
- uint8_t *p = __map_domain_page(pg);
- return &p[a & 0xfff];
-}
-
-static uint32_t *vram_getl(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 10) & 0x3f];
- uint32_t *p = __map_domain_page(pg);
- return &p[a & 0x3ff];
-}
-
-static void vram_put(struct hvm_hw_stdvga *s, void *p)
-{
- unmap_domain_page(p);
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -168,244 +129,13 @@ static int cf_check stdvga_intercept_pio(
return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
}
-static unsigned int stdvga_mem_offset(
- struct hvm_hw_stdvga *s, unsigned int mmio_addr)
-{
- unsigned int memory_map_mode = (s->gr[6] >> 2) & 3;
- unsigned int offset = mmio_addr & 0x1ffff;
-
- switch ( memory_map_mode )
- {
- case 0:
- break;
- case 1:
- if ( offset >= 0x10000 )
- goto fail;
- offset += 0; /* assume bank_offset == 0; */
- break;
- case 2:
- offset -= 0x10000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- default:
- case 3:
- offset -= 0x18000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- }
-
- return offset;
-
- fail:
- return ~0u;
-}
-
-#define GET_PLANE(data, p) (((data) >> ((p) * 8)) & 0xff)
-
-static uint8_t stdvga_mem_readb(uint64_t addr)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane;
- uint32_t ret, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return 0xff;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- vram_b = vram_getb(s, addr);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- vram_b = vram_getb(s, ((addr & ~1) << 1) | plane);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else
- {
- /* standard VGA latched access */
- vram_l = vram_getl(s, addr);
- s->latch = *vram_l;
- vram_put(s, vram_l);
-
- if ( !(s->gr[5] & 0x08) )
- {
- /* read mode 0 */
- plane = s->gr[4];
- ret = GET_PLANE(s->latch, plane);
- }
- else
- {
- /* read mode 1 */
- ret = (s->latch ^ mask16[s->gr[2]]) & mask16[s->gr[7]];
- ret |= ret >> 16;
- ret |= ret >> 8;
- ret = (~ret) & 0xff;
- }
- }
-
- return ret;
-}
-
static int cf_check stdvga_mem_read(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t *p_data)
{
- uint64_t data = ~0UL;
-
- switch ( size )
- {
- case 1:
- data = stdvga_mem_readb(addr);
- break;
-
- case 2:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- break;
-
- case 4:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- data |= stdvga_mem_readb(addr + 2) << 16;
- data |= (uint32_t)stdvga_mem_readb(addr + 3) << 24;
- break;
-
- case 8:
- data = (uint64_t)(stdvga_mem_readb(addr));
- data |= (uint64_t)(stdvga_mem_readb(addr + 1)) << 8;
- data |= (uint64_t)(stdvga_mem_readb(addr + 2)) << 16;
- data |= (uint64_t)(stdvga_mem_readb(addr + 3)) << 24;
- data |= (uint64_t)(stdvga_mem_readb(addr + 4)) << 32;
- data |= (uint64_t)(stdvga_mem_readb(addr + 5)) << 40;
- data |= (uint64_t)(stdvga_mem_readb(addr + 6)) << 48;
- data |= (uint64_t)(stdvga_mem_readb(addr + 7)) << 56;
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
-
- *p_data = data;
- return X86EMUL_OKAY;
-}
-
-static void stdvga_mem_writeb(uint64_t addr, uint32_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane, write_mode, b, func_select, mask;
- uint32_t write_mask, bit_mask, set_mask, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- plane = addr & 3;
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- addr = ((addr & ~1) << 1) | plane;
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else
- {
- write_mode = s->gr[5] & 3;
- switch ( write_mode )
- {
- default:
- case 0:
- /* rotate */
- b = s->gr[3] & 7;
- val = ((val >> b) | (val << (8 - b))) & 0xff;
- val |= val << 8;
- val |= val << 16;
-
- /* apply set/reset mask */
- set_mask = mask16[s->gr[1]];
- val = (val & ~set_mask) | (mask16[s->gr[0]] & set_mask);
- bit_mask = s->gr[8];
- break;
- case 1:
- val = s->latch;
- goto do_write;
- case 2:
- val = mask16[val & 0x0f];
- bit_mask = s->gr[8];
- break;
- case 3:
- /* rotate */
- b = s->gr[3] & 7;
- val = (val >> b) | (val << (8 - b));
-
- bit_mask = s->gr[8] & val;
- val = mask16[s->gr[0]];
- break;
- }
-
- /* apply logical operation */
- func_select = s->gr[3] >> 3;
- switch ( func_select )
- {
- case 0:
- default:
- /* nothing to do */
- break;
- case 1:
- /* and */
- val &= s->latch;
- break;
- case 2:
- /* or */
- val |= s->latch;
- break;
- case 3:
- /* xor */
- val ^= s->latch;
- break;
- }
-
- /* apply bit mask */
- bit_mask |= bit_mask << 8;
- bit_mask |= bit_mask << 16;
- val = (val & bit_mask) | (s->latch & ~bit_mask);
-
- do_write:
- /* mask data according to sr[2] */
- mask = s->sr[2];
- write_mask = mask16[mask];
- vram_l = vram_getl(s, addr);
- *vram_l = (*vram_l & ~write_mask) | (val & write_mask);
- vram_put(s, vram_l);
- }
+ ASSERT_UNREACHABLE();
+ *p_data = ~0;
+ return X86EMUL_UNHANDLEABLE;
}
static int cf_check stdvga_mem_write(
@@ -420,47 +150,8 @@ static int cf_check stdvga_mem_write(
.dir = IOREQ_WRITE,
.data = data,
};
- struct ioreq_server *srv;
-
- goto done;
-
- /* Intercept mmio write */
- switch ( size )
- {
- case 1:
- stdvga_mem_writeb(addr, (data >> 0) & 0xff);
- break;
-
- case 2:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- break;
-
- case 4:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- break;
-
- case 8:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- stdvga_mem_writeb(addr+4, (data >> 32) & 0xff);
- stdvga_mem_writeb(addr+5, (data >> 40) & 0xff);
- stdvga_mem_writeb(addr+6, (data >> 48) & 0xff);
- stdvga_mem_writeb(addr+7, (data >> 56) & 0xff);
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
+ struct ioreq_server *srv = ioreq_server_select(current->domain, &p);
- done:
- srv = ioreq_server_select(current->domain, &p);
if ( !srv )
return X86EMUL_UNHANDLEABLE;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 28dbaf2e1ba6..19ecf4fd78e3 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -115,7 +115,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "gr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static gr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b16c0966a17f19c0e55ed0b9baa28191d2590178)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 0f0bd100681b..fa25833caa50 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -49,18 +49,6 @@ static const uint8_t sr_mask[8] = {
(uint8_t)~0x00,
};
-static const uint8_t gr_mask[9] = {
- (uint8_t)~0xf0, /* 0x00 */
- (uint8_t)~0xf0, /* 0x01 */
- (uint8_t)~0xf0, /* 0x02 */
- (uint8_t)~0xe0, /* 0x03 */
- (uint8_t)~0xfc, /* 0x04 */
- (uint8_t)~0x84, /* 0x05 */
- (uint8_t)~0xf0, /* 0x06 */
- (uint8_t)~0xf0, /* 0x07 */
- (uint8_t)~0x00, /* 0x08 */
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -82,12 +70,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->gr_index = val;
break;
- case 0x3cf: /* graphics data register */
- rc = (s->gr_index < sizeof(s->gr));
- if ( rc )
- s->gr[s->gr_index] = val & gr_mask[s->gr_index];
- break;
-
default:
rc = 0;
break;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 19ecf4fd78e3..6a34ea82f402 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -114,7 +114,6 @@ struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
- uint8_t gr[9];
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "sr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static sr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 7aba44bdd78aedb97703811948c3b69ccff85032)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index fa25833caa50..5523a441dd8f 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -37,18 +37,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-/* force some bits to zero */
-static const uint8_t sr_mask[8] = {
- (uint8_t)~0xfc,
- (uint8_t)~0xc2,
- (uint8_t)~0xf0,
- (uint8_t)~0xc0,
- (uint8_t)~0xf1,
- (uint8_t)~0xff,
- (uint8_t)~0xff,
- (uint8_t)~0x00,
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -60,12 +48,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->sr_index = val;
break;
- case 0x3c5: /* sequencer data register */
- rc = (s->sr_index < sizeof(s->sr));
- if ( rc )
- s->sr[s->sr_index] = val & sr_mask[s->sr_index] ;
- break;
-
case 0x3ce: /* graphics address register */
s->gr_index = val;
break;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 6a34ea82f402..d8310f0fe492 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -112,7 +112,6 @@ struct vpci_arch_msix_entry {
struct hvm_hw_stdvga {
uint8_t sr_index;
- uint8_t sr[8];
uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "{g,s}r_index" struct members
No consumers are left, hence the producer and the fields themselves can
also go away. stdvga_outb() is then useless, rendering stdvga_out()
useless as well. Hence the entire I/O port intercept can go away.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 86c03372e107f5c18266a62281663861b1144929)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 5523a441dd8f..155a67a438d8 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -37,62 +37,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-static int stdvga_outb(uint64_t addr, uint8_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1;
-
- switch ( addr )
- {
- case 0x3c4: /* sequencer address register */
- s->sr_index = val;
- break;
-
- case 0x3ce: /* graphics address register */
- s->gr_index = val;
- break;
-
- default:
- rc = 0;
- break;
- }
-
- return rc;
-}
-
-static void stdvga_out(uint32_t port, uint32_t bytes, uint32_t val)
-{
- switch ( bytes )
- {
- case 1:
- stdvga_outb(port, val);
- break;
-
- case 2:
- stdvga_outb(port + 0, val >> 0);
- stdvga_outb(port + 1, val >> 8);
- break;
-
- default:
- break;
- }
-}
-
-static int cf_check stdvga_intercept_pio(
- int dir, unsigned int port, unsigned int bytes, uint32_t *val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- if ( dir == IOREQ_WRITE )
- {
- spin_lock(&s->lock);
- stdvga_out(port, bytes, *val);
- spin_unlock(&s->lock);
- }
-
- return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
-}
-
static int cf_check stdvga_mem_read(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t *p_data)
@@ -194,11 +138,6 @@ void stdvga_init(struct domain *d)
{
struct hvm_io_handler *handler;
- /* Sequencer registers. */
- register_portio_handler(d, 0x3c4, 2, stdvga_intercept_pio);
- /* Graphics registers. */
- register_portio_handler(d, 0x3ce, 2, stdvga_intercept_pio);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index d8310f0fe492..ec55c93d2f2e 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -111,8 +111,6 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- uint8_t sr_index;
- uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "vram_page[]" struct member
No uses are left, hence its setup, teardown, and the field itself can
also go away. stdvga_deinit() is then empty and can be dropped as well.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 3beb4baf2a0a2eef40d39eb7e6eecbfd36da5d14)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8f293f4feb65..3662e23cb604 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -685,7 +685,6 @@ int hvm_domain_initialise(struct domain *d,
return 0;
fail2:
- stdvga_deinit(d);
vioapic_deinit(d);
fail1:
if ( is_hardware_domain(d) )
@@ -748,7 +747,6 @@ void hvm_domain_destroy(struct domain *d)
if ( hvm_funcs.domain_destroy )
alternative_vcall(hvm_funcs.domain_destroy, d);
- stdvga_deinit(d);
vioapic_deinit(d);
XFREE(d->arch.hvm.pl_time);
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 155a67a438d8..9f308fc89679 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -116,8 +116,7 @@ static const struct hvm_io_ops stdvga_mem_ops = {
void stdvga_init(struct domain *d)
{
struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- struct page_info *pg;
- unsigned int i;
+ struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
@@ -125,47 +124,15 @@ void stdvga_init(struct domain *d)
memset(s, 0, sizeof(*s));
spin_lock_init(&s->lock);
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
+ /* VGA memory */
+ handler = hvm_next_io_handler(d);
+ if ( handler )
{
- pg = alloc_domheap_page(d, MEMF_no_owner);
- if ( pg == NULL )
- break;
- s->vram_page[i] = pg;
- clear_domain_page(page_to_mfn(pg));
- }
-
- if ( i == ARRAY_SIZE(s->vram_page) )
- {
- struct hvm_io_handler *handler;
-
- /* VGA memory */
- handler = hvm_next_io_handler(d);
-
- if ( handler == NULL )
- return;
-
handler->type = IOREQ_TYPE_COPY;
handler->ops = &stdvga_mem_ops;
}
}
-void stdvga_deinit(struct domain *d)
-{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- int i;
-
- if ( !has_vvga(d) )
- return;
-
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
- {
- if ( s->vram_page[i] == NULL )
- continue;
- free_domheap_page(s->vram_page[i]);
- s->vram_page[i] = NULL;
- }
-}
-
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index ec55c93d2f2e..958077de81bd 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -111,12 +111,10 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
void stdvga_init(struct domain *d);
-void stdvga_deinit(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "lock" struct member
No state is left to protect. It being the last field, drop the struct
itself as well. Similarly for then ending up empty, drop the .complete
handler.
This is part of XSA-463 / CVE-2024-45818
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b180a50326c8a2c171f37c1940a0fbbdcad4be90)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 9f308fc89679..d38d30affbff 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -69,61 +69,35 @@ static int cf_check stdvga_mem_write(
static bool cf_check stdvga_mem_accept(
const struct hvm_io_handler *handler, const ioreq_t *p)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- /*
- * The range check must be done without taking the lock, to avoid
- * deadlock when hvm_mmio_internal() is called from
- * hvm_copy_to/from_guest_phys() in hvm_process_io_intercept().
- */
if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
return 0;
- spin_lock(&s->lock);
-
if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
* Only accept single direct writes, as that's the only thing we can
* accelerate using buffered ioreq handling.
*/
- goto reject;
+ return false;
}
- /* s->lock intentionally held */
- return 1;
-
- reject:
- spin_unlock(&s->lock);
- return 0;
-}
-
-static void cf_check stdvga_mem_complete(const struct hvm_io_handler *handler)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- spin_unlock(&s->lock);
+ return true;
}
static const struct hvm_io_ops stdvga_mem_ops = {
.accept = stdvga_mem_accept,
.read = stdvga_mem_read,
.write = stdvga_mem_write,
- .complete = stdvga_mem_complete
};
void stdvga_init(struct domain *d)
{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
- memset(s, 0, sizeof(*s));
- spin_lock_init(&s->lock);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
if ( handler )
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index dd9d837e8431..333501d5f2ac 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -72,7 +72,6 @@ struct hvm_domain {
struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */
struct hvm_vioapic **vioapic;
unsigned int nr_vioapics;
- struct hvm_hw_stdvga stdvga;
/*
* hvm_hw_pmtimer is a publicly-visible name. We will defer renaming
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 958077de81bd..d123e7c9edd8 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -110,10 +110,6 @@ struct vpci_arch_msix_entry {
int pirq;
};
-struct hvm_hw_stdvga {
- spinlock_t lock;
-};
-
void stdvga_init(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: x86/hvm: Simplify stdvga_mem_accept() further
stdvga_mem_accept() is called on almost all IO emulations, and the
overwhelming likely answer is to reject the ioreq. Simply rearranging the
expression yields an improvement:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
Function old new delta
stdvga_mem_accept 109 52 -57
which is best explained looking at the disassembly:
Before: After:
f3 0f 1e fa endbr64 f3 0f 1e fa endbr64
0f b6 4e 1e movzbl 0x1e(%rsi),%ecx | 0f b6 46 1e movzbl 0x1e(%rsi),%eax
48 8b 16 mov (%rsi),%rdx | 31 d2 xor %edx,%edx
f6 c1 40 test $0x40,%cl | a8 30 test $0x30,%al
75 38 jne <stdvga_mem_accept+0x48> | 75 23 jne <stdvga_mem_accept+0x31>
31 c0 xor %eax,%eax <
48 81 fa ff ff 09 00 cmp $0x9ffff,%rdx <
76 26 jbe <stdvga_mem_accept+0x41> <
8b 46 14 mov 0x14(%rsi),%eax <
8b 7e 10 mov 0x10(%rsi),%edi <
48 0f af c7 imul %rdi,%rax <
48 8d 54 02 ff lea -0x1(%rdx,%rax,1),%rdx <
31 c0 xor %eax,%eax <
48 81 fa ff ff 0b 00 cmp $0xbffff,%rdx <
77 0c ja <stdvga_mem_accept+0x41> <
83 e1 30 and $0x30,%ecx <
75 07 jne <stdvga_mem_accept+0x41> <
83 7e 10 01 cmpl $0x1,0x10(%rsi) 83 7e 10 01 cmpl $0x1,0x10(%rsi)
0f 94 c0 sete %al | 75 1d jne <stdvga_mem_accept+0x31>
c3 ret | 48 8b 0e mov (%rsi),%rcx
66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) | 48 81 f9 ff ff 09 00 cmp $0x9ffff,%rcx
8b 46 10 mov 0x10(%rsi),%eax | 76 11 jbe <stdvga_mem_accept+0x31>
8b 7e 14 mov 0x14(%rsi),%edi | 8b 46 14 mov 0x14(%rsi),%eax
49 89 d0 mov %rdx,%r8 | 48 8d 44 01 ff lea -0x1(%rcx,%rax,1),%rax
48 83 e8 01 sub $0x1,%rax | 48 3d ff ff 0b 00 cmp $0xbffff,%rax
48 8d 54 3a ff lea -0x1(%rdx,%rdi,1),%rdx | 0f 96 c2 setbe %dl
48 0f af c7 imul %rdi,%rax | 89 d0 mov %edx,%eax
49 29 c0 sub %rax,%r8 <
31 c0 xor %eax,%eax <
49 81 f8 ff ff 09 00 cmp $0x9ffff,%r8 <
77 be ja <stdvga_mem_accept+0x2a> <
c3 ret c3 ret
By moving the "p->count != 1" check ahead of the
ioreq_mmio_{first,last}_byte() calls, both multiplies disappear along with a
lot of surrounding logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(cherry picked from commit 08ffd8705d36c7c445df3ecee8ad9b8f8d65fbe0)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index d38d30affbff..c3c43f59eead 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -69,18 +69,14 @@ static int cf_check stdvga_mem_write(
static bool cf_check stdvga_mem_accept(
const struct hvm_io_handler *handler, const ioreq_t *p)
{
- if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
+ /*
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
+ */
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 ||
+ (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
- return 0;
-
- if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
- {
- /*
- * Only accept single direct writes, as that's the only thing we can
- * accelerate using buffered ioreq handling.
- */
return false;
- }
return true;
}
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "cache" struct member
Since 68e1183411be ("libxc: introduce a xc_dom_arch for hvm-3.0-x86_32
guests"), HVM guests are built using XEN_DOMCTL_sethvmcontext, which
ends up disabling stdvga caching because of arch_hvm_load() being
involved in the processing of the request. With that the field is
useless, and can be dropped. Drop the helper functions manipulating /
checking as well right away, but leave the use sites of
stdvga_cache_is_enabled() with the hard-coded result the function would
have produced, to aid validation of subsequent dropping of further code.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 53b7246bdfb3c280adcdf714918e4decb7e108f4)
diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
index 99aaf3fc335b..8ab64057064e 100644
--- a/xen/arch/x86/hvm/save.c
+++ b/xen/arch/x86/hvm/save.c
@@ -69,9 +69,6 @@ static void arch_hvm_load(struct domain *d, const struct hvm_save_header *hdr)
/* Time when restore started */
d->arch.hvm.sync_tsc = rdtsc();
-
- /* VGA state is not saved/restored, so we nobble the cache. */
- d->arch.hvm.stdvga.cache = STDVGA_CACHE_DISABLED;
}
/* List of handlers for various HVM save and restore types */
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 5f02d886153d..2520d0dd0184 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -100,37 +100,6 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
unmap_domain_page(p);
}
-static void stdvga_try_cache_enable(struct hvm_hw_stdvga *s)
-{
- /*
- * Caching mode can only be enabled if the the cache has
- * never been used before. As soon as it is disabled, it will
- * become out-of-sync with the VGA device model and since no
- * mechanism exists to acquire current VRAM state from the
- * device model, re-enabling it would lead to stale data being
- * seen by the guest.
- */
- if ( s->cache != STDVGA_CACHE_UNINITIALIZED )
- return;
-
- gdprintk(XENLOG_INFO, "entering caching mode\n");
- s->cache = STDVGA_CACHE_ENABLED;
-}
-
-static void stdvga_cache_disable(struct hvm_hw_stdvga *s)
-{
- if ( s->cache != STDVGA_CACHE_ENABLED )
- return;
-
- gdprintk(XENLOG_INFO, "leaving caching mode\n");
- s->cache = STDVGA_CACHE_DISABLED;
-}
-
-static bool stdvga_cache_is_enabled(const struct hvm_hw_stdvga *s)
-{
- return s->cache == STDVGA_CACHE_ENABLED;
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -170,7 +139,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
if ( !prev_stdvga && s->stdvga )
{
gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- stdvga_try_cache_enable(s);
}
else if ( prev_stdvga && !s->stdvga )
{
@@ -468,7 +436,7 @@ static int cf_check stdvga_mem_write(
};
struct ioreq_server *srv;
- if ( !stdvga_cache_is_enabled(s) || !s->stdvga )
+ if ( true || !s->stdvga )
goto done;
/* Intercept mmio write */
@@ -536,18 +504,12 @@ static bool cf_check stdvga_mem_accept(
* We cannot return X86EMUL_UNHANDLEABLE on anything other then the
* first cycle of an I/O. So, since we cannot guarantee to always be
* able to send buffered writes, we have to reject any multi-cycle
- * or "indirect" I/O and, since we are rejecting an I/O, we must
- * invalidate the cache.
- * Single-cycle write transactions are accepted even if the cache is
- * not active since we can assert, when in stdvga mode, that writes
- * to VRAM have no side effect and thus we can try to buffer them.
+ * or "indirect" I/O.
*/
- stdvga_cache_disable(s);
-
goto reject;
}
else if ( p->dir == IOREQ_READ &&
- (!stdvga_cache_is_enabled(s) || !s->stdvga) )
+ (true || !s->stdvga) )
goto reject;
/* s->lock intentionally held */
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 24d1b6134f02..ce171eaca491 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -110,19 +110,12 @@ struct vpci_arch_msix_entry {
int pirq;
};
-enum stdvga_cache_state {
- STDVGA_CACHE_UNINITIALIZED,
- STDVGA_CACHE_ENABLED,
- STDVGA_CACHE_DISABLED
-};
-
struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
bool stdvga;
- enum stdvga_cache_state cache;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "stdvga" struct member
Two of its consumers are dead (in compile-time constant conditionals)
and the only remaining ones are merely controlling debug logging. Hence
the field is now pointless to set, which in particular allows to get rid
of the questionable conditional from which the field's value was
established (afaict 551ceee97513 ["x86, hvm: stdvga cache always on"]
had dropped too much of the earlier extra check that was there, and
quite likely further checks were missing).
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b740a9369e81bdda675a9780130ce2b9e75d4ec9)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 2520d0dd0184..8a9ce0534658 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -103,7 +103,7 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1, prev_stdvga = s->stdvga;
+ int rc = 1;
switch ( addr )
{
@@ -132,19 +132,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
break;
}
- /* When in standard vga mode, emulate here all writes to the vram buffer
- * so we can immediately satisfy reads without waiting for qemu. */
- s->stdvga = (s->sr[7] == 0x00);
-
- if ( !prev_stdvga && s->stdvga )
- {
- gdprintk(XENLOG_INFO, "entering stdvga mode\n");
- }
- else if ( prev_stdvga && !s->stdvga )
- {
- gdprintk(XENLOG_INFO, "leaving stdvga mode\n");
- }
-
return rc;
}
@@ -425,7 +412,6 @@ static int cf_check stdvga_mem_write(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t data)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = addr,
@@ -436,8 +422,7 @@ static int cf_check stdvga_mem_write(
};
struct ioreq_server *srv;
- if ( true || !s->stdvga )
- goto done;
+ goto done;
/* Intercept mmio write */
switch ( size )
@@ -498,19 +483,14 @@ static bool cf_check stdvga_mem_accept(
spin_lock(&s->lock);
- if ( p->dir == IOREQ_WRITE && (p->data_is_ptr || p->count != 1) )
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
- * We cannot return X86EMUL_UNHANDLEABLE on anything other then the
- * first cycle of an I/O. So, since we cannot guarantee to always be
- * able to send buffered writes, we have to reject any multi-cycle
- * or "indirect" I/O.
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
*/
goto reject;
}
- else if ( p->dir == IOREQ_READ &&
- (true || !s->stdvga) )
- goto reject;
/* s->lock intentionally held */
return 1;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index ce171eaca491..67f4d033a7f1 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -115,7 +115,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- bool stdvga;
uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: remove unused MMIO handling code
All read accesses are rejected by the ->accept handler, while writes
bypass the bulk of the function body. Drop the dead code, leaving an
assertion in the read handler.
A number of other static items (and a macro) are then unreferenced and
hence also need (want) dropping. The same applies to the "latch" field
of the state structure.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 89108547af1f230b72893b48351f9c1106189649)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 8a9ce0534658..0f0bd100681b 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -37,26 +37,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-#define PAT(x) (x)
-static const uint32_t mask16[16] = {
- PAT(0x00000000U),
- PAT(0x000000ffU),
- PAT(0x0000ff00U),
- PAT(0x0000ffffU),
- PAT(0x00ff0000U),
- PAT(0x00ff00ffU),
- PAT(0x00ffff00U),
- PAT(0x00ffffffU),
- PAT(0xff000000U),
- PAT(0xff0000ffU),
- PAT(0xff00ff00U),
- PAT(0xff00ffffU),
- PAT(0xffff0000U),
- PAT(0xffff00ffU),
- PAT(0xffffff00U),
- PAT(0xffffffffU),
-};
-
/* force some bits to zero */
static const uint8_t sr_mask[8] = {
(uint8_t)~0xfc,
@@ -81,25 +61,6 @@ static const uint8_t gr_mask[9] = {
(uint8_t)~0x00, /* 0x08 */
};
-static uint8_t *vram_getb(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 12) & 0x3f];
- uint8_t *p = __map_domain_page(pg);
- return &p[a & 0xfff];
-}
-
-static uint32_t *vram_getl(struct hvm_hw_stdvga *s, unsigned int a)
-{
- struct page_info *pg = s->vram_page[(a >> 10) & 0x3f];
- uint32_t *p = __map_domain_page(pg);
- return &p[a & 0x3ff];
-}
-
-static void vram_put(struct hvm_hw_stdvga *s, void *p)
-{
- unmap_domain_page(p);
-}
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -168,244 +129,13 @@ static int cf_check stdvga_intercept_pio(
return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
}
-static unsigned int stdvga_mem_offset(
- struct hvm_hw_stdvga *s, unsigned int mmio_addr)
-{
- unsigned int memory_map_mode = (s->gr[6] >> 2) & 3;
- unsigned int offset = mmio_addr & 0x1ffff;
-
- switch ( memory_map_mode )
- {
- case 0:
- break;
- case 1:
- if ( offset >= 0x10000 )
- goto fail;
- offset += 0; /* assume bank_offset == 0; */
- break;
- case 2:
- offset -= 0x10000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- default:
- case 3:
- offset -= 0x18000;
- if ( offset >= 0x8000 )
- goto fail;
- break;
- }
-
- return offset;
-
- fail:
- return ~0u;
-}
-
-#define GET_PLANE(data, p) (((data) >> ((p) * 8)) & 0xff)
-
-static uint8_t stdvga_mem_readb(uint64_t addr)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane;
- uint32_t ret, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return 0xff;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- vram_b = vram_getb(s, addr);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- vram_b = vram_getb(s, ((addr & ~1) << 1) | plane);
- ret = *vram_b;
- vram_put(s, vram_b);
- }
- else
- {
- /* standard VGA latched access */
- vram_l = vram_getl(s, addr);
- s->latch = *vram_l;
- vram_put(s, vram_l);
-
- if ( !(s->gr[5] & 0x08) )
- {
- /* read mode 0 */
- plane = s->gr[4];
- ret = GET_PLANE(s->latch, plane);
- }
- else
- {
- /* read mode 1 */
- ret = (s->latch ^ mask16[s->gr[2]]) & mask16[s->gr[7]];
- ret |= ret >> 16;
- ret |= ret >> 8;
- ret = (~ret) & 0xff;
- }
- }
-
- return ret;
-}
-
static int cf_check stdvga_mem_read(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t *p_data)
{
- uint64_t data = ~0UL;
-
- switch ( size )
- {
- case 1:
- data = stdvga_mem_readb(addr);
- break;
-
- case 2:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- break;
-
- case 4:
- data = stdvga_mem_readb(addr);
- data |= stdvga_mem_readb(addr + 1) << 8;
- data |= stdvga_mem_readb(addr + 2) << 16;
- data |= (uint32_t)stdvga_mem_readb(addr + 3) << 24;
- break;
-
- case 8:
- data = (uint64_t)(stdvga_mem_readb(addr));
- data |= (uint64_t)(stdvga_mem_readb(addr + 1)) << 8;
- data |= (uint64_t)(stdvga_mem_readb(addr + 2)) << 16;
- data |= (uint64_t)(stdvga_mem_readb(addr + 3)) << 24;
- data |= (uint64_t)(stdvga_mem_readb(addr + 4)) << 32;
- data |= (uint64_t)(stdvga_mem_readb(addr + 5)) << 40;
- data |= (uint64_t)(stdvga_mem_readb(addr + 6)) << 48;
- data |= (uint64_t)(stdvga_mem_readb(addr + 7)) << 56;
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
-
- *p_data = data;
- return X86EMUL_OKAY;
-}
-
-static void stdvga_mem_writeb(uint64_t addr, uint32_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int plane, write_mode, b, func_select, mask;
- uint32_t write_mask, bit_mask, set_mask, *vram_l;
- uint8_t *vram_b;
-
- addr = stdvga_mem_offset(s, addr);
- if ( addr == ~0u )
- return;
-
- if ( s->sr[4] & 0x08 )
- {
- /* chain 4 mode : simplest access */
- plane = addr & 3;
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else if ( s->gr[5] & 0x10 )
- {
- /* odd/even mode (aka text mode mapping) */
- plane = (s->gr[4] & 2) | (addr & 1);
- mask = (1 << plane);
- if ( s->sr[2] & mask )
- {
- addr = ((addr & ~1) << 1) | plane;
- vram_b = vram_getb(s, addr);
- *vram_b = val;
- vram_put(s, vram_b);
- }
- }
- else
- {
- write_mode = s->gr[5] & 3;
- switch ( write_mode )
- {
- default:
- case 0:
- /* rotate */
- b = s->gr[3] & 7;
- val = ((val >> b) | (val << (8 - b))) & 0xff;
- val |= val << 8;
- val |= val << 16;
-
- /* apply set/reset mask */
- set_mask = mask16[s->gr[1]];
- val = (val & ~set_mask) | (mask16[s->gr[0]] & set_mask);
- bit_mask = s->gr[8];
- break;
- case 1:
- val = s->latch;
- goto do_write;
- case 2:
- val = mask16[val & 0x0f];
- bit_mask = s->gr[8];
- break;
- case 3:
- /* rotate */
- b = s->gr[3] & 7;
- val = (val >> b) | (val << (8 - b));
-
- bit_mask = s->gr[8] & val;
- val = mask16[s->gr[0]];
- break;
- }
-
- /* apply logical operation */
- func_select = s->gr[3] >> 3;
- switch ( func_select )
- {
- case 0:
- default:
- /* nothing to do */
- break;
- case 1:
- /* and */
- val &= s->latch;
- break;
- case 2:
- /* or */
- val |= s->latch;
- break;
- case 3:
- /* xor */
- val ^= s->latch;
- break;
- }
-
- /* apply bit mask */
- bit_mask |= bit_mask << 8;
- bit_mask |= bit_mask << 16;
- val = (val & bit_mask) | (s->latch & ~bit_mask);
-
- do_write:
- /* mask data according to sr[2] */
- mask = s->sr[2];
- write_mask = mask16[mask];
- vram_l = vram_getl(s, addr);
- *vram_l = (*vram_l & ~write_mask) | (val & write_mask);
- vram_put(s, vram_l);
- }
+ ASSERT_UNREACHABLE();
+ *p_data = ~0;
+ return X86EMUL_UNHANDLEABLE;
}
static int cf_check stdvga_mem_write(
@@ -420,47 +150,8 @@ static int cf_check stdvga_mem_write(
.dir = IOREQ_WRITE,
.data = data,
};
- struct ioreq_server *srv;
-
- goto done;
-
- /* Intercept mmio write */
- switch ( size )
- {
- case 1:
- stdvga_mem_writeb(addr, (data >> 0) & 0xff);
- break;
-
- case 2:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- break;
-
- case 4:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- break;
-
- case 8:
- stdvga_mem_writeb(addr+0, (data >> 0) & 0xff);
- stdvga_mem_writeb(addr+1, (data >> 8) & 0xff);
- stdvga_mem_writeb(addr+2, (data >> 16) & 0xff);
- stdvga_mem_writeb(addr+3, (data >> 24) & 0xff);
- stdvga_mem_writeb(addr+4, (data >> 32) & 0xff);
- stdvga_mem_writeb(addr+5, (data >> 40) & 0xff);
- stdvga_mem_writeb(addr+6, (data >> 48) & 0xff);
- stdvga_mem_writeb(addr+7, (data >> 56) & 0xff);
- break;
-
- default:
- gdprintk(XENLOG_WARNING, "invalid io size: %u\n", size);
- break;
- }
+ struct ioreq_server *srv = ioreq_server_select(current->domain, &p);
- done:
- srv = ioreq_server_select(current->domain, &p);
if ( !srv )
return X86EMUL_UNHANDLEABLE;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 67f4d033a7f1..91714f36140e 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -115,7 +115,6 @@ struct hvm_hw_stdvga {
uint8_t sr[8];
uint8_t gr_index;
uint8_t gr[9];
- uint32_t latch;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "gr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static gr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b16c0966a17f19c0e55ed0b9baa28191d2590178)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 0f0bd100681b..fa25833caa50 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -49,18 +49,6 @@ static const uint8_t sr_mask[8] = {
(uint8_t)~0x00,
};
-static const uint8_t gr_mask[9] = {
- (uint8_t)~0xf0, /* 0x00 */
- (uint8_t)~0xf0, /* 0x01 */
- (uint8_t)~0xf0, /* 0x02 */
- (uint8_t)~0xe0, /* 0x03 */
- (uint8_t)~0xfc, /* 0x04 */
- (uint8_t)~0x84, /* 0x05 */
- (uint8_t)~0xf0, /* 0x06 */
- (uint8_t)~0xf0, /* 0x07 */
- (uint8_t)~0x00, /* 0x08 */
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -82,12 +70,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->gr_index = val;
break;
- case 0x3cf: /* graphics data register */
- rc = (s->gr_index < sizeof(s->gr));
- if ( rc )
- s->gr[s->gr_index] = val & gr_mask[s->gr_index];
- break;
-
default:
rc = 0;
break;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 91714f36140e..126622e53c3e 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -114,7 +114,6 @@ struct hvm_hw_stdvga {
uint8_t sr_index;
uint8_t sr[8];
uint8_t gr_index;
- uint8_t gr[9];
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "sr[]" struct member
No consumers are left, hence the producer and the array itself can also
go away. The static sr_mask[] is then orphaned and hence needs dropping,
too.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 7aba44bdd78aedb97703811948c3b69ccff85032)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index fa25833caa50..5523a441dd8f 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -37,18 +37,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-/* force some bits to zero */
-static const uint8_t sr_mask[8] = {
- (uint8_t)~0xfc,
- (uint8_t)~0xc2,
- (uint8_t)~0xf0,
- (uint8_t)~0xc0,
- (uint8_t)~0xf1,
- (uint8_t)~0xff,
- (uint8_t)~0xff,
- (uint8_t)~0x00,
-};
-
static int stdvga_outb(uint64_t addr, uint8_t val)
{
struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
@@ -60,12 +48,6 @@ static int stdvga_outb(uint64_t addr, uint8_t val)
s->sr_index = val;
break;
- case 0x3c5: /* sequencer data register */
- rc = (s->sr_index < sizeof(s->sr));
- if ( rc )
- s->sr[s->sr_index] = val & sr_mask[s->sr_index] ;
- break;
-
case 0x3ce: /* graphics address register */
s->gr_index = val;
break;
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 126622e53c3e..3e9079eab62c 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -112,7 +112,6 @@ struct vpci_arch_msix_entry {
struct hvm_hw_stdvga {
uint8_t sr_index;
- uint8_t sr[8];
uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "{g,s}r_index" struct members
No consumers are left, hence the producer and the fields themselves can
also go away. stdvga_outb() is then useless, rendering stdvga_out()
useless as well. Hence the entire I/O port intercept can go away.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 86c03372e107f5c18266a62281663861b1144929)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 5523a441dd8f..155a67a438d8 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -37,62 +37,6 @@
#define VGA_MEM_BASE 0xa0000
#define VGA_MEM_SIZE 0x20000
-static int stdvga_outb(uint64_t addr, uint8_t val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
- int rc = 1;
-
- switch ( addr )
- {
- case 0x3c4: /* sequencer address register */
- s->sr_index = val;
- break;
-
- case 0x3ce: /* graphics address register */
- s->gr_index = val;
- break;
-
- default:
- rc = 0;
- break;
- }
-
- return rc;
-}
-
-static void stdvga_out(uint32_t port, uint32_t bytes, uint32_t val)
-{
- switch ( bytes )
- {
- case 1:
- stdvga_outb(port, val);
- break;
-
- case 2:
- stdvga_outb(port + 0, val >> 0);
- stdvga_outb(port + 1, val >> 8);
- break;
-
- default:
- break;
- }
-}
-
-static int cf_check stdvga_intercept_pio(
- int dir, unsigned int port, unsigned int bytes, uint32_t *val)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- if ( dir == IOREQ_WRITE )
- {
- spin_lock(&s->lock);
- stdvga_out(port, bytes, *val);
- spin_unlock(&s->lock);
- }
-
- return X86EMUL_UNHANDLEABLE; /* propagate to external ioemu */
-}
-
static int cf_check stdvga_mem_read(
const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
uint64_t *p_data)
@@ -194,11 +138,6 @@ void stdvga_init(struct domain *d)
{
struct hvm_io_handler *handler;
- /* Sequencer registers. */
- register_portio_handler(d, 0x3c4, 2, stdvga_intercept_pio);
- /* Graphics registers. */
- register_portio_handler(d, 0x3ce, 2, stdvga_intercept_pio);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 3e9079eab62c..bf9ddfc70e1e 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -111,8 +111,6 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- uint8_t sr_index;
- uint8_t gr_index;
struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "vram_page[]" struct member
No uses are left, hence its setup, teardown, and the field itself can
also go away. stdvga_deinit() is then empty and can be dropped as well.
This is part of XSA-463 / CVE-2024-45818
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 3beb4baf2a0a2eef40d39eb7e6eecbfd36da5d14)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0fe2b85b1653..fbca7e49495a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -700,7 +700,6 @@ int hvm_domain_initialise(struct domain *d,
return 0;
fail2:
- stdvga_deinit(d);
vioapic_deinit(d);
fail1:
if ( is_hardware_domain(d) )
@@ -763,7 +762,6 @@ void hvm_domain_destroy(struct domain *d)
if ( hvm_funcs.domain_destroy )
alternative_vcall(hvm_funcs.domain_destroy, d);
- stdvga_deinit(d);
vioapic_deinit(d);
XFREE(d->arch.hvm.pl_time);
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 155a67a438d8..9f308fc89679 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -116,8 +116,7 @@ static const struct hvm_io_ops stdvga_mem_ops = {
void stdvga_init(struct domain *d)
{
struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- struct page_info *pg;
- unsigned int i;
+ struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
@@ -125,47 +124,15 @@ void stdvga_init(struct domain *d)
memset(s, 0, sizeof(*s));
spin_lock_init(&s->lock);
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
+ /* VGA memory */
+ handler = hvm_next_io_handler(d);
+ if ( handler )
{
- pg = alloc_domheap_page(d, MEMF_no_owner);
- if ( pg == NULL )
- break;
- s->vram_page[i] = pg;
- clear_domain_page(page_to_mfn(pg));
- }
-
- if ( i == ARRAY_SIZE(s->vram_page) )
- {
- struct hvm_io_handler *handler;
-
- /* VGA memory */
- handler = hvm_next_io_handler(d);
-
- if ( handler == NULL )
- return;
-
handler->type = IOREQ_TYPE_COPY;
handler->ops = &stdvga_mem_ops;
}
}
-void stdvga_deinit(struct domain *d)
-{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
- int i;
-
- if ( !has_vvga(d) )
- return;
-
- for ( i = 0; i != ARRAY_SIZE(s->vram_page); i++ )
- {
- if ( s->vram_page[i] == NULL )
- continue;
- free_domheap_page(s->vram_page[i]);
- s->vram_page[i] = NULL;
- }
-}
-
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index bf9ddfc70e1e..d49f6d6f8c06 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -111,12 +111,10 @@ struct vpci_arch_msix_entry {
};
struct hvm_hw_stdvga {
- struct page_info *vram_page[64]; /* shadow of 0xa0000-0xaffff */
spinlock_t lock;
};
void stdvga_init(struct domain *d);
-void stdvga_deinit(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/HVM: drop stdvga's "lock" struct member
No state is left to protect. It being the last field, drop the struct
itself as well. Similarly for then ending up empty, drop the .complete
handler.
This is part of XSA-463 / CVE-2024-45818
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit b180a50326c8a2c171f37c1940a0fbbdcad4be90)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 9f308fc89679..d38d30affbff 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -69,61 +69,35 @@ static int cf_check stdvga_mem_write(
static bool cf_check stdvga_mem_accept(
const struct hvm_io_handler *handler, const ioreq_t *p)
{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- /*
- * The range check must be done without taking the lock, to avoid
- * deadlock when hvm_mmio_internal() is called from
- * hvm_copy_to/from_guest_phys() in hvm_process_io_intercept().
- */
if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
return 0;
- spin_lock(&s->lock);
-
if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
{
/*
* Only accept single direct writes, as that's the only thing we can
* accelerate using buffered ioreq handling.
*/
- goto reject;
+ return false;
}
- /* s->lock intentionally held */
- return 1;
-
- reject:
- spin_unlock(&s->lock);
- return 0;
-}
-
-static void cf_check stdvga_mem_complete(const struct hvm_io_handler *handler)
-{
- struct hvm_hw_stdvga *s = ¤t->domain->arch.hvm.stdvga;
-
- spin_unlock(&s->lock);
+ return true;
}
static const struct hvm_io_ops stdvga_mem_ops = {
.accept = stdvga_mem_accept,
.read = stdvga_mem_read,
.write = stdvga_mem_write,
- .complete = stdvga_mem_complete
};
void stdvga_init(struct domain *d)
{
- struct hvm_hw_stdvga *s = &d->arch.hvm.stdvga;
struct hvm_io_handler *handler;
if ( !has_vvga(d) )
return;
- memset(s, 0, sizeof(*s));
- spin_lock_init(&s->lock);
-
/* VGA memory */
handler = hvm_next_io_handler(d);
if ( handler )
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index dd9d837e8431..333501d5f2ac 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -72,7 +72,6 @@ struct hvm_domain {
struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */
struct hvm_vioapic **vioapic;
unsigned int nr_vioapics;
- struct hvm_hw_stdvga stdvga;
/*
* hvm_hw_pmtimer is a publicly-visible name. We will defer renaming
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index d49f6d6f8c06..d72b29f73fa2 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -110,10 +110,6 @@ struct vpci_arch_msix_entry {
int pirq;
};
-struct hvm_hw_stdvga {
- spinlock_t lock;
-};
-
void stdvga_init(struct domain *d);
extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: x86/hvm: Simplify stdvga_mem_accept() further
stdvga_mem_accept() is called on almost all IO emulations, and the
overwhelming likely answer is to reject the ioreq. Simply rearranging the
expression yields an improvement:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
Function old new delta
stdvga_mem_accept 109 52 -57
which is best explained looking at the disassembly:
Before: After:
f3 0f 1e fa endbr64 f3 0f 1e fa endbr64
0f b6 4e 1e movzbl 0x1e(%rsi),%ecx | 0f b6 46 1e movzbl 0x1e(%rsi),%eax
48 8b 16 mov (%rsi),%rdx | 31 d2 xor %edx,%edx
f6 c1 40 test $0x40,%cl | a8 30 test $0x30,%al
75 38 jne <stdvga_mem_accept+0x48> | 75 23 jne <stdvga_mem_accept+0x31>
31 c0 xor %eax,%eax <
48 81 fa ff ff 09 00 cmp $0x9ffff,%rdx <
76 26 jbe <stdvga_mem_accept+0x41> <
8b 46 14 mov 0x14(%rsi),%eax <
8b 7e 10 mov 0x10(%rsi),%edi <
48 0f af c7 imul %rdi,%rax <
48 8d 54 02 ff lea -0x1(%rdx,%rax,1),%rdx <
31 c0 xor %eax,%eax <
48 81 fa ff ff 0b 00 cmp $0xbffff,%rdx <
77 0c ja <stdvga_mem_accept+0x41> <
83 e1 30 and $0x30,%ecx <
75 07 jne <stdvga_mem_accept+0x41> <
83 7e 10 01 cmpl $0x1,0x10(%rsi) 83 7e 10 01 cmpl $0x1,0x10(%rsi)
0f 94 c0 sete %al | 75 1d jne <stdvga_mem_accept+0x31>
c3 ret | 48 8b 0e mov (%rsi),%rcx
66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) | 48 81 f9 ff ff 09 00 cmp $0x9ffff,%rcx
8b 46 10 mov 0x10(%rsi),%eax | 76 11 jbe <stdvga_mem_accept+0x31>
8b 7e 14 mov 0x14(%rsi),%edi | 8b 46 14 mov 0x14(%rsi),%eax
49 89 d0 mov %rdx,%r8 | 48 8d 44 01 ff lea -0x1(%rcx,%rax,1),%rax
48 83 e8 01 sub $0x1,%rax | 48 3d ff ff 0b 00 cmp $0xbffff,%rax
48 8d 54 3a ff lea -0x1(%rdx,%rdi,1),%rdx | 0f 96 c2 setbe %dl
48 0f af c7 imul %rdi,%rax | 89 d0 mov %edx,%eax
49 29 c0 sub %rax,%r8 <
31 c0 xor %eax,%eax <
49 81 f8 ff ff 09 00 cmp $0x9ffff,%r8 <
77 be ja <stdvga_mem_accept+0x2a> <
c3 ret c3 ret
By moving the "p->count != 1" check ahead of the
ioreq_mmio_{first,last}_byte() calls, both multiplies disappear along with a
lot of surrounding logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(cherry picked from commit 08ffd8705d36c7c445df3ecee8ad9b8f8d65fbe0)
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index d38d30affbff..c3c43f59eead 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -69,18 +69,14 @@ static int cf_check stdvga_mem_write(
static bool cf_check stdvga_mem_accept(
const struct hvm_io_handler *handler, const ioreq_t *p)
{
- if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
+ /*
+ * Only accept single direct writes, as that's the only thing we can
+ * accelerate using buffered ioreq handling.
+ */
+ if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 ||
+ (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
(ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
- return 0;
-
- if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
- {
- /*
- * Only accept single direct writes, as that's the only thing we can
- * accelerate using buffered ioreq handling.
- */
return false;
- }
return true;
}
© 2016 - 2025 Red Hat, Inc.