[PULL 05/12] hpet: fix bounds check for s->timer[]

Paolo Bonzini posted 12 patches 6 days, 21 hours ago
Maintainers: Pierrick Bouvier <pierrick.bouvier@linaro.org>, Pedro Barbuda <pbarbuda@microsoft.com>, Mohamed Mediouni <mohamed@unpredictable.fr>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Wei Liu <wei.liu@kernel.org>
[PULL 05/12] hpet: fix bounds check for s->timer[]
Posted by Paolo Bonzini 6 days, 21 hours ago
Fix an off-by-one issue in QEMU's HPET read and write MMIO handlers.
Both handlers check timer_id > s->num_timers instead of timer_id >=
s->num_timers, allowing a guest to access one timer beyond the valid
range.

The affected slot is initialized properly in hpet_realize, which goes
through all HPET_MAX_TIMERS elements of the array, so even though
it is not reset in hpet_reset() the bug does not cause any use of
uninitialized host memory.  Because of this, and also because (even
though HPET_MAX_TIMERS is 32) the HPET only has room for 24 timers in
its MMIO region, the bug has no security implications.

Commit 869b0afa4fa ("rust/hpet: Drop BqlCell wrapper for num_timers",
2025-06-06) silently fixed the same bug in rust/hw/timer/hpet/src/device.rs.

Reported-by: Yuma Kurogome, Ricerca Security, Inc. <yumak@ricsec.co.jp>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/timer/hpet.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 767093c431a..42285cff762 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -464,13 +464,14 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
         }
     } else {
         uint8_t timer_id = (addr - 0x100) / 0x20;
-        HPETTimer *timer = &s->timer[timer_id];
+        HPETTimer *timer;
 
-        if (timer_id > s->num_timers) {
+        if (timer_id >= s->num_timers) {
             trace_hpet_timer_id_out_of_range(timer_id);
             return 0;
         }
 
+        timer = &s->timer[timer_id];
         switch (addr & 0x1f) {
         case HPET_TN_CFG: // including interrupt capabilities
             return timer->config >> shift;
@@ -564,13 +565,15 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
         }
     } else {
         uint8_t timer_id = (addr - 0x100) / 0x20;
-        HPETTimer *timer = &s->timer[timer_id];
+        HPETTimer *timer;
 
         trace_hpet_ram_write_timer_id(timer_id);
-        if (timer_id > s->num_timers) {
+        if (timer_id >= s->num_timers) {
             trace_hpet_timer_id_out_of_range(timer_id);
             return;
         }
+
+        timer = &s->timer[timer_id];
         switch (addr & 0x18) {
         case HPET_TN_CFG:
             trace_hpet_ram_write_tn_cfg(addr & 4);
-- 
2.53.0