9pfs/proxy: Check return value of proxy_marshal()

[PATCH] 9pfs/proxy: Check return value of proxy_marshal()

Posted by Greg Kurz 3 years, 4 months ago

This should always successfully write exactly two 32-bit integers.
Make it clear with an assert(), like v9fs_receive_status() and
v9fs_receive_response() already do when unmarshalling the same
header.

Fixes: Coverity CID 1438968
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-proxy.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 6f598a0f111c..4aa4e0a3baa0 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -537,7 +537,8 @@ static int v9fs_request(V9fsProxy *proxy, int type, void *response, ...)
     }
 
     /* marshal the header details */
-    proxy_marshal(iovec, 0, "dd", header.type, header.size);
+    retval = proxy_marshal(iovec, 0, "dd", header.type, header.size);
+    assert(retval == 4 * 2);
     header.size += PROXY_HDR_SZ;
 
     retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);

Re: [PATCH] 9pfs/proxy: Check return value of proxy_marshal()

Posted by Christian Schoenebeck via 3 years, 4 months ago

On Montag, 11. Januar 2021 10:49:56 CET Greg Kurz wrote:
> This should always successfully write exactly two 32-bit integers.
> Make it clear with an assert(), like v9fs_receive_status() and
> v9fs_receive_response() already do when unmarshalling the same
> header.
> 
> Fixes: Coverity CID 1438968
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

What's your workload Greg, are you able to push this through your queue?

It's time that I signup for coverity. I'm doing that now before I forget it  
again.

> ---
>  hw/9pfs/9p-proxy.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 6f598a0f111c..4aa4e0a3baa0 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -537,7 +537,8 @@ static int v9fs_request(V9fsProxy *proxy, int type, void
> *response, ...) }
> 
>      /* marshal the header details */
> -    proxy_marshal(iovec, 0, "dd", header.type, header.size);
> +    retval = proxy_marshal(iovec, 0, "dd", header.type, header.size);
> +    assert(retval == 4 * 2);
>      header.size += PROXY_HDR_SZ;
> 
>      retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);

Best regards,
Christian Schoenebeck

Re: [PATCH] 9pfs/proxy: Check return value of proxy_marshal()

Posted by Greg Kurz 3 years, 4 months ago

On Mon, 11 Jan 2021 14:15:17 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 11. Januar 2021 10:49:56 CET Greg Kurz wrote:
> > This should always successfully write exactly two 32-bit integers.
> > Make it clear with an assert(), like v9fs_receive_status() and
> > v9fs_receive_response() already do when unmarshalling the same
> > header.
> > 
> > Fixes: Coverity CID 1438968
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 
> What's your workload Greg, are you able to push this through your queue?
> 

I'll take care of the security issue first, likely later today or tomorrow.
It is generally recommended to have separate PRs for CVEs, in order to
ease the backport effort of downstream vendors.

No hurry for this patch though. It isn't even a bug fix : we really can't
get an error at this point since previous calls to proxy_marshal() in this
function obviously succeeded at writing stuff at higher offsets... So this
is really a cosmetic only change to make Coverity happy.

I might be able to send a PR with this next week or the week after I guess.

> It's time that I signup for coverity. I'm doing that now before I forget it  
> again.
> 
> > ---
> >  hw/9pfs/9p-proxy.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> > index 6f598a0f111c..4aa4e0a3baa0 100644
> > --- a/hw/9pfs/9p-proxy.c
> > +++ b/hw/9pfs/9p-proxy.c
> > @@ -537,7 +537,8 @@ static int v9fs_request(V9fsProxy *proxy, int type, void
> > *response, ...) }
> > 
> >      /* marshal the header details */
> > -    proxy_marshal(iovec, 0, "dd", header.type, header.size);
> > +    retval = proxy_marshal(iovec, 0, "dd", header.type, header.size);
> > +    assert(retval == 4 * 2);
> >      header.size += PROXY_HDR_SZ;
> > 
> >      retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
> 
> Best regards,
> Christian Schoenebeck
> 
>

Re: [PATCH] 9pfs/proxy: Check return value of proxy_marshal()

Posted by Greg Kurz 3 years, 3 months ago

On Mon, 11 Jan 2021 10:49:56 +0100
Greg Kurz <groug@kaod.org> wrote:

> This should always successfully write exactly two 32-bit integers.
> Make it clear with an assert(), like v9fs_receive_status() and
> v9fs_receive_response() already do when unmarshalling the same
> header.
> 
> Fixes: Coverity CID 1438968
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---

Applied to https://gitlab.com/gkurz/qemu/-/tree/9p-next

>  hw/9pfs/9p-proxy.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 6f598a0f111c..4aa4e0a3baa0 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -537,7 +537,8 @@ static int v9fs_request(V9fsProxy *proxy, int type, void *response, ...)
>      }
>  
>      /* marshal the header details */
> -    proxy_marshal(iovec, 0, "dd", header.type, header.size);
> +    retval = proxy_marshal(iovec, 0, "dd", header.type, header.size);
> +    assert(retval == 4 * 2);
>      header.size += PROXY_HDR_SZ;
>  
>      retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
> 
>