Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 04:37:50PM +0530, Amit Shah wrote:
> On (Mon) 17 Nov 2014 [12:52:59], Michael S. Tsirkin wrote:
> > On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> > > On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > > > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > > > This patchset fixes CVE-2014-7840: invalid
> > > > > > migration stream can cause arbitrary qemu memory
> > > > > > overwrite.
> > > > > > First patch includes the minimal fix for the issue.
> > > > > > Follow-up patches on top add extra checking to reduce the
> > > > > > chance this kind of bug recurs.
> > > > > > 
> > > > > > Note: these are already (tentatively-pending review)
> > > > > > queued in my tree, so only review/ack
> > > > > > is necessary.
> > > > > 
> > > > > Why not let this go in via the migration tree?
> > > > 
> > > > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > > > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > > > testing this and have it on my tree already, might as well just merge.
> > > 
> > > IMO asking as a courtesy would've been better than just stating it.
> > 
> > Right, thanks for reminding me.
> > 
> > BTW, there is actually a good reason to special-case it: it's a CVE fix,
> > which I handle.  So they stay on my private queue and are passed
> > to vendors so vendors can fix downstreams, until making fix public is
> > cleared with all reporters and vendors.
> > After reporting is cleared, I try to collect acks but don't normally route
> > patches through separate queues - that would make it harder to
> > track the status which we need for CVEs.
> 
> Patch is public, so all of this doesn't really matter.
> 
> But: involving maintainers in their areas, even if the patch is
> embargoed, should be a pre-requisite.  I'm not sure if we're doing
> that, but please do that so patches get a proper review from the
> maintainers.

Involving more people means more back and forth with reporters which
must approve any disclosure.  If the issue isn't clear, I do involve
maintainers.  I send patches on list and try to merge them only after
they get ack from relevant people. I'm sorry, but this is as far as I
have the time to go.

> > I guess this specific one actually is well contained, so it could go in
> > through a specific tree if it had to.  In fact, it is still possible if
> > Juan says he prefers it so: I only expect to send pull request around
> > tomorrow or the day after that.
> 
> I'm sure we prefer migration patches go through the migration tree.

I'm sorry - I disagree. maintainer boundaries in qemu are quite flexible
because code is somewhat monolithic.  We prefer that patches are
reviewed by people that are familiar with it, that is for sure.  Which
tree is definitely secondary.  If there's a conflict, we can resolve it.
I doubt it will happen here.

> Also, this week I'm looking at the migration queue -- it's an
> unofficial split of maintenance duties between Juan and me while we're
> still trying to find out what works best.
> 

I don't know how am I supposed to know that.
Send patch to MAINTAINERS or something?

> > > > Which reminds me: we really should have someone in MAINTAINERS
> > > > for migration-related files.
> > > 
> > > There is, since last week.
> > 
> > That's good. I see Juan is listed there now, so all's well.
> 
> But that was well-known anyway :-)
> 
> 
>               Amit

-- 
MST