[v1] hw/pci/pcie_doe: Check mailbox length for overflows

[PATCH] hw/pci/pcie_doe: Check mailbox length for overflows

Posted by alistair23@gmail.com 2 weeks, 6 days ago

From: Alistair Francis <alistair.francis@wdc.com>

It was possible that a guest could overflow the `doe_cap->write_mbox`
buffer by writing more then PCI_DOE_DW_SIZE_MAX dwords.
`doe_cap->write_mbox_len` would continue to increment and there were no
bounds checks on the length when offsetting into doe_cap->write_mbox.

This patch adds a check and reports a guest error if we would overflow.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3679
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/pci/pcie_doe.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
index 2210f86968..765ebea42c 100644
--- a/hw/pci/pcie_doe.c
+++ b/hw/pci/pcie_doe.c
@@ -356,8 +356,14 @@ void pcie_doe_write_config(DOECap *doe_cap,
         if (size != DWORD_BYTE) {
             return;
         }
-        doe_cap->write_mbox[doe_cap->write_mbox_len] = val;
-        doe_cap->write_mbox_len++;
+        if (doe_cap->write_mbox_len < PCI_DOE_DW_SIZE_MAX) {
+            doe_cap->write_mbox[doe_cap->write_mbox_len] = val;
+            doe_cap->write_mbox_len++;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "Mailbox write length (%d) is overflowing\n",
+                          doe_cap->write_mbox_len);
+        }
         break;
     case PCI_EXP_DOE_CAP:
         /* fallthrough */
-- 
2.54.0

Re: [PATCH] hw/pci/pcie_doe: Check mailbox length for overflows

Posted by Philippe Mathieu-Daudé 2 weeks, 6 days ago

Hi Alistair,

On 6/7/26 02:36, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> It was possible that a guest could overflow the `doe_cap->write_mbox`
> buffer by writing more then PCI_DOE_DW_SIZE_MAX dwords.
> `doe_cap->write_mbox_len` would continue to increment and there were no
> bounds checks on the length when offsetting into doe_cap->write_mbox.
> 
> This patch adds a check and reports a guest error if we would overflow.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3679
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   hw/pci/pcie_doe.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
> index 2210f86968..765ebea42c 100644
> --- a/hw/pci/pcie_doe.c
> +++ b/hw/pci/pcie_doe.c
> @@ -356,8 +356,14 @@ void pcie_doe_write_config(DOECap *doe_cap,
>           if (size != DWORD_BYTE) {
>               return;
>           }
> -        doe_cap->write_mbox[doe_cap->write_mbox_len] = val;
> -        doe_cap->write_mbox_len++;
> +        if (doe_cap->write_mbox_len < PCI_DOE_DW_SIZE_MAX) {
> +            doe_cap->write_mbox[doe_cap->write_mbox_len] = val;
> +            doe_cap->write_mbox_len++;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "Mailbox write length (%d) is overflowing\n",
> +                          doe_cap->write_mbox_len);


Do you know how real hardware behaves? I'd expect it to wrap, not
discard.

> +        }
>           break;
>       case PCI_EXP_DOE_CAP:
>           /* fallthrough */

Re: [PATCH] hw/pci/pcie_doe: Check mailbox length for overflows

Posted by Alistair Francis 2 weeks, 5 days ago

On Mon, Jul 6, 2026 at 6:22 PM Philippe Mathieu-Daudé
<philmd@oss.qualcomm.com> wrote:
>
> Hi Alistair,
>
> On 6/7/26 02:36, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > It was possible that a guest could overflow the `doe_cap->write_mbox`
> > buffer by writing more then PCI_DOE_DW_SIZE_MAX dwords.
> > `doe_cap->write_mbox_len` would continue to increment and there were no
> > bounds checks on the length when offsetting into doe_cap->write_mbox.
> >
> > This patch adds a check and reports a guest error if we would overflow.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3679
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   hw/pci/pcie_doe.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
> > index 2210f86968..765ebea42c 100644
> > --- a/hw/pci/pcie_doe.c
> > +++ b/hw/pci/pcie_doe.c
> > @@ -356,8 +356,14 @@ void pcie_doe_write_config(DOECap *doe_cap,
> >           if (size != DWORD_BYTE) {
> >               return;
> >           }
> > -        doe_cap->write_mbox[doe_cap->write_mbox_len] = val;
> > -        doe_cap->write_mbox_len++;
> > +        if (doe_cap->write_mbox_len < PCI_DOE_DW_SIZE_MAX) {
> > +            doe_cap->write_mbox[doe_cap->write_mbox_len] = val;
> > +            doe_cap->write_mbox_len++;
> > +        } else {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "Mailbox write length (%d) is overflowing\n",
> > +                          doe_cap->write_mbox_len);
>
>
> Do you know how real hardware behaves? I'd expect it to wrap, not
> discard.

Good question.

Real hardware requires a length for a message to be valid. So a valid
message can't be longer then PCI_DOE_DW_SIZE_MAX.

The PCIe spec states: "If the number of DW transferred does not match
the Length indicated in DOE Data Object Header 2 for a data object,
then the entire data object must be silently discarded." So we
actually should drop the entire message, not just the overflow. I'll
fixup in v2.

Alistair

>
> > +        }
> >           break;
> >       case PCI_EXP_DOE_CAP:
> >           /* fallthrough */
>