[PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Navid Emamdoost]

The qpci_iomap() function fails with a fatal g_assert(addr) if it
probes a PCI BAR that has a size of zero. This is expected behavior
for certain devices, like the Q35 PCI Host Bridge, which have valid but
unimplemented BARs.
This assertion blocks the creation of fuzz targets for complex machine
types that include these devices.
Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a
zero-sized BAR is now handled gracefully by returning an empty BAR
struct, allowing fuzzing to proceed. The original assertion is kept for
all other builds to maintain strict checking for qtest and production
environments.

Signed-off-by: Navid Emamdoost <navidem@google.com>
---
 tests/qtest/libqos/pci.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index a59197b992..df9e2a3993 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
         addr &= PCI_BASE_ADDRESS_MEM_MASK;
     }
 
+#ifdef CONFIG_FUZZ
+    /*
+     * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal
+     * error. This occurs when probing devices like the Q35 host bridge. We
+     * return gracefully to allow fuzzing to continue. In non-fuzzing builds,
+     * we retain the original g_assert() to catch unexpected behavior.
+     */
+    if (!addr) {
+        if (sizeptr) {
+            *sizeptr = 0;
+        }
+        memset(&bar, 0, sizeof(bar));
+        return bar;
+    }
+#endif
+
     g_assert(addr); /* Must have *some* size bits */
 
     size = 1U << ctz32(addr);
-- 
2.51.0.710.ga91ca5db03-goog

[PATCH 2/2] tests/qtest/fuzz: Add generic fuzzer for pcie-pci-bridge

[Navid Emamdoost]

Add a new generic fuzz target for the 'pcie-pci-bridge' device. This
target uses a Q35 machine with a multi-level PCI hierarchy to exercise
the bridge's functionality.
This is made possible by the preceding change to handle unimplemented
BARs during fuzzing.

---

This new target significantly improves code coverage for the pcie-pci-bridge
implementation. The baseline coverage shown below was generated by
running all existing fuzz targets with the oss-fuzz corpus.

=== Component: hw/pci  ===
-------------------------------------------------------------------------------
File                           New Target           Baseline             Change
-------------------------------------------------------------------------------
shpc.c                         359/511 (70.3%)      0/511 (0.0%)         +359
pci_bridge.c                   255/304 (83.9%)      12/304 (3.9%)        +243
pcie.c                         390/774 (50.4%)      160/774 (20.7%)      +230
pcie_aer.c                     119/524 (22.7%)      38/524 (7.3%)        +81
pci.c                          1154/2069 (55.8%)    1084/2069 (52.4%)    +70
pcie_port.c                    58/119 (48.7%)       17/119 (14.3%)       +41
pci.h                          86/132 (65.2%)       81/132 (61.4%)       +5

=== Component: hw/pci-bridge  ===
-------------------------------------------------------------------------------
File                           New Target           Baseline             Change
-------------------------------------------------------------------------------
pcie_root_port.c               86/127 (67.7%)       13/127 (10.2%)       +73
pcie_pci_bridge.c              62/94 (66.0%)        20/94 (21.3%)        +42
gen_pcie_root_port.c           45/66 (68.2%)        19/66 (28.8%)        +26

Signed-off-by: Navid Emamdoost <navidem@google.com>
---
 tests/qtest/fuzz/generic_fuzz_configs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index ef0ad95712..e025f57a3e 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -247,6 +247,14 @@ const generic_fuzz_config predefined_configs[] = {
         .args = "-machine q35 -nodefaults "
         "-parallel file:/dev/null",
         .objects = "parallel*",
+    },{
+        .name = "pcie-pci-bridge",
+        .args = "-machine q35 -nodefaults "
+        "-device pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=true,addr=0x2 "
+        "-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 "
+        "-netdev user,id=net0 "
+        "-device e1000,netdev=net0,id=nic0,bus=pci.2,addr=0x3",
+        .objects = "pci* shpc*"
     }
 };
 
-- 
2.51.0.710.ga91ca5db03-goog

Re: [PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Alexander Bulekov]

On 251008 1919, Navid Emamdoost wrote:
> The qpci_iomap() function fails with a fatal g_assert(addr) if it
> probes a PCI BAR that has a size of zero. This is expected behavior
> for certain devices, like the Q35 PCI Host Bridge, which have valid but
> unimplemented BARs.
> This assertion blocks the creation of fuzz targets for complex machine
> types that include these devices.
> Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a
> zero-sized BAR is now handled gracefully by returning an empty BAR
> struct, allowing fuzzing to proceed. The original assertion is kept for
> all other builds to maintain strict checking for qtest and production
> environments.

Is there a way to determine whether a BAR is unimplememnted from the
PCIDev in generic_fuzz.c:pci_enum so that we can skip the call to iomap?

>
> Signed-off-by: Navid Emamdoost <navidem@google.com>
> ---
>  tests/qtest/libqos/pci.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index a59197b992..df9e2a3993 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>          addr &= PCI_BASE_ADDRESS_MEM_MASK;
>      }
>
> +#ifdef CONFIG_FUZZ
> +    /*
> +     * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal
> +     * error. This occurs when probing devices like the Q35 host bridge. We
> +     * return gracefully to allow fuzzing to continue. In non-fuzzing builds,
> +     * we retain the original g_assert() to catch unexpected behavior.
> +     */
> +    if (!addr) {
> +        if (sizeptr) {
> +            *sizeptr = 0;
> +        }
> +        memset(&bar, 0, sizeof(bar));
> +        return bar;
> +    }
> +#endif
> +
>      g_assert(addr); /* Must have *some* size bits */
>
>      size = 1U << ctz32(addr);
> --
> 2.51.0.710.ga91ca5db03-goog
>
>

Re: [PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Navid Emamdoost]

Hi Alexander,

On Fri, Oct 10, 2025 at 8:59 AM Alexander Bulekov <alxndr@bu.edu> wrote:
>
> On 251008 1919, Navid Emamdoost wrote:
> > The qpci_iomap() function fails with a fatal g_assert(addr) if it
> > probes a PCI BAR that has a size of zero. This is expected behavior
> > for certain devices, like the Q35 PCI Host Bridge, which have valid but
> > unimplemented BARs.
> > This assertion blocks the creation of fuzz targets for complex machine
> > types that include these devices.
> > Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a
> > zero-sized BAR is now handled gracefully by returning an empty BAR
> > struct, allowing fuzzing to proceed. The original assertion is kept for
> > all other builds to maintain strict checking for qtest and production
> > environments.
>
> Is there a way to determine whether a BAR is unimplememnted from the
> PCIDev in generic_fuzz.c:pci_enum so that we can skip the call to iomap?
>

Fair point. I don't think we have a reliable way to determine if a BAR
is truly unimplemented from the PCIDevice model without probing it. If
we moved that hardware probe into pci_enum, it would become
inefficient for all the BARs that are implemented, as they would be
probed twice: once in pci_enum just to check, and then again inside
qpci_iomap to do the actual mapping. That's why I think delegating
this check to qpci_iomap is the cleaner approach.

> >
> > Signed-off-by: Navid Emamdoost <navidem@google.com>
> > ---
> >  tests/qtest/libqos/pci.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> > index a59197b992..df9e2a3993 100644
> > --- a/tests/qtest/libqos/pci.c
> > +++ b/tests/qtest/libqos/pci.c
> > @@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
> >          addr &= PCI_BASE_ADDRESS_MEM_MASK;
> >      }
> >
> > +#ifdef CONFIG_FUZZ
> > +    /*
> > +     * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal
> > +     * error. This occurs when probing devices like the Q35 host bridge. We
> > +     * return gracefully to allow fuzzing to continue. In non-fuzzing builds,
> > +     * we retain the original g_assert() to catch unexpected behavior.
> > +     */
> > +    if (!addr) {
> > +        if (sizeptr) {
> > +            *sizeptr = 0;
> > +        }
> > +        memset(&bar, 0, sizeof(bar));
> > +        return bar;
> > +    }
> > +#endif
> > +
> >      g_assert(addr); /* Must have *some* size bits */
> >
> >      size = 1U << ctz32(addr);
> > --
> > 2.51.0.710.ga91ca5db03-goog
> >
> >



-- 
Thank you,
Navid.

Re: [PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Navid Emamdoost]

On Mon, Oct 13, 2025 at 6:14 PM Navid Emamdoost <navidem@google.com> wrote:
>
> Hi Alexander,
>
> On Fri, Oct 10, 2025 at 8:59 AM Alexander Bulekov <alxndr@bu.edu> wrote:
> >
> > On 251008 1919, Navid Emamdoost wrote:
> > > The qpci_iomap() function fails with a fatal g_assert(addr) if it
> > > probes a PCI BAR that has a size of zero. This is expected behavior
> > > for certain devices, like the Q35 PCI Host Bridge, which have valid but
> > > unimplemented BARs.
> > > This assertion blocks the creation of fuzz targets for complex machine
> > > types that include these devices.
> > > Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a
> > > zero-sized BAR is now handled gracefully by returning an empty BAR
> > > struct, allowing fuzzing to proceed. The original assertion is kept for
> > > all other builds to maintain strict checking for qtest and production
> > > environments.
> >
> > Is there a way to determine whether a BAR is unimplememnted from the
> > PCIDev in generic_fuzz.c:pci_enum so that we can skip the call to iomap?
> >
>
> Fair point. I don't think we have a reliable way to determine if a BAR
> is truly unimplemented from the PCIDevice model without probing it. If
> we moved that hardware probe into pci_enum, it would become
> inefficient for all the BARs that are implemented, as they would be
> probed twice: once in pci_enum just to check, and then again inside
> qpci_iomap to do the actual mapping. That's why I think delegating
> this check to qpci_iomap is the cleaner approach.
>

Friendly ping.

> > >
> > > Signed-off-by: Navid Emamdoost <navidem@google.com>
> > > ---
> > >  tests/qtest/libqos/pci.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> > > index a59197b992..df9e2a3993 100644
> > > --- a/tests/qtest/libqos/pci.c
> > > +++ b/tests/qtest/libqos/pci.c
> > > @@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
> > >          addr &= PCI_BASE_ADDRESS_MEM_MASK;
> > >      }
> > >
> > > +#ifdef CONFIG_FUZZ
> > > +    /*
> > > +     * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal
> > > +     * error. This occurs when probing devices like the Q35 host bridge. We
> > > +     * return gracefully to allow fuzzing to continue. In non-fuzzing builds,
> > > +     * we retain the original g_assert() to catch unexpected behavior.
> > > +     */
> > > +    if (!addr) {
> > > +        if (sizeptr) {
> > > +            *sizeptr = 0;
> > > +        }
> > > +        memset(&bar, 0, sizeof(bar));
> > > +        return bar;
> > > +    }
> > > +#endif
> > > +
> > >      g_assert(addr); /* Must have *some* size bits */
> > >
> > >      size = 1U << ctz32(addr);
> > > --
> > > 2.51.0.710.ga91ca5db03-goog
> > >
> > >
>
>
>
> --
> Thank you,
> Navid.



-- 
Thank you,
Navid.

Re: [PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Peter Maydell]

On Wed, 8 Oct 2025 at 20:21, Navid Emamdoost <navidem@google.com> wrote:
>
> The qpci_iomap() function fails with a fatal g_assert(addr) if it
> probes a PCI BAR that has a size of zero. This is expected behavior
> for certain devices, like the Q35 PCI Host Bridge, which have valid but
> unimplemented BARs.
> This assertion blocks the creation of fuzz targets for complex machine
> types that include these devices.
> Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a
> zero-sized BAR is now handled gracefully by returning an empty BAR
> struct, allowing fuzzing to proceed. The original assertion is kept for
> all other builds to maintain strict checking for qtest and production
> environments.
>
> Signed-off-by: Navid Emamdoost <navidem@google.com>
> ---
>  tests/qtest/libqos/pci.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index a59197b992..df9e2a3993 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>          addr &= PCI_BASE_ADDRESS_MEM_MASK;
>      }
>
> +#ifdef CONFIG_FUZZ
> +    /*
> +     * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal
> +     * error. This occurs when probing devices like the Q35 host bridge. We
> +     * return gracefully to allow fuzzing to continue. In non-fuzzing builds,
> +     * we retain the original g_assert() to catch unexpected behavior.
> +     */
> +    if (!addr) {
> +        if (sizeptr) {
> +            *sizeptr = 0;
> +        }
> +        memset(&bar, 0, sizeof(bar));
> +        return bar;
> +    }
> +#endif

Personally I think I'd prefer it if we didn't make this
dependent on CONFIG_FUZZ. If BARs with no size are
a valid thing then the libqos API should handle them
(e.g. in theory one should be able to write a test that the
Q35 host bridge provides the addr=0 BARs it is supposed to).

I think if we added the size to the QPCIBar struct then we
could assert in the accessors like qpci_io_readb() and
friends that the offset provided was in range. That would
catch both the unlikely "we implemented the BAR with no
size" case and the rather more likely "we got the size too
small" case (and also the "bug in the test and it got the
offset too big" case), and would mean that we don't lose
anything by not asserting that we have a non-zero-size BAR here.

What do you think?

thanks
-- PMM

Re: [PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Navid Emamdoost]

On Thu, Nov 13, 2025 at 6:03 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 8 Oct 2025 at 20:21, Navid Emamdoost <navidem@google.com> wrote:
> >
> > The qpci_iomap() function fails with a fatal g_assert(addr) if it
> > probes a PCI BAR that has a size of zero. This is expected behavior
> > for certain devices, like the Q35 PCI Host Bridge, which have valid but
> > unimplemented BARs.
> > This assertion blocks the creation of fuzz targets for complex machine
> > types that include these devices.
> > Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a
> > zero-sized BAR is now handled gracefully by returning an empty BAR
> > struct, allowing fuzzing to proceed. The original assertion is kept for
> > all other builds to maintain strict checking for qtest and production
> > environments.
> >
> > Signed-off-by: Navid Emamdoost <navidem@google.com>
> > ---
> >  tests/qtest/libqos/pci.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> > index a59197b992..df9e2a3993 100644
> > --- a/tests/qtest/libqos/pci.c
> > +++ b/tests/qtest/libqos/pci.c
> > @@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
> >          addr &= PCI_BASE_ADDRESS_MEM_MASK;
> >      }
> >
> > +#ifdef CONFIG_FUZZ
> > +    /*
> > +     * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal
> > +     * error. This occurs when probing devices like the Q35 host bridge. We
> > +     * return gracefully to allow fuzzing to continue. In non-fuzzing builds,
> > +     * we retain the original g_assert() to catch unexpected behavior.
> > +     */
> > +    if (!addr) {
> > +        if (sizeptr) {
> > +            *sizeptr = 0;
> > +        }
> > +        memset(&bar, 0, sizeof(bar));
> > +        return bar;
> > +    }
> > +#endif
>
> Personally I think I'd prefer it if we didn't make this
> dependent on CONFIG_FUZZ. If BARs with no size are
> a valid thing then the libqos API should handle them
> (e.g. in theory one should be able to write a test that the
> Q35 host bridge provides the addr=0 BARs it is supposed to).
>
> I think if we added the size to the QPCIBar struct then we
> could assert in the accessors like qpci_io_readb() and
> friends that the offset provided was in range. That would
> catch both the unlikely "we implemented the BAR with no
> size" case and the rather more likely "we got the size too
> small" case (and also the "bug in the test and it got the
> offset too big" case), and would mean that we don't lose
> anything by not asserting that we have a non-zero-size BAR here.
>
> What do you think?
>
> thanks
> -- PMM

Hi Peter,

That's a much more elegant approach. Thank you for the suggestion!
I've done an initial investigation into the impact of adding a size
field to QPCIBar and checking it in the accessors. As you anticipated,
making the API safer immediately and correctly flushed out a few
latent issues in the existing qtest suite.
Before I prepare and send the full v2 patch series, I wanted to run my
plan for fixing the test failures by you:

- Issue with qpci_legacy_iomap: Several tests (like ide-test and
tco-test) fail because they use qpci_legacy_iomap, which has no way to
provide a BAR size. My plan is to change its signature to
qpci_legacy_iomap(dev, addr, size) and then update the handful of
failing call sites to provide the correct, explicit I/O region size.
This seems like the cleanest way to make them compatible and safer.

- Issue with nvme-test: The qos-test fails during the
nvmetest_oob_cmb_test because the test logic was performing an illegal
out-of-bounds access on the entire PCI BAR. My plan is to rewrite this
specific test to correctly read the CMB registers and perform a valid
out-of-bounds check within the BAR's limits, which is what the test
was originally intended to do.

Does this plan for handling the test failures seem correct to you? If
yes, I will go ahead and prepare the v2 series with the above fixes.

-- 
Thank you,
Navid.

Re: [PATCH 1/2] libqos: pci: Avoid fatal assert on zero-sized BARs in fuzz builds

[Peter Maydell]

On Tue, 25 Nov 2025 at 02:31, Navid Emamdoost <navidem@google.com> wrote:
>
> On Thu, Nov 13, 2025 at 6:03 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > I think if we added the size to the QPCIBar struct then we
> > could assert in the accessors like qpci_io_readb() and
> > friends that the offset provided was in range. That would
> > catch both the unlikely "we implemented the BAR with no
> > size" case and the rather more likely "we got the size too
> > small" case (and also the "bug in the test and it got the
> > offset too big" case), and would mean that we don't lose
> > anything by not asserting that we have a non-zero-size BAR here.

> That's a much more elegant approach. Thank you for the suggestion!
> I've done an initial investigation into the impact of adding a size
> field to QPCIBar and checking it in the accessors. As you anticipated,
> making the API safer immediately and correctly flushed out a few
> latent issues in the existing qtest suite.
> Before I prepare and send the full v2 patch series, I wanted to run my
> plan for fixing the test failures by you:
>
> - Issue with qpci_legacy_iomap: Several tests (like ide-test and
> tco-test) fail because they use qpci_legacy_iomap, which has no way to
> provide a BAR size. My plan is to change its signature to
> qpci_legacy_iomap(dev, addr, size) and then update the handful of
> failing call sites to provide the correct, explicit I/O region size.
> This seems like the cleanest way to make them compatible and safer.
>
> - Issue with nvme-test: The qos-test fails during the
> nvmetest_oob_cmb_test because the test logic was performing an illegal
> out-of-bounds access on the entire PCI BAR. My plan is to rewrite this
> specific test to correctly read the CMB registers and perform a valid
> out-of-bounds check within the BAR's limits, which is what the test
> was originally intended to do.
>
> Does this plan for handling the test failures seem correct to you? If
> yes, I will go ahead and prepare the v2 series with the above fixes.

Yes, that all sounds OK.

thanks
-- PMM

[PATCH v2 0/5] tests/qtest: Rework libqos PCI BAR handling to support fuzzing

[Navid Emamdoost]

Hi Peter,

Thank you for the excellent feedback on the first version of this series.
I have implemented your suggestions in this new version of the patch series.

The original prerequisite patch (which used #ifdef CONFIG_FUZZ) has been
expanded into a 4-patch series that makes the libqos PCI API safer and more
robust for all use cases. This involved fixing several tests in the suite
that were performing invalid memory accesses, which the new API now
correctly catches.

The final patch in the series, which adds the pcie-pci-bridge fuzzer,
remains unchanged.

---

Changes in v2:

- Reworked the prerequisite from a single #ifdef-based patch into a
4-patch series that improves the core libqos API.
- (1/5) The core change now adds a size field to QPCIBar and moves
bounds checking into the accessor functions.
- (2/5) The qpci_legacy_iomap function is updated to require a size,
and its callers (ide-test, tco-test) are fixed.
- (3/5) ahci-test is fixed to check only implemented ports.
- (4/5) nvme-test is reworked to be compatible with the new strict
BAR checks.
- (5/5) The fuzzer patch is unchanged.


Navid Emamdoost (5):
  libqos: pci: Handle zero-sized BARs gracefully
  libqos: pci: Require size for legacy I/O port mapping
  tests/qtest: ahci-test: Check only implemented ports in verify_state
  tests/qtest: Rework nvmetest_oob_cmb_test for BAR check
  tests/qtest/fuzz: Add generic fuzzer for pcie-pci-bridge

 tests/qtest/ahci-test.c                 |  8 +++++++
 tests/qtest/fuzz/generic_fuzz_configs.h |  8 +++++++
 tests/qtest/ide-test.c                  |  2 +-
 tests/qtest/libqos/pci.c                | 29 +++++++++++++++++++---
 tests/qtest/libqos/pci.h                |  3 ++-
 tests/qtest/nvme-test.c                 | 32 ++++++++++++++++++-------
 tests/qtest/tco-test.c                  |  2 +-
 7 files changed, 69 insertions(+), 15 deletions(-)

-- 
2.52.0.158.g65b55ccf14-goog

[PATCH v2 1/5] libqos: pci: Handle zero-sized BARs gracefully

[Navid Emamdoost]

The qpci_iomap() function would previously fail with a fatal assertion
if it probed a PCI BAR that had a size of zero. This is, however,
expected behavior for some devices like the Q35 host bridge, and the
assertion blocked the creation of new fuzzing targets.

Instead of asserting at map time, modify the QPCIBar struct to store
the BAR's size. Defer the safety check to the accessor functions
(qpci_io_readb, qpci_memread, etc.), which now assert that any
access is within the BAR's bounds.

Signed-off-by: Navid Emamdoost navidem@google.com
---
 tests/qtest/libqos/pci.c | 25 ++++++++++++++++++++++++-
 tests/qtest/libqos/pci.h |  1 +
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index a59197b992..70caf382cc 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -396,6 +396,7 @@ void qpci_config_writel(QPCIDevice *dev, uint8_t offset, uint32_t value)
 
 uint8_t qpci_io_readb(QPCIDevice *dev, QPCIBar token, uint64_t off)
 {
+	g_assert(off + 1 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -410,6 +411,7 @@ uint8_t qpci_io_readb(QPCIDevice *dev, QPCIBar token, uint64_t off)
 
 uint16_t qpci_io_readw(QPCIDevice *dev, QPCIBar token, uint64_t off)
 {
+	g_assert(off + 2 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -424,6 +426,7 @@ uint16_t qpci_io_readw(QPCIDevice *dev, QPCIBar token, uint64_t off)
 
 uint32_t qpci_io_readl(QPCIDevice *dev, QPCIBar token, uint64_t off)
 {
+	g_assert(off + 4 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -438,6 +441,7 @@ uint32_t qpci_io_readl(QPCIDevice *dev, QPCIBar token, uint64_t off)
 
 uint64_t qpci_io_readq(QPCIDevice *dev, QPCIBar token, uint64_t off)
 {
+	g_assert(off + 8 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -453,6 +457,7 @@ uint64_t qpci_io_readq(QPCIDevice *dev, QPCIBar token, uint64_t off)
 void qpci_io_writeb(QPCIDevice *dev, QPCIBar token, uint64_t off,
                     uint8_t value)
 {
+	g_assert(off + 1 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -465,6 +470,7 @@ void qpci_io_writeb(QPCIDevice *dev, QPCIBar token, uint64_t off,
 void qpci_io_writew(QPCIDevice *dev, QPCIBar token, uint64_t off,
                     uint16_t value)
 {
+	g_assert(off + 2 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -478,6 +484,7 @@ void qpci_io_writew(QPCIDevice *dev, QPCIBar token, uint64_t off,
 void qpci_io_writel(QPCIDevice *dev, QPCIBar token, uint64_t off,
                     uint32_t value)
 {
+	g_assert(off + 4 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -491,6 +498,7 @@ void qpci_io_writel(QPCIDevice *dev, QPCIBar token, uint64_t off,
 void qpci_io_writeq(QPCIDevice *dev, QPCIBar token, uint64_t off,
                     uint64_t value)
 {
+	g_assert(off + 8 <= token.size);
     QPCIBus *bus = dev->bus;
 
     if (token.is_io) {
@@ -500,10 +508,10 @@ void qpci_io_writeq(QPCIDevice *dev, QPCIBar token, uint64_t off,
         bus->memwrite(bus, token.addr + off, &value, sizeof(value));
     }
 }
-
 void qpci_memread(QPCIDevice *dev, QPCIBar token, uint64_t off,
                   void *buf, size_t len)
 {
+	g_assert(off + len <= token.size);
     g_assert(!token.is_io);
     dev->bus->memread(dev->bus, token.addr + off, buf, len);
 }
@@ -511,6 +519,7 @@ void qpci_memread(QPCIDevice *dev, QPCIBar token, uint64_t off,
 void qpci_memwrite(QPCIDevice *dev, QPCIBar token, uint64_t off,
                    const void *buf, size_t len)
 {
+	g_assert(off + len <= token.size);
     g_assert(!token.is_io);
     dev->bus->memwrite(dev->bus, token.addr + off, buf, len);
 }
@@ -541,6 +550,19 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
         addr &= PCI_BASE_ADDRESS_MEM_MASK;
     }
 
+    if (!addr){
+        /*
+         * This is an unimplemented BAR. It is not a fatal error.
+         * We model it as a BAR with a size of zero. Any attempt to
+         * access it will be caught by assertions in the accessors.
+         */
+        if (sizeptr) {
+            *sizeptr = 0;
+        }
+        memset(&bar, 0, sizeof(bar));
+        return bar;
+    }
+
     g_assert(addr); /* Must have *some* size bits */
 
     size = 1U << ctz32(addr);
@@ -572,6 +594,7 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     }
 
     bar.addr = loc;
+    bar.size = size;
     return bar;
 }
 
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 8389614523..e790e5293d 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -58,6 +58,7 @@ struct QPCIBus {
 
 struct QPCIBar {
     uint64_t addr;
+    uint64_t size;
     bool is_io;
 };
 
-- 
2.52.0.158.g65b55ccf14-goog

Re: [PATCH v2 1/5] libqos: pci: Handle zero-sized BARs gracefully

[Peter Maydell]

On Thu, 27 Nov 2025 at 00:12, Navid Emamdoost <navidem@google.com> wrote:
>
> The qpci_iomap() function would previously fail with a fatal assertion
> if it probed a PCI BAR that had a size of zero. This is, however,
> expected behavior for some devices like the Q35 host bridge, and the
> assertion blocked the creation of new fuzzing targets.
>
> Instead of asserting at map time, modify the QPCIBar struct to store
> the BAR's size. Defer the safety check to the accessor functions
> (qpci_io_readb, qpci_memread, etc.), which now assert that any
> access is within the BAR's bounds.
>
> Signed-off-by: Navid Emamdoost navidem@google.com
> ---
>  tests/qtest/libqos/pci.c | 25 ++++++++++++++++++++++++-
>  tests/qtest/libqos/pci.h |  1 +
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index a59197b992..70caf382cc 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -396,6 +396,7 @@ void qpci_config_writel(QPCIDevice *dev, uint8_t offset, uint32_t value)
>
>  uint8_t qpci_io_readb(QPCIDevice *dev, QPCIBar token, uint64_t off)
>  {
> +       g_assert(off + 1 <= token.size);
>      QPCIBus *bus = dev->bus;

The indent seems to be wrong for all your changes to these functions?

Also, we need "make check" to pass for every commit in the
patchset, not just after it has all been applied. So we need
to make the fixes that you have in patches 2-4 before we
can start enforcing the size limits with assertions.

> @@ -541,6 +550,19 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>          addr &= PCI_BASE_ADDRESS_MEM_MASK;
>      }
>
> +    if (!addr){

Missing space before "{". (scripts/checkpatch.pl will
probably catch this kind of style error.)

> +        /*
> +         * This is an unimplemented BAR. It is not a fatal error.
> +         * We model it as a BAR with a size of zero. Any attempt to
> +         * access it will be caught by assertions in the accessors.
> +         */
> +        if (sizeptr) {
> +            *sizeptr = 0;
> +        }
> +        memset(&bar, 0, sizeof(bar));
> +        return bar;
> +    }
> +
>      g_assert(addr); /* Must have *some* size bits */

We can drop this assert now, because we just dealt with
the addr == 0 case.

>      size = 1U << ctz32(addr);
> @@ -572,6 +594,7 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>      }
>
>      bar.addr = loc;
> +    bar.size = size;
>      return bar;
>  }

thanks
-- PMM

Re: [PATCH v2 1/5] libqos: pci: Handle zero-sized BARs gracefully

[Navid Emamdoost]

Hi Peter,

On Thu, Nov 27, 2025 at 5:17 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Nov 2025 at 00:12, Navid Emamdoost <navidem@google.com> wrote:
> >
> > The qpci_iomap() function would previously fail with a fatal assertion
> > if it probed a PCI BAR that had a size of zero. This is, however,
> > expected behavior for some devices like the Q35 host bridge, and the
> > assertion blocked the creation of new fuzzing targets.
> >
> > Instead of asserting at map time, modify the QPCIBar struct to store
> > the BAR's size. Defer the safety check to the accessor functions
> > (qpci_io_readb, qpci_memread, etc.), which now assert that any
> > access is within the BAR's bounds.
> >
> > Signed-off-by: Navid Emamdoost navidem@google.com
> > ---
> >  tests/qtest/libqos/pci.c | 25 ++++++++++++++++++++++++-
> >  tests/qtest/libqos/pci.h |  1 +
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> > index a59197b992..70caf382cc 100644
> > --- a/tests/qtest/libqos/pci.c
> > +++ b/tests/qtest/libqos/pci.c
> > @@ -396,6 +396,7 @@ void qpci_config_writel(QPCIDevice *dev, uint8_t offset, uint32_t value)
> >
> >  uint8_t qpci_io_readb(QPCIDevice *dev, QPCIBar token, uint64_t off)
> >  {
> > +       g_assert(off + 1 <= token.size);
> >      QPCIBus *bus = dev->bus;
>
> The indent seems to be wrong for all your changes to these functions?
>
> Also, we need "make check" to pass for every commit in the
> patchset, not just after it has all been applied. So we need
> to make the fixes that you have in patches 2-4 before we
> can start enforcing the size limits with assertions.

Do you think it's better to squash all changes of patch1-4 into
a single commit that reworks the libqos PCI API and fixes all affected tests.

>
> > @@ -541,6 +550,19 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
> >          addr &= PCI_BASE_ADDRESS_MEM_MASK;
> >      }
> >
> > +    if (!addr){
>
> Missing space before "{". (scripts/checkpatch.pl will
> probably catch this kind of style error.)
>
> > +        /*
> > +         * This is an unimplemented BAR. It is not a fatal error.
> > +         * We model it as a BAR with a size of zero. Any attempt to
> > +         * access it will be caught by assertions in the accessors.
> > +         */
> > +        if (sizeptr) {
> > +            *sizeptr = 0;
> > +        }
> > +        memset(&bar, 0, sizeof(bar));
> > +        return bar;
> > +    }
> > +
> >      g_assert(addr); /* Must have *some* size bits */
>
> We can drop this assert now, because we just dealt with
> the addr == 0 case.
>
> >      size = 1U << ctz32(addr);
> > @@ -572,6 +594,7 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
> >      }
> >
> >      bar.addr = loc;
> > +    bar.size = size;
> >      return bar;
> >  }
>
> thanks
> -- PMM



-- 
Thank you,
Navid.

[PATCH v2 2/5] libqos: pci: Require size for legacy I/O port mapping

[Navid Emamdoost]

The accessor functions for QPCIBar (qpci_io_readb, etc.) perform
strict bounds checking to ensure memory safety. However, the
qpci_legacy_iomap function created QPCIBar tokens for legacy I/O
ports without an associated size, making this safety check impossible.

To fix this, modify the signature of qpci_legacy_iomap to require the
caller to explicitly provide the size of the legacy I/O region.

Update all existing callers of this function, including the IDE
(ide-test.c) and TCO watchdog (tco-test.c) test suites, to provide
the correct, known sizes for the hardware they are testing. This not
only fixes the test failures but also makes the tests more robust and
explicit about the I/O regions they interact with.

Signed-off-by: Navid Emamdoost <navidem@google.com>
---
 tests/qtest/ide-test.c   | 2 +-
 tests/qtest/libqos/pci.c | 4 ++--
 tests/qtest/libqos/pci.h | 2 +-
 tests/qtest/tco-test.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index ceee444a9e..524458e9f6 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -173,7 +173,7 @@ static QPCIDevice *get_pci_device(QTestState *qts, QPCIBar *bmdma_bar,
     /* Map bmdma BAR */
     *bmdma_bar = qpci_iomap(dev, 4, NULL);
 
-    *ide_bar = qpci_legacy_iomap(dev, IDE_BASE);
+    *ide_bar = qpci_legacy_iomap(dev, IDE_BASE, 8);
 
     qpci_device_enable(dev);
 
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index 70caf382cc..f07fc9140e 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -603,9 +603,9 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar bar)
     /* FIXME */
 }
 
-QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
+QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr, uint64_t size)
 {
-    QPCIBar bar = { .addr = addr, .is_io = true };
+    QPCIBar bar = { .addr = addr, .size = size, .is_io = true };
     return bar;
 }
 
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index e790e5293d..6e8e0fbff6 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -123,7 +123,7 @@ void qpci_memwrite(QPCIDevice *bus, QPCIBar token, uint64_t off,
                    const void *buf, size_t len);
 QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr);
 void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
-QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
+QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr, uint64_t size);
 
 void qpci_unplug_acpi_device_test(QTestState *qs, const char *id, uint8_t slot);
 
diff --git a/tests/qtest/tco-test.c b/tests/qtest/tco-test.c
index 20ccefabcb..3af7c14e73 100644
--- a/tests/qtest/tco-test.c
+++ b/tests/qtest/tco-test.c
@@ -77,7 +77,7 @@ static void test_init(TestData *d)
     /* set Root Complex BAR */
     qpci_config_writel(d->dev, ICH9_LPC_RCBA, RCBA_BASE_ADDR | 0x1);
 
-    d->tco_io_bar = qpci_legacy_iomap(d->dev, PM_IO_BASE_ADDR + 0x60);
+    d->tco_io_bar = qpci_legacy_iomap(d->dev, PM_IO_BASE_ADDR + 0x60, 32);
     d->qts = qs;
 }
 
-- 
2.52.0.158.g65b55ccf14-goog

Re: [PATCH v2 2/5] libqos: pci: Require size for legacy I/O port mapping

[Peter Maydell]

On Thu, 27 Nov 2025 at 00:12, Navid Emamdoost <navidem@google.com> wrote:
>
> The accessor functions for QPCIBar (qpci_io_readb, etc.) perform
> strict bounds checking to ensure memory safety. However, the
> qpci_legacy_iomap function created QPCIBar tokens for legacy I/O
> ports without an associated size, making this safety check impossible.
>
> To fix this, modify the signature of qpci_legacy_iomap to require the
> caller to explicitly provide the size of the legacy I/O region.
>
> Update all existing callers of this function, including the IDE
> (ide-test.c) and TCO watchdog (tco-test.c) test suites, to provide
> the correct, known sizes for the hardware they are testing. This not
> only fixes the test failures but also makes the tests more robust and
> explicit about the I/O regions they interact with.
>
> Signed-off-by: Navid Emamdoost <navidem@google.com>
> ---
>  tests/qtest/ide-test.c   | 2 +-
>  tests/qtest/libqos/pci.c | 4 ++--
>  tests/qtest/libqos/pci.h | 2 +-
>  tests/qtest/tco-test.c   | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index ceee444a9e..524458e9f6 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -173,7 +173,7 @@ static QPCIDevice *get_pci_device(QTestState *qts, QPCIBar *bmdma_bar,
>      /* Map bmdma BAR */
>      *bmdma_bar = qpci_iomap(dev, 4, NULL);
>
> -    *ide_bar = qpci_legacy_iomap(dev, IDE_BASE);
> +    *ide_bar = qpci_legacy_iomap(dev, IDE_BASE, 8);

I think we could add a
#define IDE_SIZE 8
after the existing #define of IDE_BASE, and then use that rather
than a plain constant 8 here.

>
>      qpci_device_enable(dev);
> diff --git a/tests/qtest/tco-test.c b/tests/qtest/tco-test.c
> index 20ccefabcb..3af7c14e73 100644
> --- a/tests/qtest/tco-test.c
> +++ b/tests/qtest/tco-test.c
> @@ -77,7 +77,7 @@ static void test_init(TestData *d)
>      /* set Root Complex BAR */
>      qpci_config_writel(d->dev, ICH9_LPC_RCBA, RCBA_BASE_ADDR | 0x1);
>
> -    d->tco_io_bar = qpci_legacy_iomap(d->dev, PM_IO_BASE_ADDR + 0x60);
> +    d->tco_io_bar = qpci_legacy_iomap(d->dev, PM_IO_BASE_ADDR + 0x60, 32);

we already include ich9.h which defines a named constant
for the IO size, so we can use ICH9_PMIO_TCO_LEN here.

thanks
-- PMM

[PATCH v2 3/5] tests/qtest: ahci-test: Check only implemented ports in verify_state

[Navid Emamdoost]

The verify_state helper function in ahci-test.c incorrectly
assumed that all 32 potential AHCI ports were implemented. During post-
migration checks, it would loop through all 32 ports, attempting to
read registers for non-existent ones.
This resulted in an out-of-bounds access on the main HBA BAR. This
latent bug was exposed by the recent introduction of strict bounds
checking in the libqos PCI accessors, which now correctly triggers a
fatal assertion.
Fix this by modifying the loop in verify_state to first read the
AHCI_PI (Ports Implemented) register and then only check the state
for ports that the device reports as present.

Signed-off-by: Navid Emamdoost <navidem@google.com>
---
 tests/qtest/ahci-test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index e8aabfc13f..06c5bd08d8 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -81,6 +81,7 @@ static void string_bswap16(uint16_t *s, size_t bytes)
 static void verify_state(AHCIQState *ahci, uint64_t hba_old)
 {
     int i, j;
+    uint32_t ports_impl;
     uint32_t ahci_fingerprint;
     uint64_t hba_base;
     AHCICommandHeader cmd;
@@ -99,7 +100,14 @@ static void verify_state(AHCIQState *ahci, uint64_t hba_old)
     g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP), ==, ahci->cap);
     g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP2), ==, ahci->cap2);
 
+    ports_impl = ahci_rreg(ahci, AHCI_PI);
+
     for (i = 0; i < 32; i++) {
+
+        if (!(ports_impl & (1 << i))) {
+            continue; /* Skip unimplemented ports */
+        }
+
         g_assert_cmphex(ahci_px_rreg(ahci, i, AHCI_PX_FB), ==,
                         ahci->port[i].fb);
         g_assert_cmphex(ahci_px_rreg(ahci, i, AHCI_PX_CLB), ==,
-- 
2.52.0.158.g65b55ccf14-goog

Re: [PATCH v2 3/5] tests/qtest: ahci-test: Check only implemented ports in verify_state

[Peter Maydell]

On Thu, 27 Nov 2025 at 00:12, Navid Emamdoost <navidem@google.com> wrote:
>
> The verify_state helper function in ahci-test.c incorrectly
> assumed that all 32 potential AHCI ports were implemented. During post-
> migration checks, it would loop through all 32 ports, attempting to
> read registers for non-existent ones.
> This resulted in an out-of-bounds access on the main HBA BAR. This
> latent bug was exposed by the recent introduction of strict bounds
> checking in the libqos PCI accessors, which now correctly triggers a
> fatal assertion.
> Fix this by modifying the loop in verify_state to first read the
> AHCI_PI (Ports Implemented) register and then only check the state
> for ports that the device reports as present.
>
> Signed-off-by: Navid Emamdoost <navidem@google.com>
> ---
>  tests/qtest/ahci-test.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index e8aabfc13f..06c5bd08d8 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -81,6 +81,7 @@ static void string_bswap16(uint16_t *s, size_t bytes)
>  static void verify_state(AHCIQState *ahci, uint64_t hba_old)
>  {
>      int i, j;
> +    uint32_t ports_impl;
>      uint32_t ahci_fingerprint;
>      uint64_t hba_base;
>      AHCICommandHeader cmd;
> @@ -99,7 +100,14 @@ static void verify_state(AHCIQState *ahci, uint64_t hba_old)
>      g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP), ==, ahci->cap);
>      g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP2), ==, ahci->cap2);
>
> +    ports_impl = ahci_rreg(ahci, AHCI_PI);
> +
>      for (i = 0; i < 32; i++) {
> +
> +        if (!(ports_impl & (1 << i))) {

We should use "1U << i" here, because coverity will probably
complain about shifting into the sign bit of a signed integer
otherwise.

> +            continue; /* Skip unimplemented ports */
> +        }
> +
>          g_assert_cmphex(ahci_px_rreg(ahci, i, AHCI_PX_FB), ==,
>                          ahci->port[i].fb);
>          g_assert_cmphex(ahci_px_rreg(ahci, i, AHCI_PX_CLB), ==,

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 4/5] tests/qtest: Rework nvmetest_oob_cmb_test for BAR check

[Navid Emamdoost]

The nvmetest_oob_cmb_test was designed to deliberately perform an
out-of-bounds write on a PCI BAR. This was intended as a regression
test for CVE-2018-16847.
The recent change to libqos introduced strict bounds checking on all
BAR accessors, which correctly caused this test to fail with a fatal
assertion, as it was performing an illegal memory access.
This change reworks the test to honor its original intent—verifying
safe accesses at the BAR boundary—without violating the new API contract.
Instead of attempting an illegal write, the test now performs several
valid read/write operations at the very end of the BAR (at offsets
size - 1, size - 2, and size - 4) to confirm the entire region
is accessible.
This makes the test compatible with the safer libqos API while still
serving as a regression test for the original issue.

Signed-off-by: Navid Emamdoost <navidem@google.com>
---
 tests/qtest/nvme-test.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/tests/qtest/nvme-test.c b/tests/qtest/nvme-test.c
index 5ad6821f7a..8be37ae043 100644
--- a/tests/qtest/nvme-test.c
+++ b/tests/qtest/nvme-test.c
@@ -48,23 +48,37 @@ static void *nvme_create(void *pci_bus, QGuestAllocator *alloc, void *addr)
 /* This used to cause a NULL pointer dereference.  */
 static void nvmetest_oob_cmb_test(void *obj, void *data, QGuestAllocator *alloc)
 {
-    const int cmb_bar_size = 2 * MiB;
     QNvme *nvme = obj;
     QPCIDevice *pdev = &nvme->dev;
     QPCIBar bar;
+    const uint64_t expected_cmb_size = 2 * MiB;
 
+    /* Enable the device's I/O and memory resources at the PCI level. */
     qpci_device_enable(pdev);
+
+    /* Map BAR 2, which is the dedicated BAR for the Controller Memory Buffer. */
     bar = qpci_iomap(pdev, 2, NULL);
 
-    qpci_io_writel(pdev, bar, 0, 0xccbbaa99);
-    g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99);
-    g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99);
+    /* Sanity check that the probed BAR size matches the command line. */
+    g_assert_cmpint(bar.size, ==, expected_cmb_size);
+
+    /*
+     * Perform read/write checks at the very end of the BAR to ensure
+     * that the entire region is accessible and that boundary accesses of
+     * different sizes are handled correctly.
+     */
+
+    /* Test the last valid byte (the fix for the CVE was about 1-byte access) */
+    qpci_io_writeb(pdev, bar, bar.size - 1, 0x11);
+    g_assert_cmpint(qpci_io_readb(pdev, bar, bar.size - 1), ==, 0x11);
+
+    /* Test the last valid word */
+    qpci_io_writew(pdev, bar, bar.size - 2, 0x2233);
+    g_assert_cmpint(qpci_io_readw(pdev, bar, bar.size - 2), ==, 0x2233);
 
-    /* Test partially out-of-bounds accesses.  */
-    qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211);
-    g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11);
-    g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, 0x2211);
-    g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211);
+    /* Test the last valid dword */
+    qpci_io_writel(pdev, bar, bar.size - 4, 0x44556677);
+    g_assert_cmpint(qpci_io_readl(pdev, bar, bar.size - 4), ==, 0x44556677);
 }
 
 static void nvmetest_reg_read_test(void *obj, void *data, QGuestAllocator *alloc)
-- 
2.52.0.158.g65b55ccf14-goog

Re: [PATCH v2 4/5] tests/qtest: Rework nvmetest_oob_cmb_test for BAR check

[Peter Maydell]

On Thu, 27 Nov 2025 at 00:13, Navid Emamdoost <navidem@google.com> wrote:
>
> The nvmetest_oob_cmb_test was designed to deliberately perform an
> out-of-bounds write on a PCI BAR. This was intended as a regression
> test for CVE-2018-16847.
> The recent change to libqos introduced strict bounds checking on all
> BAR accessors, which correctly caused this test to fail with a fatal
> assertion, as it was performing an illegal memory access.
> This change reworks the test to honor its original intent—verifying
> safe accesses at the BAR boundary—without violating the new API contract.
> Instead of attempting an illegal write, the test now performs several
> valid read/write operations at the very end of the BAR (at offsets
> size - 1, size - 2, and size - 4) to confirm the entire region
> is accessible.
> This makes the test compatible with the safer libqos API while still
> serving as a regression test for the original issue.

This one I'll have to leave for the nvme folks to review.
(You'll want to recast the commit message because this
change has to go before we add the assertions, not after.)

thanks
-- PMM

[PATCH v2 5/5] tests/qtest/fuzz: Add generic fuzzer for pcie-pci-bridge

[Navid Emamdoost]

Add a new generic fuzz target for the 'pcie-pci-bridge' device. This
target uses a Q35 machine with a multi-level PCI hierarchy to exercise
the bridge's functionality.
This is made possible by the preceding change to handle unimplemented
BARs during fuzzing.

---

This new target significantly improves code coverage for the pcie-pci-bridge
implementation. The baseline coverage shown below was generated by
running all existing fuzz targets with the oss-fuzz corpus.

=== Component: hw/pci  ===
-------------------------------------------------------------------------------
File                           New Target           Baseline             Change
-------------------------------------------------------------------------------
shpc.c                         359/511 (70.3%)      0/511 (0.0%)         +359
pci_bridge.c                   255/304 (83.9%)      12/304 (3.9%)        +243
pcie.c                         390/774 (50.4%)      160/774 (20.7%)      +230
pcie_aer.c                     119/524 (22.7%)      38/524 (7.3%)        +81
pci.c                          1154/2069 (55.8%)    1084/2069 (52.4%)    +70
pcie_port.c                    58/119 (48.7%)       17/119 (14.3%)       +41
pci.h                          86/132 (65.2%)       81/132 (61.4%)       +5

=== Component: hw/pci-bridge  ===
-------------------------------------------------------------------------------
File                           New Target           Baseline             Change
-------------------------------------------------------------------------------
pcie_root_port.c               86/127 (67.7%)       13/127 (10.2%)       +73
pcie_pci_bridge.c              62/94 (66.0%)        20/94 (21.3%)        +42
gen_pcie_root_port.c           45/66 (68.2%)        19/66 (28.8%)        +26

Signed-off-by: Navid Emamdoost <navidem@google.com>
---
 tests/qtest/fuzz/generic_fuzz_configs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index ef0ad95712..e025f57a3e 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -247,6 +247,14 @@ const generic_fuzz_config predefined_configs[] = {
         .args = "-machine q35 -nodefaults "
         "-parallel file:/dev/null",
         .objects = "parallel*",
+    },{
+        .name = "pcie-pci-bridge",
+        .args = "-machine q35 -nodefaults "
+        "-device pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=true,addr=0x2 "
+        "-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 "
+        "-netdev user,id=net0 "
+        "-device e1000,netdev=net0,id=nic0,bus=pci.2,addr=0x3",
+        .objects = "pci* shpc*"
     }
 };
 
-- 
2.52.0.158.g65b55ccf14-goog