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

[Chinmay Rath]

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 <ra...@linux.ibm.com>
---
  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 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.

Regards,
Chinmay

Thanks,
Amit