Re: [Qemu-devel] [PATCH v5 5/5] 9pfs: handle transport errors in pdu_complete()

[Stefano Stabellini]

On Wed, 28 Jun 2017, Greg Kurz wrote:
> Contrary to what is written in the comment, a buggy guest can misconfigure
> the transport buffers and pdu_marshal() may return an error.  If this ever
> happens, it is up to the transport layer to handle the situation (9P is
> transport agnostic).
> 
> This fixes Coverity issue CID1348518.
> 
> Signed-off-by: Greg Kurz <address@hidden>

Reviewed-by: Stefano Stabellini <address@hidden>


> ---
>  hw/9pfs/9p.c |   23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index da0d6da65b45..f7871e6f3e5a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -624,15 +624,11 @@ void pdu_free(V9fsPDU *pdu)
>      QLIST_INSERT_HEAD(&s->free_list, pdu, next);
>  }
>  
> -/*
> - * We don't do error checking for pdu_marshal/unmarshal here
> - * because we always expect to have enough space to encode
> - * error details
> - */
>  static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
>  {
>      int8_t id = pdu->id + 1; /* Response */
>      V9fsState *s = pdu->s;
> +    int ret;
>  
>      if (len < 0) {
>          int err = -len;
> @@ -644,11 +640,19 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, 
> ssize_t len)
>              str.data = strerror(err);
>              str.size = strlen(str.data);
>  
> -            len += pdu_marshal(pdu, len, "s", &str);
> +            ret = pdu_marshal(pdu, len, "s", &str);
> +            if (ret < 0) {
> +                goto out_notify;
> +            }
> +            len += ret;
>              id = P9_RERROR;
>          }
>  
> -        len += pdu_marshal(pdu, len, "d", err);
> +        ret = pdu_marshal(pdu, len, "d", err);
> +        if (ret < 0) {
> +            goto out_notify;
> +        }
> +        len += ret;
>  
>          if (s->proto_version == V9FS_PROTO_2000L) {
>              id = P9_RLERROR;
> @@ -657,12 +661,15 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, 
> ssize_t len)
>      }
>  
>      /* fill out the header */
> -    pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
> +    if (pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag) < 0) {
> +        goto out_notify;
> +    }
>  
>      /* keep these in sync */
>      pdu->size = len;
>      pdu->id = id;
>  
> +out_notify:
>      pdu->s->transport->push_and_notify(pdu);
>  
>      /* Now wakeup anybody waiting in flush for this request */
>