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

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