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

[Peter Maydell]

On Fri, 12 Jul 2024 at 16:18, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> 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:
> >>      #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.

It does run on all the code, but if the code changes enough
that can cause it to close out the old issue on the old code
and create a new issue in the system for the new code (which I
then notice because I look at newly-found things to triage them).
So things like refactorings and moving code around can cause
issues to show up.

The other reason this might have shown up now is that they seem
to have added a new check type which flags up a lot of "possible
overflow" errors, so there's a huge pile of new issues for old
code that I'm gradually going through to see which are false
positives and which we should look at.

-- PMM