2:21 p.m.
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(a)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
2:43 p.m.
New subject: [libvirt] [PATCH] x86Decode: avoid NULL-dereference upon questionable input
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:59 p.m.
New subject: [libvirt] [PATCH] x86Decode: avoid NULL-dereference upon questionable input
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.
5414
days inactive
5414
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Jim Meyering