Re: [libvirt] [PATCH] uml: avoid crash on partial read
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();