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