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

[Greg Kurz]

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
> 
>