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

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.

Thanks,
Amit