Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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. Queued up patches 2-4 in my tree, thanks. Amit
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
On Wed, Nov 12, 2014 at 11:44:35AM +0200, 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. Michael S. Tsirkin (4): Reviewed-by: Amos Kong ak...@redhat.com migration: fix parameter validation on ram load exec: add wrapper for host pointer access cpu: assert host pointer offset within block cpu: verify that block-host is set include/exec/cpu-all.h | 7 +++ arch_init.c| 5 +++-- exec.c | 10 +- 3 files changed, 15 insertions(+), 7 deletions(-) -- MST -- Amos. signature.asc Description: Digital signature
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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. Any more acks for this? Dave? Also, Michael agreed to just push patch 1 for now, and 2-4 can go in later (2.3)? Michael, is that fine with you? Amit
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
On (Mon) 17 Nov 2014 [14:36:45], Michael S. Tsirkin wrote: On Mon, Nov 17, 2014 at 05:50:34PM +0530, Amit Shah wrote: On (Mon) 17 Nov 2014 [13:48:58], Michael S. Tsirkin wrote: 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. The other aspect of the thing is sub-optimal, or patches with bugs, get pushed in, because the maintainers didn't get involved. Patches don't get merged before they are on list for a while. I typically ping people if I don't get acks. BTW I was talking about embargoed bugs / patches. That's not relevant for this discussion. I'll create a new thread to discuss qemu's security policy for embargoed bugs. Amit
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
* Amit Shah (amit.s...@redhat.com) 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. Any more acks for this? Dave? Yep, Reviewed-by: Dr. David Alan Gilbert dgilb...@redhat.com (for the set) Also, Michael agreed to just push patch 1 for now, and 2-4 can go in later (2.3)? Michael, is that fine with you? Amit Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
On Tue, Nov 18, 2014 at 02:31:34PM +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. Any more acks for this? Dave? Also, Michael agreed to just push patch 1 for now, and 2-4 can go in later (2.3)? Michael, is that fine with you? Amit I'm fine with putting them off. Someone wants to take them or should I? -- MST
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
* Michael S. Tsirkin (m...@redhat.com) wrote: On Tue, Nov 18, 2014 at 02:31:34PM +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. Any more acks for this? Dave? Also, Michael agreed to just push patch 1 for now, and 2-4 can go in later (2.3)? Michael, is that fine with you? Amit I'm fine with putting them off. Someone wants to take them or should I? exec.c stuff seems to be pretty much all over, so I think you're as good as anyone. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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? Amit 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. Which reminds me: we really should have someone in MAINTAINERS for migration-related files. -- MST
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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. Which reminds me: we really should have someone in MAINTAINERS for migration-related files. There is, since last week. Amit
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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. 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. Which reminds me: we really should have someone in MAINTAINERS for migration-related files. There is, since last week. Amit That's good. I see Juan is listed there now, so all's well. -- MST
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
On (Mon) 17 Nov 2014 [13:48:58], Michael S. Tsirkin wrote: 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. The other aspect of the thing is sub-optimal, or patches with bugs, get pushed in, because the maintainers didn't get involved. Also, maintainers don't like code being changed behind their backs... But if you're overloaded with handling security issues, we can help identify a co-maintainer as well. 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. For well-maintained areas, I'm not sure I agree with the flexibility argument. Juan has been maintaining the migration tree for a long while now. 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. Right now you don't need to -- I just pick patches up and pass them on to Juan, who does the pull req. Send patch to MAINTAINERS or something? I'm going to add myself to migration maintainers, yes. But that's secondary; the pull reqs still go via Juan. Amit
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
On Mon, Nov 17, 2014 at 05:50:34PM +0530, Amit Shah wrote: On (Mon) 17 Nov 2014 [13:48:58], Michael S. Tsirkin wrote: 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. The other aspect of the thing is sub-optimal, or patches with bugs, get pushed in, because the maintainers didn't get involved. Patches don't get merged before they are on list for a while. I typically ping people if I don't get acks. Also, maintainers don't like code being changed behind their backs... no one is changing code in secret here. So don't turn your back - review patches :0 FYI I'm sometimes merging patches on list that don't get reviews for weeks - if they seem to make sense. But if you're overloaded with handling security issues, we can help identify a co-maintainer as well. Sure. I don't think we need more process to slow down the flow of patches even more. 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. For well-maintained areas, I'm not sure I agree with the flexibility argument. Juan has been maintaining the migration tree for a long while now. Did you look at the patches? Most of them touch files like exec.c which is all over the place. 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. Right now you don't need to -- I just pick patches up and pass them on to Juan, who does the pull req. Send patch to MAINTAINERS or something? I'm going to add myself to migration maintainers, yes. But that's secondary; the pull reqs still go via Juan. Amit
Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840
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? Amit
