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.durr...@citrix.com>
> ---
> Cc: Jan Beulich <jbeul...@suse.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>
> Cc: Wei Liu <w...@xen.org>
> Cc: "Roger Pau Monné" <roger....@citrix.com>
> Cc: Igor Druzhinin <igor.druzhi...@citrix.com>
>
> 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.coop...@citrix.com>
> ---
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
