hw/watchdog/spapr_watchdog.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
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
Signed-off-by: Chinmay Rath <rathc@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 {
@@ -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;
}
args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
--
2.55.0On 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 <rathc@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.
Thanks,
Amit
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 <rathc@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
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 <rathc@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
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
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 <rathc@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
> 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
© 2016 - 2026 Red Hat, Inc.