Eric Blake wrote:
Coverity detected a potential dereference of uninitialized memory if recvfrom got cut short.
* src/uml/uml_driver.c (umlMonitorCommand): Validate complete read prior to dereferencing res. ---
The patch borrows ideas from macvtap.c, the only other file in libvirt that currently uses recvfrom.
I did not analyze whether this is a security hole, where a malicious UDP packet could intentionally force the dereferencing of uninitialized memory to misbehave in a controlled manner.
That warning highlights a problem even with a complete read (and malicious content). It exposes what is at the very least a risk of DoS, since we use the just-read (and not sanity-checked) res.length as the new buffer size in VIR_REALLOC_N.
src/uml/uml_driver.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bbea429..eec239f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -733,14 +733,24 @@ static int umlMonitorCommand(virConnectPtr conn, }
do { + ssize_t nbytes; addrlen = sizeof(addr); - if (recvfrom(priv->monitor, &res, sizeof res, 0, - (struct sockaddr *)&addr, &addrlen) < 0) { + nbytes = recvfrom(priv->monitor, &res, sizeof res, 0, + (struct sockaddr *)&addr, &addrlen) < 0; + if (nbytes < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } + if (nbytes < sizeof res) { + virReportSystemError(errno, + _("incomplete reply %s"), + cmd); + goto error; + }
ACK to the above patch, but we need one more: Please add a check here to ensure that res.length is no larger than sizeof res.data.
if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError();