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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Sent: 02 July 2019 11:29
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Jan 
> Beulich <jbeulich@xxxxxxxx>;
> Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] x86/msi: fix loop termination condition in
> pci_msi_conf_write_intercept()
> 
> On 02/07/2019 10:47, Andrew Cooper wrote:
> > 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.
> 
> There is actually a second bug here which is being fixed.  How about
> this for the commit message?
> 

Apart from exchange/outlook terminally mangling it (as you can probably see 
below... unless it miraculously unmangles this reply), it looks ok to me. I 
assume you are happy to fix on commit?

  Paul

> x86/msi: fix loop termination condition in
> pci_msi_conf_write_intercept()
> 
> 
> 
> 
> 
> 
> 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.
> 
> 
> 
> 
> 
> 
> A block of MSI entries stores the number of vectors in
> entry[0].msi.nvec,
> 
> 
> with all subsequent entries using a value of 0.  Therefore, for a block
> of
> 
> 
> two or more MSIs will terminate the loop early, as entry[1].msi.nvec is
> 0.
> 
> 
> 
> 
> 
> 
> However, for a single MSI, ++entry moves the pointer out of bounds, and
> a
> 
> 
> bogus read is used for the termination condition.  In the case that
> the
> 
> 
> loop body gets entered, there are subsequent OoB writes which
> clobber
> 
> 
> adjacent memory in the
> heap.
> 
> 
> 
> 
> 
> 
> 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.
> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel