Re: [PATCH] target/ppc: Add lower bound check for watchdogNumber

On 2026/08/17 05:27 PM, Chinmay Rath wrote:
> 
> On 8/17/26 17:03, Amit Machhiwal wrote:
> > On 2026/08/17 04:28 PM, Chinmay Rath wrote:
> > > Add missing lower bound check for H_WATCHDOG H_CALL's watchdogNumber parameter
> > > as per PAPR documentation ver 12.10.00 section 14.15.5 'H_WATCHDOG'.
> > > 
> > > Closes : https://gitlab.com/qemu-project/qemu/-/work_items/3600
> > > 
> > Nit: This newline is not needed.
> > 
> > > Signed-off-by: Chinmay Rath <[email protected]>
> > > ---
> > >   hw/watchdog/spapr_watchdog.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> > > index 5b3f50de3a..5d7478e833 100644
> > > --- a/hw/watchdog/spapr_watchdog.c
> > > +++ b/hw/watchdog/spapr_watchdog.c
> > > @@ -145,7 +145,7 @@ static target_ulong h_watchdog(PowerPCCPU *cpu,
> > >       switch (operation) {
> > >       case PSERIES_WDTF_OP_START:
> > > -        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> > > +        if (watchdogNumber < 1 || watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> > >               return H_P2;
> > >           }
> > >           if (timeoutInMs <= WDT_MIN_TIMEOUT) {
> > > @@ -170,7 +170,8 @@ static target_ulong h_watchdog(PowerPCCPU *cpu,
> > >       case PSERIES_WDTF_OP_STOP:
> > >           if (watchdogNumber == PSERIES_WDT_STOP_ALL) {
> > >               ret = watchdog_stop_all(spapr);
> > > -        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
> > > +        } else if (watchdogNumber > 0 &&
> > > +                   watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
> > >               ret = watchdog_stop(watchdogNumber,
> > >                                   &spapr->wds[watchdogNumber - 1]);
> > >           } else {
> > Nit: The bounds check in OP_STOP uses a positive-selection guard
> > (watchdogNumber > 0 && watchdogNumber <= ARRAY_SIZE(...)) with the error
> > in the trailing else, which reads differently from the negative guards
> > used in OP_START and OP_QUERY_LPM. Consider flipping it to match:
> > 
> >    } else if (watchdogNumber < 1 ||
> >               watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> >        return H_P2;
> >    } else {
> >        ret = watchdog_stop(watchdogNumber,
> >                            &spapr->wds[watchdogNumber - 1]);
> >    }
> > 
> > > @@ -184,7 +185,7 @@ static target_ulong h_watchdog(PowerPCCPU *cpu,
> > >           trace_spapr_watchdog_query(args[0]);
> > >           break;
> > >       case PSERIES_WDTF_OP_QUERY_LPM:
> > > -        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> > > +        if (watchdogNumber < 1 || watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> > >               return H_P2;
> > >           }
> > Suggestion (follow-up patch): After this fix lands, it may be worth
> > extracting a small helper to avoid the repeated bounds expression across
> > OP_START, OP_STOP, and OP_QUERY_LPM:
> > 
> >    static inline bool watchdog_number_valid(target_ulong n,
> >                                             SpaprMachineState *spapr)
> >    {
> >        return n >= 1 && n <= ARRAY_SIZE(spapr->wds);
> >    }
> > 
> > The three call sites then become a uniform !watchdog_number_valid(...)
> > or watchdog_number_valid(...) expression, the bounds are defined in
> > exactly one place, and any future change to the valid range (e.g. a
> > dynamic wds size) has a single point of update. Not a blocker for this
> > patch — just a clean-up worth a separate patch.
> Hey Amit,
> Thanks for the suggestion. I actually did think of this. But, this common
> check would only be used by 2 operations : Start and LPM requirement. Stop
> operation's watchdogNumber validation works differently in the sense that
> unlike others, it can take a specific negative value, that is -1, which is

Thanks for the explanation, Chinmay. Just a small correction though —
PSERIES_WDT_STOP_ALL is not -1. It is defined as:

  #define PSERIES_WDT_STOP_ALL  ((uint64_t)~0)

Since watchdogNumber is target_ulong (an unsigned type), this is
0xFFFFFFFFFFFFFFFF — the maximum value of a 64-bit unsigned integer, not
a negative value. -1 would only be the interpretation if the bit pattern
were read through a signed type, which never happens here.

> used to indicate stopping all watchdogs. (This is also the reason why the
> bound check flow is different for stop operation as you have already pointed
> out above).
> 
> Due to this, I decided not to consolidate. However we can definitely pass
> the operation type to the validator function and check accordingly though.

This actually reinforces the case for the shared helper. Since
watchdogNumber is unsigned, watchdogNumber < 1 can never be true for
PSERIES_WDT_STOP_ALL — 0xFFFFFFFFFFFFFFFF < 1 is false for an unsigned
comparison. More importantly, STOP_ALL is caught by the explicit ==
PSERIES_WDT_STOP_ALL arm before the bounds check is ever reached, so it
would never interact with watchdog_number_valid() at all.

~Amit