cpu: Don't access invalid memory in virCPUx86Translate

[libvirt] [PATCH] cpu: Don't access invalid memory in virCPUx86Translate

Posted by Michal Privoznik 5 years, 1 month ago

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;
 
@@ -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;
 
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cpu: Don't access invalid memory in virCPUx86Translate

Posted by Jiri Denemark 5 years, 1 month ago

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>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cpu: Don't access invalid memory in virCPUx86Translate

Posted by Ján Tomko 5 years, 1 month ago

On Thu, Mar 07, 2019 at 02:38:03PM +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>

One sign-off ought to be enough for everybody.

Jano

>---
> src/cpu/cpu_x86.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list