[PATCH 0/3] Fix use-after-free in block job reconnection code
Peter Krempa (3): qemu: migration: Don't use return value of qemuBlockJobUpdate qemuBlockJobUpdate: Remove return value qemuBlockJobRefreshJobs: Warn readers that 'job' may be invalid after update src/qemu/qemu_blockjob.c | 9 +++------ src/qemu/qemu_blockjob.h | 7 ++++--- src/qemu/qemu_migration.c | 17 +++++++---------- 3 files changed, 14 insertions(+), 19 deletions(-) -- 2.24.1
Upcoming patch will remove it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8a1801d408..bc280e856a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -510,7 +510,6 @@ qemuMigrationSrcNBDStorageCopyReady(virDomainObjPtr vm, { size_t i; size_t notReady = 0; - int status; for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; @@ -526,8 +525,8 @@ qemuMigrationSrcNBDStorageCopyReady(virDomainObjPtr vm, return -1; } - status = qemuBlockJobUpdate(vm, job, asyncJob); - if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { + qemuBlockJobUpdate(vm, job, asyncJob); + if (job->state == VIR_DOMAIN_BLOCK_JOB_FAILED) { qemuMigrationNBDReportMirrorError(job, disk->dst); virObjectUnref(job); return -1; @@ -567,7 +566,6 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm, size_t i; size_t active = 0; size_t completed = 0; - int status; bool failed = false; retry: @@ -582,8 +580,8 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm, if (!(job = qemuBlockJobDiskGetJob(disk))) continue; - status = qemuBlockJobUpdate(vm, job, asyncJob); - switch (status) { + qemuBlockJobUpdate(vm, job, asyncJob); + switch (job->state) { case VIR_DOMAIN_BLOCK_JOB_FAILED: if (check) { qemuMigrationNBDReportMirrorError(job, disk->dst); @@ -599,7 +597,7 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm, active++; } - if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) + if (job->state == VIR_DOMAIN_BLOCK_JOB_COMPLETED) completed++; virObjectUnref(job); @@ -650,11 +648,10 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - int status; int rv; - status = qemuBlockJobUpdate(vm, job, asyncJob); - switch (status) { + qemuBlockJobUpdate(vm, job, asyncJob); + switch (job->state) { case VIR_DOMAIN_BLOCK_JOB_FAILED: case VIR_DOMAIN_BLOCK_JOB_CANCELED: if (failNoJob) { -- 2.24.1
On Thu, Mar 26, 2020 at 01:39:39PM +0100, Peter Krempa wrote:
Upcoming patch will remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8a1801d408..bc280e856a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -510,7 +510,6 @@ qemuMigrationSrcNBDStorageCopyReady(virDomainObjPtr vm, { size_t i; size_t notReady = 0; - int status;
for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; @@ -526,8 +525,8 @@ qemuMigrationSrcNBDStorageCopyReady(virDomainObjPtr vm, return -1; }
- status = qemuBlockJobUpdate(vm, job, asyncJob); - if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { + qemuBlockJobUpdate(vm, job, asyncJob); + if (job->state == VIR_DOMAIN_BLOCK_JOB_FAILED) { qemuMigrationNBDReportMirrorError(job, disk->dst); virObjectUnref(job); return -1; @@ -567,7 +566,6 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm, size_t i; size_t active = 0; size_t completed = 0; - int status; bool failed = false;
retry: @@ -582,8 +580,8 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm, if (!(job = qemuBlockJobDiskGetJob(disk))) continue;
- status = qemuBlockJobUpdate(vm, job, asyncJob); - switch (status) { + qemuBlockJobUpdate(vm, job, asyncJob); + switch (job->state) { case VIR_DOMAIN_BLOCK_JOB_FAILED: if (check) { qemuMigrationNBDReportMirrorError(job, disk->dst); @@ -599,7 +597,7 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm, active++; }
- if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) + if (job->state == VIR_DOMAIN_BLOCK_JOB_COMPLETED) completed++;
virObjectUnref(job); @@ -650,11 +648,10 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - int status; int rv;
- status = qemuBlockJobUpdate(vm, job, asyncJob); - switch (status) { + qemuBlockJobUpdate(vm, job, asyncJob); + switch (job->state) { case VIR_DOMAIN_BLOCK_JOB_FAILED: case VIR_DOMAIN_BLOCK_JOB_CANCELED: if (failNoJob) { -- 2.24.1
Reviewed-by: Pavel Mores <pmores@redhat.com>
No callers use it any more. Additionally if qemuBlockJobUpdate was called with the last reference of the job e.g. in qemuBlockJobRefreshJobs, the reading of the job state would happen from freed memory. Reported-by: Pavel Mores <pmores@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 8 ++------ src/qemu/qemu_blockjob.h | 7 ++++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 21a043d369..6576f8721f 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1658,10 +1658,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, * * Update disk's mirror state in response to a block job event stored in * blockJobStatus by qemuProcessHandleBlockJob event handler. - * - * Returns the block job event processed or -1 if there was no pending event. */ -int +void qemuBlockJobUpdate(virDomainObjPtr vm, qemuBlockJobDataPtr job, int asyncJob) @@ -1669,14 +1667,12 @@ qemuBlockJobUpdate(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; if (job->newstate == -1) - return -1; + return; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) qemuBlockJobEventProcess(priv->driver, vm, job, asyncJob); else qemuBlockJobEventProcessLegacy(priv->driver, vm, job, asyncJob); - - return job->state; } diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 9264c70217..19498b5bd8 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -232,9 +232,10 @@ int qemuBlockJobRefreshJobs(virQEMUDriverPtr driver, virDomainObjPtr vm); -int qemuBlockJobUpdate(virDomainObjPtr vm, - qemuBlockJobDataPtr job, - int asyncJob); +void +qemuBlockJobUpdate(virDomainObjPtr vm, + qemuBlockJobDataPtr job, + int asyncJob); void qemuBlockJobSyncBegin(qemuBlockJobDataPtr job); void qemuBlockJobSyncEnd(virDomainObjPtr vm, -- 2.24.1
On Thu, Mar 26, 2020 at 01:39:40PM +0100, Peter Krempa wrote:
No callers use it any more. Additionally if qemuBlockJobUpdate was called with the last reference of the job e.g. in qemuBlockJobRefreshJobs, the reading of the job state would happen from freed memory.
Reported-by: Pavel Mores <pmores@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 8 ++------ src/qemu/qemu_blockjob.h | 7 ++++--- 2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 21a043d369..6576f8721f 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1658,10 +1658,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, * * Update disk's mirror state in response to a block job event stored in * blockJobStatus by qemuProcessHandleBlockJob event handler. - * - * Returns the block job event processed or -1 if there was no pending event. */ -int +void qemuBlockJobUpdate(virDomainObjPtr vm, qemuBlockJobDataPtr job, int asyncJob) @@ -1669,14 +1667,12 @@ qemuBlockJobUpdate(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData;
if (job->newstate == -1) - return -1; + return;
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) qemuBlockJobEventProcess(priv->driver, vm, job, asyncJob); else qemuBlockJobEventProcessLegacy(priv->driver, vm, job, asyncJob); - - return job->state; }
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 9264c70217..19498b5bd8 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -232,9 +232,10 @@ int qemuBlockJobRefreshJobs(virQEMUDriverPtr driver, virDomainObjPtr vm);
-int qemuBlockJobUpdate(virDomainObjPtr vm, - qemuBlockJobDataPtr job, - int asyncJob); +void +qemuBlockJobUpdate(virDomainObjPtr vm, + qemuBlockJobDataPtr job, + int asyncJob);
void qemuBlockJobSyncBegin(qemuBlockJobDataPtr job); void qemuBlockJobSyncEnd(virDomainObjPtr vm, -- 2.24.1
Reviewed-by: Pavel Mores <pmores@redhat.com>
Add a comment noting that job update can cause the pointer to be invalid and thus should not be accessed after. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 6576f8721f..2032c0c1c5 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -571,6 +571,7 @@ qemuBlockJobRefreshJobs(virQEMUDriverPtr driver, if (job->newstate != -1) qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + /* 'job' may be invalid after this update */ } /* remove data for job which qemu didn't report (the algorithm is -- 2.24.1
On Thu, Mar 26, 2020 at 01:39:41PM +0100, Peter Krempa wrote:
Add a comment noting that job update can cause the pointer to be invalid and thus should not be accessed after.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 6576f8721f..2032c0c1c5 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -571,6 +571,7 @@ qemuBlockJobRefreshJobs(virQEMUDriverPtr driver,
if (job->newstate != -1) qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + /* 'job' may be invalid after this update */ }
/* remove data for job which qemu didn't report (the algorithm is -- 2.24.1
Reviewed-by: Pavel Mores <pmores@redhat.com>
On 26. 3. 2020 13:39, Peter Krempa wrote:
Peter Krempa (3): qemu: migration: Don't use return value of qemuBlockJobUpdate qemuBlockJobUpdate: Remove return value qemuBlockJobRefreshJobs: Warn readers that 'job' may be invalid after update
src/qemu/qemu_blockjob.c | 9 +++------ src/qemu/qemu_blockjob.h | 7 ++++--- src/qemu/qemu_migration.c | 17 +++++++---------- 3 files changed, 14 insertions(+), 19 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Michal Prívozník -
Pavel Mores -
Peter Krempa