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

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 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