]> git.ipfire.org Git - thirdparty/libvirt.git/commit
util: eliminate "use after free" in callers of virNetDevLinkDump
authorLaine Stump <laine@laine.org>
Wed, 15 Oct 2014 22:49:01 +0000 (00:49 +0200)
committerLaine Stump <laine@laine.org>
Thu, 13 Nov 2014 17:43:35 +0000 (12:43 -0500)
commit3690a782320a32c2877482475e44e52e3867ce3c
tree6a587b013ad256b721d19799a9f1138d21e6142a
parent3d751cdcdbfac95b4a39a7db1b6e12e20838cb65
util: eliminate "use after free" in callers of virNetDevLinkDump

virNetDevLinkDump() gets a message from netlink into "resp", then
calls nlmsg_parse() to fill the table "tb" with pointers into resp. It
then returns tb to its caller, but not before freeing the buffer at
resp. That means that all the callers of virNetDevLinkDump() are
examining memory that has already been freed. This can be verified by
filling the buffer at resp with garbage prior to freeing it (or, I
suppose, just running libvirtd under valgrind) then performing some
operation that calls virNetDevLinkDump().

The upstream commit log incorrectly states that the code has been like
this ever since virNetDevLinkDump() was written. In reality, the
problem was introduced with commit e95de74d, first in libvirt-1.0.5,
which was attempting to eliminate a typecast that caused compiler
warnings. It has only been pure luck (or maybe a lack of heavy load,
and/or maybe an allocation algorithm in malloc() that delays re-use of
just-freed memory) that has kept this from causing errors, for example
when configuring a PCI passthrough or macvtap passthrough network
interface.

The solution taken in this patch is the simplest - just return resp to
the caller along with tb, then have the caller free it after they are
finished using the data (pointers) in tb. I alternately could have
made a cleaner interface by creating a new struct that put tb and resp
together along with a vir*Free() function for it, but this function is
only used in a couple places, and I'm not sure there will be
additional new uses of virNetDevLinkDump(), so the value of adding a
new type, extra APIs, etc. is dubious.

(cherry picked from commit f9f9699f40729556238b905f67a7d6f68c084f6a)

Conflicts:
src/util/virnetdevvportprofile.c - whitespace/copyright change
src/util/virnetdev.c
src/util/virnetdev.h
src/util/virnetdevvportprofile.c