On 8/17/26 17:42, Amit Machhiwal wrote:
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.
Oooh right, I missed this. Thanks ! Will send a v2 right away.
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
