[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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();


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]