[PATCH] hpet: fix bounds check for s->timer[]

[PATCH] hpet: fix bounds check for s->timer[]

[Paolo Bonzini]

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

Re: [PATCH] hpet: fix bounds check for s->timer[]

[Peter Maydell]

On Fri, 27 Mar 2026 at 17:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

If we can only fit 24 timers into the MMIO region, should we do one of:
 * lower HPET_MAX_TIMERS
 * enlarge the MMIO region
 * leave HPET_MAX_TIMERS where it is but make realize enforce
   that num_timers <= 24 ?

thanks
-- PMM

Re: [PATCH] hpet: fix bounds check for s->timer[]

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

Il ven 27 mar 2026, 19:46 Peter Maydell <peter.maydell@linaro.org> ha
scritto:

> > (even
> > though HPET_MAX_TIMERS is 32) the HPET only has room for 24 timers in
> > its MMIO region,
>

If we can only fit 24 timers into the MMIO region, should we do one of:
>  * lower HPET_MAX_TIMERS
>  * enlarge the MMIO region
>  * leave HPET_MAX_TIMERS where it is but make realize enforce
>    that num_timers <= 24 ?
>

Lowering HPET_MAX_TIMERS is the easiest, yes. No one really uses anything
but the default anyway.

Paolo


> thanks
> -- PMM
>
>

[-- Attachment #2: Type: text/html, Size: 1477 bytes --]

Re: [PATCH] hpet: fix bounds check for s->timer[]

[Philippe Mathieu-Daudé]

On 27/3/26 18:47, Paolo Bonzini wrote:
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] hpet: fix bounds check for s->timer[]

[Zhao Liu]

On Fri, Mar 27, 2026 at 09:02:09PM +0100, Paolo Bonzini wrote:
> Date: Fri, 27 Mar 2026 21:02:09 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH] hpet: fix bounds check for s->timer[]
> 
> Il ven 27 mar 2026, 19:46 Peter Maydell <peter.maydell@linaro.org> ha
> scritto:
> 
> > > (even
> > > though HPET_MAX_TIMERS is 32) the HPET only has room for 24 timers in
> > > its MMIO region,

It seems like a missing case for HPET spec v1.0a (about how to extend
MMIO). The MMIO size (HPET_LEN = 0x400) and max timers (HPET_MAX_TIMERS
= 32) are both from the spec. And general capabilities register
allocates bits 8-12 for NUM_TIM_CAP (up to 32 timers).

The spec only mentions for IA64 platform, the timer register space can
be up to 64K bytes with page protection capability. :(

> If we can only fit 24 timers into the MMIO region, should we do one of:
> >  * lower HPET_MAX_TIMERS
> >  * enlarge the MMIO region
> >  * leave HPET_MAX_TIMERS where it is but make realize enforce
> >    that num_timers <= 24 ?
> >
> 
> Lowering HPET_MAX_TIMERS is the easiest, yes. No one really uses anything
> but the default anyway.

yes, I agree, maybe no vender implememnts more than 24 timers, which is
why HPET doesn't provide further details on MMIO extensions I think.

Regards,
Zhao

Re: [PATCH] hpet: fix bounds check for s->timer[]

[Zhao Liu]

On Fri, Mar 27, 2026 at 06:47:01PM +0100, Paolo Bonzini wrote:
> Date: Fri, 27 Mar 2026 18:47:01 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] hpet: fix bounds check for s->timer[]
> X-Mailer: git-send-email 2.53.0
> 
> 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(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>