Re: [PULL v3 52/85] contrib/vhost-user-*: use QEMU bswap helper functions

[Stefano Garzarella]

On Fri, Jul 12, 2024 at 03:24:47PM GMT, Peter Maydell wrote:
> On Wed, 3 Jul 2024 at 23:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> > From: Stefano Garzarella <sgarzare@redhat.com>
> >
> > Let's replace the calls to le*toh() and htole*() with qemu/bswap.h
> > helpers to make the code more portable.
> >
> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > Message-Id: <20240618100447.145697-1-sgarzare@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  contrib/vhost-user-blk/vhost-user-blk.c |  9 +++++----
> >  contrib/vhost-user-input/main.c         | 16 ++++++++--------
> >  2 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/contrib/vhost-user-blk/vhost-user-blk.c 
> > b/contrib/vhost-user-blk/vhost-user-blk.c
> > index a8ab9269a2..9492146855 100644
> > --- a/contrib/vhost-user-blk/vhost-user-blk.c
> > +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> > @@ -16,6 +16,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include "qemu/bswap.h"
> >  #include "standard-headers/linux/virtio_blk.h"
> >  #include "libvhost-user-glib.h"
> >
> > @@ -194,8 +195,8 @@ vub_discard_write_zeroes(VubReq *req, struct iovec *iov, 
> > uint32_t iovcnt,
> >      #if defined(__linux__) && defined(BLKDISCARD) && defined(BLKZEROOUT)
> >      VubDev *vdev_blk = req->vdev_blk;
> >      desc = buf;
> > -    uint64_t range[2] = { le64toh(desc->sector) << 9,
> > -                          le32toh(desc->num_sectors) << 9 };
> > +    uint64_t range[2] = { le64_to_cpu(desc->sector) << 9,
> > +                          le32_to_cpu(desc->num_sectors) << 9 };
>
> Hi; Coverity points out that this does a 32-bit shift, not a
> 64-bit one, so it could unintentionally chop the high bits off
> if desc->num_sectors is big enough (CID 1549454).
> We could fix this by making it
>    (uint64_t)le32_to_cpu(desc->num_sectors) << 9
> I think.

Yep, I think so! I'll send a patch.

> (It looks like the issue was already there before, so

Yes, it is pre-existing to this patch, introduced from the beginning 
with commit caa1ee4313 ("vhost-user-blk: add discard/write zeroes 
features support")

> Coverity has just noticed it because of the code change here.)

Ah, I thought it ran on all the code, not just the changes.

Thanks,
Stefano