[Libvir] PATCH: Avoid buffer out of bounds access in Xen capabilities
The Xen driver uses a regex to process the hypervisor capabilities data "(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?"; notice how the last match group, however, is optional due to the '?'. The code processing matches does not check to see if the match is present or not, and just indexes the string on match 3 if (strncmp (&token[subs[3].rm_so], "p", 1) == 0) Unfortunately, subs[3].rm_so is -1 if the match was not present, so we're doing an out of bounds array access here. This is fairly harmless, but it is still good to fix it. So this patch adds a check for -1 before accessing the match. I also replace the strncmp() calls with a call to the brand new STRPREFIX() convenience macro Dan. Index: src/xen_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xen_internal.c,v retrieving revision 1.119 diff -u -r1.119 xen_internal.c --- src/xen_internal.c 10 Apr 2008 16:54:54 -0000 1.119 +++ src/xen_internal.c 28 Apr 2008 22:07:12 -0000 @@ -2349,28 +2349,31 @@ if (regexec (&xen_cap_rec, token, sizeof subs / sizeof subs[0], subs, 0) == 0) { - int hvm = strncmp (&token[subs[1].rm_so], "hvm", 3) == 0; + int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm"); const char *model; int bits, pae = 0, nonpae = 0, ia64_be = 0; - if (strncmp (&token[subs[2].rm_so], "x86_32", 6) == 0) { + + if (STRPREFIX(&token[subs[2].rm_so], "x86_32")) { model = "i686"; bits = 32; - if (strncmp (&token[subs[3].rm_so], "p", 1) == 0) + if (subs[3].rm_so != -1 && + STRPREFIX(&token[subs[3].rm_so], "p")) pae = 1; else nonpae = 1; } - else if (strncmp (&token[subs[2].rm_so], "x86_64", 6) == 0) { + else if (STRPREFIX(&token[subs[2].rm_so], "x86_64")) { model = "x86_64"; bits = 64; } - else if (strncmp (&token[subs[2].rm_so], "ia64", 4) == 0) { + else if (STRPREFIX(&token[subs[2].rm_so], "ia64")) { model = "ia64"; bits = 64; - if (strncmp (&token[subs[3].rm_so], "be", 2) == 0) + if (subs[3].rm_so != -1 && + STRPREFIX(&token[subs[3].rm_so], "be")) ia64_be = 1; } - else if (strncmp (&token[subs[2].rm_so], "powerpc64", 4) == 0) { + else if (STRPREFIX(&token[subs[2].rm_so], "powerpc64")) { model = "ppc64"; bits = 64; } else { @@ -2380,7 +2383,7 @@ /* Search for existing matching (model,hvm) tuple */ for (i = 0 ; i < nr_guest_archs ; i++) { - if (!strcmp(guest_archs[i].model, model) && + if (STREQ(guest_archs[i].model, model) && guest_archs[i].hvm == hvm) { break; } -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Apr 28, 2008 at 11:42:47PM +0100, Daniel P. Berrange wrote:
The Xen driver uses a regex to process the hypervisor capabilities data
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
notice how the last match group, however, is optional due to the '?'. The code processing matches does not check to see if the match is present or not, and just indexes the string on match 3
if (strncmp (&token[subs[3].rm_so], "p", 1) == 0)
Unfortunately, subs[3].rm_so is -1 if the match was not present, so we're doing an out of bounds array access here. This is fairly harmless, but it is still good to fix it. So this patch adds a check for -1 before accessing the match. I also replace the strncmp() calls with a call to the brand new STRPREFIX() convenience macro
Okidoc, i assume valgrind spotted that, that's fairly well hidden ... +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Apr 29, 2008 at 02:46:14AM -0400, Daniel Veillard wrote:
On Mon, Apr 28, 2008 at 11:42:47PM +0100, Daniel P. Berrange wrote:
The Xen driver uses a regex to process the hypervisor capabilities data
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
notice how the last match group, however, is optional due to the '?'. The code processing matches does not check to see if the match is present or not, and just indexes the string on match 3
if (strncmp (&token[subs[3].rm_so], "p", 1) == 0)
Unfortunately, subs[3].rm_so is -1 if the match was not present, so we're doing an out of bounds array access here. This is fairly harmless, but it is still good to fix it. So this patch adds a check for -1 before accessing the match. I also replace the strncmp() calls with a call to the brand new STRPREFIX() convenience macro
Okidoc, i assume valgrind spotted that, that's fairly well hidden ...
Yeah, and valgrind only finds it on i386 - not x86_64, which is why I didn't spot it for sooo long ! Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Daniel Veillard