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
