On Thu, Mar 07, 2019 at 14:38:03 +0100, Michal Privoznik wrote:

Problem is that if there are no signatures for a CPU, then we still allocate cpu->signatures (even though with size 0). Later, we access cpu->signatures[0] if cpu->signatures is not NULL.

Invalid read of size 4 at 0x5F439D7: virCPUx86Translate (cpu_x86.c:2930) by 0x5F3C239: virCPUTranslate (cpu.c:927) by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870) ... Address 0xf752d40 is 0 bytes after a block of size 0 alloc'd at 0x4C30EC6: calloc (vg_replace_malloc.c:711) by 0x5DBDE4E: virAllocN (viralloc.c:190) by 0x5F3E4FA: x86ModelCopySignatures (cpu_x86.c:990) by 0x5F3E60F: x86ModelCopy (cpu_x86.c:1008) by 0x5F3E7CB: x86ModelFromCPU (cpu_x86.c:1068) by 0x5F4397E: virCPUx86Translate (cpu_x86.c:2922) by 0x5F3C239: virCPUTranslate (cpu.c:927) by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870) ... Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_x86.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 92941d1287..c8697819ab 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -987,6 +987,9 @@ x86ModelCopySignatures(virCPUx86ModelPtr dst, { size_t i;

+ if (src->nsignatures == 0) + return 0; + if (VIR_ALLOC_N(dst->signatures, src->nsignatures) < 0) return -1;

Yes, this is what it should have looked like from the beginning :-)

@@ -2926,7 +2929,7 @@ virCPUx86Translate(virCPUDefPtr cpu, virCPUx86DataAddCPUIDInt(&model->data, &model->vendor->cpuid) < 0) goto cleanup;

- if (model->signatures && + if (model->nsignatures && x86DataAddSignature(&model->data, model->signatures[0]) < 0) goto cleanup;

I don't think this is necessary, but if we're going to change this, we should change all places where we decide on model->signatures. Anyway, the first hunk is sufficient to fix this bug so with the second hunk removed: Reviewed-by: Jiri Denemark <jdenemar@redhat.com>