[PATCH] As reported in qemu-project/qemu#3324

[PATCH] As reported in qemu-project/qemu#3324

[Zexiang Zhang]

From: kiki <Chan9Yan9@gmail.com>

A malformed IVE value can result in an invalid server field being
passed to icp_irq(). The function assumes the server id is valid and
may access invalid state otherwise, potentially leading to a crash.

Fix this by validating the server id before using it and ignoring
invalid values.

Reported-by: Zexiang Zhang <chan9yan9@gmail.com>
Signed-off-by: Zexiang Zhang <chan9yan9@gmail.com>
---
 hw/intc/xics.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 1d40c4386d..25c7b0c8a5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -222,6 +222,13 @@ void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
 
     trace_xics_icp_irq(server, nr, priority);
 
+    if (!icp) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
+                      server, nr);
+        ics_reject(ics, nr);
+        return;
+    }
+
     if ((priority >= CPPR(icp))
         || (XISR(icp) && (icp->pending_priority <= priority))) {
         ics_reject(ics, nr);
-- 
2.34.1

Re: [PATCH] As reported in qemu-project/qemu#3324

[Aditya Gupta]

Hello Zexiang,

On 26/03/26 11:27PM, Zexiang Zhang wrote:
> From: kiki <Chan9Yan9@gmail.com>
> 
> A malformed IVE value can result in an invalid server field being
> passed to icp_irq(). The function assumes the server id is valid and
> may access invalid state otherwise, potentially leading to a crash.
> 
> Fix this by validating the server id before using it and ignoring
> invalid values.
> 
> Reported-by: Zexiang Zhang <chan9yan9@gmail.com>
> Signed-off-by: Zexiang Zhang <chan9yan9@gmail.com>

About subject, can you change the subject to decribe the fix, something
like 'ppc/pnv: Fix Null Pointer Deref in PHB3', what do you say ?

There's a build error:

	../hw/intc/xics.c: In function ‘icp_irq’:
	../hw/intc/xics.c:226:9: error: implicit declaration of function ‘qemu_log_mask’; did you mean ‘qemu_log’? [-Wimplicit-function-declaration]
	  226 |         qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
	      |         ^~~~~~~~~~~~~
	      |         qemu_log
	../hw/intc/xics.c:226:9: error: nested extern declaration of ‘qemu_log_mask’ [-Werror=nested-externs]
	../hw/intc/xics.c:226:23: error: ‘LOG_GUEST_ERROR’ undeclared (first use in this function); did you mean ‘MOD_ESTERROR’?
	  226 |         qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
	      |                       ^~~~~~~~~~~~~~~
	      |                       MOD_ESTERROR
	../hw/intc/xics.c:226:23: note: each undeclared identifier is reported only once for each function it appears in
	cc1: all warnings being treated as errors

Add '#include "qemu/log.h", maybe after osdep.h include, to fix above
error.

Also, I will recommend running 'make check-functional-ppc64 -j4' to test
the patch before post.

> ---
>  hw/intc/xics.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 1d40c4386d..25c7b0c8a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -222,6 +222,13 @@ void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>  
>      trace_xics_icp_irq(server, nr, priority);
>  
> +    if (!icp) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
> +                      server, nr);
> +        ics_reject(ics, nr);
> +        return;
> +    }
> +
>      if ((priority >= CPPR(icp))
>          || (XISR(icp) && (icp->pending_priority <= priority))) {
>          ics_reject(ics, nr);

The change looks good to me. Can you post a v2 with the subject and
build fixed ?

Thanks,
- Aditya G

Re: [PATCH] As reported in qemu-project/qemu#3324

[Alex Bennée]

Aditya Gupta <adityag@linux.ibm.com> writes:

> Hello Zexiang,
>
> On 26/03/26 11:27PM, Zexiang Zhang wrote:
>> From: kiki <Chan9Yan9@gmail.com>
>> 
>> A malformed IVE value can result in an invalid server field being
>> passed to icp_irq(). The function assumes the server id is valid and
>> may access invalid state otherwise, potentially leading to a crash.
>> 
>> Fix this by validating the server id before using it and ignoring
>> invalid values.
>> 
>> Reported-by: Zexiang Zhang <chan9yan9@gmail.com>
>> Signed-off-by: Zexiang Zhang <chan9yan9@gmail.com>
>
> About subject, can you change the subject to decribe the fix, something
> like 'ppc/pnv: Fix Null Pointer Deref in PHB3', what do you say ?
>
> There's a build error:
>
> 	../hw/intc/xics.c: In function ‘icp_irq’:
> 	../hw/intc/xics.c:226:9: error: implicit declaration of function ‘qemu_log_mask’; did you mean ‘qemu_log’? [-Wimplicit-function-declaration]
> 	  226 |         qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
> 	      |         ^~~~~~~~~~~~~
> 	      |         qemu_log
> 	../hw/intc/xics.c:226:9: error: nested extern declaration of ‘qemu_log_mask’ [-Werror=nested-externs]
> 	../hw/intc/xics.c:226:23: error: ‘LOG_GUEST_ERROR’ undeclared (first use in this function); did you mean ‘MOD_ESTERROR’?
> 	  226 |         qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
> 	      |                       ^~~~~~~~~~~~~~~
> 	      |                       MOD_ESTERROR
> 	../hw/intc/xics.c:226:23: note: each undeclared identifier is reported only once for each function it appears in
> 	cc1: all warnings being treated as errors
>
> Add '#include "qemu/log.h", maybe after osdep.h include, to fix above
> error.
>
> Also, I will recommend running 'make check-functional-ppc64 -j4' to test
> the patch before post.
>
>> ---
>>  hw/intc/xics.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 1d40c4386d..25c7b0c8a5 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -222,6 +222,13 @@ void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>>  
>>      trace_xics_icp_irq(server, nr, priority);
>>  
>> +    if (!icp) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
>> +                      server, nr);
>> +        ics_reject(ics, nr);
>> +        return;
>> +    }
>> +
>>      if ((priority >= CPPR(icp))
>>          || (XISR(icp) && (icp->pending_priority <= priority))) {
>>          ics_reject(ics, nr);
>
> The change looks good to me. Can you post a v2 with the subject and
> build fixed ?

The bug fix link can go in:

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3324

just above your sign off.

>
> Thanks,
> - Aditya G

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] As reported in qemu-project/qemu#3324

[Aditya Gupta]

On 30/03/26 02:04, Alex Bennée wrote:
> Aditya Gupta <adityag@linux.ibm.com> writes:
> ...
>       if ((priority >= CPPR(icp))
>           || (XISR(icp) && (icp->pending_priority <= priority))) {
>           ics_reject(ics, nr);
>> The change looks good to me. Can you post a v2 with the subject and
>> build fixed ?
> The bug fix link can go in:
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3324
>
> just above your sign off.


Hey Zexiang,

Ping, planning to send a v2 for this ?

cc +gautam


Thanks,

- Aditya G

[PATCH] As reported in qemu-project/qemu#3324

[Zexiang Zhang]

From: kiki <Chan9Yan9@gmail.com>

A malformed IVE value can result in an invalid server field being
passed to icp_irq(). The function assumes the server id is valid and
may access invalid state otherwise, potentially leading to a crash.

Fix this by validating the server id before using it and ignoring
invalid values.

Reported-by: Zexiang Zhang <chan9yan9@gmail.com>
Signed-off-by: Zexiang Zhang <chan9yan9@gmail.com>
---
 hw/intc/xics.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 1d40c4386d..25c7b0c8a5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -222,6 +222,13 @@ void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
 
     trace_xics_icp_irq(server, nr, priority);
 
+    if (!icp) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XICS: invalid server %d for IRQ 0x%x\n",
+                      server, nr);
+        ics_reject(ics, nr);
+        return;
+    }
+
     if ((priority >= CPPR(icp))
         || (XISR(icp) && (icp->pending_priority <= priority))) {
         ics_reject(ics, nr);
-- 
2.34.1