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

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

Posted by Chinmay Rath 1 month, 1 week ago

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

Changes from v1 : Incorporated changes suggested by Amit.
v1 : https://lore.kernel.org/qemu-devel/20260817105854.437724-1-rathc@linux.ibm.com/

 hw/watchdog/spapr_watchdog.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
index 5b3f50de3a..460b4457a6 100644
--- a/hw/watchdog/spapr_watchdog.c
+++ b/hw/watchdog/spapr_watchdog.c
@@ -127,6 +127,12 @@ static void watchdog_expired(void *pw)
     }
 }
 
+static inline bool watchdogNumber_valid(target_ulong watchdogNumber,
+                                        SpaprMachineState *spapr)
+{
+    return watchdogNumber >= 1 && watchdogNumber <= ARRAY_SIZE(spapr->wds);
+}
+
 static target_ulong h_watchdog(PowerPCCPU *cpu,
                                SpaprMachineState *spapr,
                                target_ulong opcode, target_ulong *args)
@@ -145,7 +151,7 @@ static target_ulong h_watchdog(PowerPCCPU *cpu,
 
     switch (operation) {
     case PSERIES_WDTF_OP_START:
-        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
+        if (!watchdogNumber_valid(watchdogNumber, spapr)) {
             return H_P2;
         }
         if (timeoutInMs <= WDT_MIN_TIMEOUT) {
@@ -170,11 +176,11 @@ 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_valid(watchdogNumber, spapr)) {
+            return H_P2;
+        } else {
             ret = watchdog_stop(watchdogNumber,
                                 &spapr->wds[watchdogNumber - 1]);
-        } else {
-            return H_P2;
         }
         break;
     case PSERIES_WDTF_OP_QUERY:
@@ -184,7 +190,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_valid(watchdogNumber, spapr)) {
             return H_P2;
         }
         args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
-- 
2.55.0

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

Posted by Amit Machhiwal 1 month, 1 week ago

On 2026/08/18 02:40 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
> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
> ---
> 
> Changes from v1 : Incorporated changes suggested by Amit.

Thanks for the quick v2 — the helper and the OP_STOP restructuring look
good. One remaining nit below.

> v1 : https://lore.kernel.org/qemu-devel/20260817105854.437724-1-rathc@linux.ibm.com/
> 
>  hw/watchdog/spapr_watchdog.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> index 5b3f50de3a..460b4457a6 100644
> --- a/hw/watchdog/spapr_watchdog.c
> +++ b/hw/watchdog/spapr_watchdog.c
> @@ -127,6 +127,12 @@ static void watchdog_expired(void *pw)
>      }
>  }
>  
> +static inline bool watchdogNumber_valid(target_ulong watchdogNumber,

The function name watchdogNumber_valid mixes camelCase with snake_case.
Per the QEMU coding style (docs/devel/style.rst), CamelCase is reserved
for structured type names and typedefs — regular function names follow
lower_case_with_underscores. This should be watchdog_number_valid.

With that fixed:

Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>

Thanks,
Amit

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

Posted by Chinmay Rath 1 month, 1 week ago

On 8/18/26 15:23, Amit Machhiwal wrote:
> On 2026/08/18 02:40 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
>> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
>> ---
>>
>> Changes from v1 : Incorporated changes suggested by Amit.
> Thanks for the quick v2 — the helper and the OP_STOP restructuring look
> good. One remaining nit below.
>
>> v1 : https://lore.kernel.org/qemu-devel/20260817105854.437724-1-rathc@linux.ibm.com/
>>
>>   hw/watchdog/spapr_watchdog.c | 16 +++++++++++-----
>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
>> index 5b3f50de3a..460b4457a6 100644
>> --- a/hw/watchdog/spapr_watchdog.c
>> +++ b/hw/watchdog/spapr_watchdog.c
>> @@ -127,6 +127,12 @@ static void watchdog_expired(void *pw)
>>       }
>>   }
>>   
>> +static inline bool watchdogNumber_valid(target_ulong watchdogNumber,
> The function name watchdogNumber_valid mixes camelCase with snake_case.
> Per the QEMU coding style (docs/devel/style.rst), CamelCase is reserved
> for structured type names and typedefs — regular function names follow
> lower_case_with_underscores. This should be watchdog_number_valid.
Hey thanks for letting me know. I have posted the correct patch 
retaining your R-by.
I had gone ahead with this naming cause the variable itself was named 
'watchdogNumber'.
But going though the qemu coding style, since watchdogNumber is not a 
'structured' type, I guess it should have been named in snake case and 
not camel case in the first place.

Regards,
Chinmay
>
> With that fixed:
>
> Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>
>
> Thanks,
> Amit
>