A guest-triggerable assertion crash (DoS) exists in pnv_phb4_config_write()
and pnv_phb4_config_read(). When a guest performs an 8-byte access to
PHB_CONFIG_DATA (offset 0x130), QEMU aborts because the switch(size)
statement in the config accessors only handles 1, 2, and 4-byte accesses,
hitting g_assert_not_reached() in the default case.

Since guest input is untrusted, an invalid access size should not crash
the host. Fix this by adding a size > 4 guard in pnv_phb4_reg_write() and
pnv_phb4_reg_read() before the config accessor calls, and by replacing
the g_assert_not_reached() in pnv_phb4_config_write() and
pnv_phb4_config_read() with phb_error() to cover any other callers.
Invalid reads now return ~0ull, maintaining PCI conventions.

Fixes: 4f9924c4d4cf ("ppc/pnv: Add models for POWER9 PHB4 PCIe Host bridge")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3591
Signed-off-by: Nikhil Kumar Singh <nikhilks@linux.ibm.com>
---
 hw/pci-host/pnv_phb4.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 705a5bcf07..02ca2f978c 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -84,7 +84,9 @@ static void pnv_phb4_config_write(PnvPHB4 *phb, unsigned off,
         val = bswap32(val);
         break;
     default:
-        g_assert_not_reached();
+        phb_error(phb, "invalid config write size %u at offset 0x%x\n",
+                  size, off);
+        return;
     }
     pci_host_config_write_common(pdev, cfg_addr, limit, val, size);
 }
@@ -119,7 +121,9 @@ static uint64_t pnv_phb4_config_read(PnvPHB4 *phb, unsigned off,
     case 4:
         return bswap32(val);
     default:
-        g_assert_not_reached();
+        phb_error(phb, "invalid config read size %u at offset 0x%x\n",
+                  size, off);
+        return ~0ull;
     }
 }
 
@@ -507,6 +511,11 @@ static void pnv_phb4_reg_write(void *opaque, hwaddr off, uint64_t val,
 
     /* Special case outbound configuration data */
     if ((off & 0xfffc) == PHB_CONFIG_DATA) {
+        if (size > 4) {
+            phb_error(phb, "invalid config write size %u at 0x%"PRIx64"\n",
+                      size, off);
+            return;
+        }
         pnv_phb4_config_write(phb, off & 0x3, size, val);
         return;
     }
@@ -644,6 +653,11 @@ static uint64_t pnv_phb4_reg_read(void *opaque, hwaddr off, unsigned size)
     uint64_t val;
 
     if ((off & 0xfffc) == PHB_CONFIG_DATA) {
+        if (size > 4) {
+            phb_error(phb, "invalid config read size %u at 0x%"PRIx64"\n",
+                      size, off);
+            return ~0ull;
+        }
         return pnv_phb4_config_read(phb, off & 0x3, size);
     }
 
-- 
2.54.0

Hi,

On 5/8/26 11:05, Nikhil Kumar Singh wrote:
> A guest-triggerable assertion crash (DoS) exists in pnv_phb4_config_write()
> and pnv_phb4_config_read(). When a guest performs an 8-byte access to
> PHB_CONFIG_DATA (offset 0x130), QEMU aborts because the switch(size)
> statement in the config accessors only handles 1, 2, and 4-byte accesses,
> hitting g_assert_not_reached() in the default case.
> 
> Since guest input is untrusted, an invalid access size should not crash
> the host. Fix this by adding a size > 4 guard in pnv_phb4_reg_write() and
> pnv_phb4_reg_read() before the config accessor calls, and by replacing
> the g_assert_not_reached() in pnv_phb4_config_write() and
> pnv_phb4_config_read() with phb_error() to cover any other callers.
> Invalid reads now return ~0ull, maintaining PCI conventions.
> 
> Fixes: 4f9924c4d4cf ("ppc/pnv: Add models for POWER9 PHB4 PCIe Host bridge")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3591
> Signed-off-by: Nikhil Kumar Singh <nikhilks@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb4.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)


> @@ -507,6 +511,11 @@ static void pnv_phb4_reg_write(void *opaque, hwaddr off, uint64_t val,
>   
>       /* Special case outbound configuration data */
>       if ((off & 0xfffc) == PHB_CONFIG_DATA) {
> +        if (size > 4) {
> +            phb_error(phb, "invalid config write size %u at 0x%"PRIx64"\n",
> +                      size, off);
> +            return;
> +        }
>           pnv_phb4_config_write(phb, off & 0x3, size, val);

Alternatively MIN(size, 4).

But without looking much at this model, I'd expect these regions to
be mapped as the usual "pci-conf-idx" / "pci-data-idx" ones (FYI see
pci_host_conf_le_ops and pci_host_data_le_ops).

Anyway my 2 cents, since this involves more of "silence that DoS".

>           return;
>       }
> @@ -644,6 +653,11 @@ static uint64_t pnv_phb4_reg_read(void *opaque, hwaddr off, unsigned size)
>       uint64_t val;
>   
>       if ((off & 0xfffc) == PHB_CONFIG_DATA) {
> +        if (size > 4) {
> +            phb_error(phb, "invalid config read size %u at 0x%"PRIx64"\n",
> +                      size, off);
> +            return ~0ull;
> +        }
>           return pnv_phb4_config_read(phb, off & 0x3, size);
>       }
>

Hi Philippe,

Thanks for the review.

1. Regarding MIN(size, 4): I avoided this because silently truncating an 
8-byte guest read to 4 bytes leaves the upper 32 bits unpredictable, 
which might mask guest OS bugs. Returning ~0ull explicitly matches 
standard PCI behaviour for invalid reads and is safer.

2. Regarding pci_host_data_le_ops: I agree this is the right way to 
handle it. However, PHB_CONFIG_DATA is currently interleaved inside the 
larger big-endian PHB4 MMIO region. Moving it to generic little-endian 
ops requires a memory region refactor using overlays.

Since this patch addresses an immediate DoS crash, I kept the scope 
minimal. Memory region refactoring can be picked up as a separate 
follow-up patch.

Regards,
~ Nikhil

On 5/8/26 15:53, Nikhil Kumar Singh wrote:
> Hi Philippe,
> 
> Thanks for the review.
> 
> 1. Regarding MIN(size, 4): I avoided this because silently truncating an 
> 8-byte guest read to 4 bytes leaves the upper 32 bits unpredictable, 
> which might mask guest OS bugs. Returning ~0ull explicitly matches 
> standard PCI behaviour for invalid reads and is safer.

OK.

> 2. Regarding pci_host_data_le_ops: I agree this is the right way to 
> handle it. However, PHB_CONFIG_DATA is currently interleaved inside the 
> larger big-endian PHB4 MMIO region. Moving it to generic little-endian 
> ops requires a memory region refactor using overlays.

Yeah, "if it ain't broke, don’t fix it", so let's keep maintaining
something old and different.

> Since this patch addresses an immediate DoS crash, I kept the scope 
> minimal.

OK, no objection to your patch, just my 2 cents ;)

> Memory region refactoring can be picked up as a separate 
> follow-up patch.
> 
> Regards,
> ~ Nikhil
> 

+ Jishnu

Thanks Philippe for your inputs.

Hi Aditya,
Are we expecting a v2 or a follow-up patch here?
Needs your ack, thanks.

regards,
Harsh

On 05/08/26 9:23 pm, Philippe Mathieu-Daudé wrote:
> On 5/8/26 15:53, Nikhil Kumar Singh wrote:
>> Hi Philippe,
>>
>> Thanks for the review.
>>
>> 1. Regarding MIN(size, 4): I avoided this because silently truncating 
>> an 8-byte guest read to 4 bytes leaves the upper 32 bits 
>> unpredictable, which might mask guest OS bugs. Returning ~0ull 
>> explicitly matches standard PCI behaviour for invalid reads and is safer.
> 
> OK.
> 
>> 2. Regarding pci_host_data_le_ops: I agree this is the right way to 
>> handle it. However, PHB_CONFIG_DATA is currently interleaved inside 
>> the larger big-endian PHB4 MMIO region. Moving it to generic little- 
>> endian ops requires a memory region refactor using overlays.
> 
> Yeah, "if it ain't broke, don’t fix it", so let's keep maintaining
> something old and different.
> 
>> Since this patch addresses an immediate DoS crash, I kept the scope 
>> minimal.
> 
> OK, no objection to your patch, just my 2 cents ;)
> 
>> Memory region refactoring can be picked up as a separate follow-up patch.
>>
>> Regards,
>> ~ Nikhil
>>
>