[libvirt] [PATCH] x86Decode: avoid NULL-dereference upon questionable input
Clang warned about the potential NULL-dereference via the STREQ/strcmp below. models[i] could be NULL. Even "models" could be NULL, and the "allowed = ..." test makes that appear to be deliberately allowed. The change below is the least invasive and cleanest I could come up with, assuming there is no need to diagnose (e.g., by returning -1) the condition of a NULL models[i] pointer.
From 623ac546a93f3e5b554647b05524b5041a642530 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Sun, 28 Feb 2010 13:34:06 +0100 Subject: [PATCH] x86Decode: avoid NULL-dereference upon questionable input
* src/cpu/cpu_x86.c (x86Decode): Don't dereference NULL when passed a NULL "models" pointer, or when passed a nonzero "nmodels" value and a corresponding NULL models[i]. --- src/cpu/cpu_x86.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 2194c32..b263629 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -957,89 +957,89 @@ x86Compare(virCPUDefPtr host, virCPUDefPtr cpu) { return x86Compute(host, cpu, NULL); } static virCPUCompareResult x86GuestData(virCPUDefPtr host, virCPUDefPtr guest, union cpuData **data) { return x86Compute(host, guest, data); } static int x86Decode(virCPUDefPtr cpu, const union cpuData *data, const char **models, unsigned int nmodels) { int ret = -1; struct x86_map *map; const struct x86_model *candidate; virCPUDefPtr cpuCandidate; virCPUDefPtr cpuModel = NULL; struct cpuX86cpuid *cpuid; int i; if (data == NULL || (map = x86LoadMap()) == NULL) return -1; candidate = map->models; while (candidate != NULL) { bool allowed = (models == NULL); for (i = 0; i < candidate->ncpuid; i++) { cpuid = x86DataCpuid(data, candidate->cpuid[i].function); if (cpuid == NULL || !x86cpuidMatchMasked(cpuid, candidate->cpuid + i)) goto next; } for (i = 0; i < nmodels; i++) { - if (STREQ(models[i], candidate->name)) { + if (models && models[i] && STREQ(models[i], candidate->name)) { allowed = true; break; } } if (!allowed) { VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring", candidate->name); goto next; } if (!(cpuCandidate = x86DataToCPU(data, candidate, map))) goto out; if (cpuModel == NULL || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; } else virCPUDefFree(cpuCandidate); next: candidate = candidate->next; } if (cpuModel == NULL) { virCPUReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot find suitable CPU model for given data")); goto out; } cpu->model = cpuModel->model; cpu->nfeatures = cpuModel->nfeatures; cpu->features = cpuModel->features; VIR_FREE(cpuModel); ret = 0; out: x86MapFree(map); virCPUDefFree(cpuModel); return ret; } -- 1.7.0.1.414.g89213d
According to Jim Meyering on 3/1/2010 1:21 PM:
Clang warned about the potential NULL-dereference via the STREQ/strcmp below. models[i] could be NULL. Even "models" could be NULL, and the "allowed = ..." test makes that appear to be deliberately allowed.
This same function was also listed by coverity, but only for models, not models[i].
The change below is the least invasive and cleanest I could come up with, assuming there is no need to diagnose (e.g., by returning -1) the condition of a NULL models[i] pointer.
while (candidate != NULL) { bool allowed = (models == NULL);
for (i = 0; i < candidate->ncpuid; i++) { cpuid = x86DataCpuid(data, candidate->cpuid[i].function); if (cpuid == NULL || !x86cpuidMatchMasked(cpuid, candidate->cpuid + i)) goto next; }
for (i = 0; i < nmodels; i++) { - if (STREQ(models[i], candidate->name)) { + if (models && models[i] && STREQ(models[i], candidate->name)) {
Isn't the intent that (models==NULL) iff (nmodels==0)? In which case, this code is unreachable if models is NULL. But your patch certainly is the least-invasive possible, and while it is only a false positive for well-formed arguments, I didn't spend time checking all clients of x86Decode to see if there is ever a possibility of bad arguments. ACK -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
According to Jim Meyering on 3/1/2010 1:21 PM:
Clang warned about the potential NULL-dereference via the STREQ/strcmp below. models[i] could be NULL. Even "models" could be NULL, and the "allowed = ..." test makes that appear to be deliberately allowed.
This same function was also listed by coverity, but only for models, not models[i].
Yes, I was disappointed to see Coverity missed that.
The change below is the least invasive and cleanest I could come up with, assuming there is no need to diagnose (e.g., by returning -1) the condition of a NULL models[i] pointer.
while (candidate != NULL) { bool allowed = (models == NULL);
for (i = 0; i < candidate->ncpuid; i++) { cpuid = x86DataCpuid(data, candidate->cpuid[i].function); if (cpuid == NULL || !x86cpuidMatchMasked(cpuid, candidate->cpuid + i)) goto next; }
for (i = 0; i < nmodels; i++) { - if (STREQ(models[i], candidate->name)) { + if (models && models[i] && STREQ(models[i], candidate->name)) {
Isn't the intent that (models==NULL) iff (nmodels==0)?
That is the intent, but the code at this level does not detect the mismatch. I think someone made a change recently to protect us at a higher (cpu-independent) level. But that doesn't help us here, if a new caller of this function violates those higher-level constraints.
In which case, this code is unreachable if models is NULL. But your patch certainly is the least-invasive possible, and while it is only a false positive for well-formed arguments, I didn't spend time checking all clients of x86Decode to see if there is ever a possibility of bad arguments.
ACK
Thanks.
participants (2)
-
Eric Blake -
Jim Meyering