Re: [Xen-devel] [PATCH] x86/msi: fix loop termination condition in pci_msi_conf_write_intercept()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/07/2019 10:34, Paul Durrant wrote:
> The for loop that deals with MSI masking is coded as follows:
>
> for ( pos = 0; pos < entry->msi.nvec; ++pos, ++entry )
>
> Thus the loop termination condition is dereferencing a struct pointer that
> is being incremented by the loop. However, it is clear from following code
> paths in msi_capability_init() that this is unsafe as for instance, in the
> case of nvec == 1, entry will point at a single struct msi_desc allocation
> and thus the loop will walk beyond the bounds of the allocation before
> dereferencing the memory to determine whether the loop should terminate.

More specifically, only entry[0].msi.nvec is correct.  All subsequent
nvec fields are 0 in a block of entries.

> Also, because the body of the loop writes via the entry pointer, this can
> then lead to heap memory corruption, or indeed corruption of anything in
> the direct map.
>
> This patch simply initializes a stack variable to the value of
> entry->msi.nvec before starting the loop and then uses that in the
> termination condition instead.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> Cc: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>
> Credit to Andrew Cooper and Igor Druzhinin for helping narrow down the
> source of the memory corruption. It has taken many weeks of head-scratching
> to get to this fix.

This has taken an embarrassingly long time figure out, even after
debugging hinted that the assignment to guest_masked (in context) was
the culprit of memory corruption.

Needless to say, this wants backporting to all trees.

Reivewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> ---
>  xen/arch/x86/msi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index babc4147c4..89e61160e9 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1328,6 +1328,7 @@ int pci_msi_conf_write_intercept(struct pci_dev *pdev, 
> unsigned int reg,
>      {
>          uint16_t cntl;
>          uint32_t unused;
> +        unsigned int nvec = entry->msi.nvec;
>  
>          pos = entry->msi_attrib.pos;
>          if ( reg < pos || reg >= entry->msi.mpos + 8 )
> @@ -1340,7 +1341,7 @@ int pci_msi_conf_write_intercept(struct pci_dev *pdev, 
> unsigned int reg,
>  
>          cntl = pci_conf_read16(seg, bus, slot, func, msi_control_reg(pos));
>          unused = ~(uint32_t)0 >> (32 - multi_msi_capable(cntl));
> -        for ( pos = 0; pos < entry->msi.nvec; ++pos, ++entry )
> +        for ( pos = 0; pos < nvec; ++pos, ++entry )
>          {
>              entry->msi_attrib.guest_masked =
>                  *data >> entry->msi_attrib.entry_nr;


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel