Re: [PATCH for-5.1?] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Eric Blake <eblake@redhat.com>]

On 8/5/20 4:22 AM, Tuguoyi wrote:
> When calculating the offset, the result of left shift operation will be promoted
> to type int64 automatically because the left operand of + operator is uint64_t.
> but the result after integer promotion may be produce an error value for us and
> trigger the following asserting error.
> 
> For example, consider i=0x2000, cluster_bits=18, the result of left shift
> operation will be 0x80000000. Cause argument i is of signed integer type,
> the result is automatically promoted to 0xffffffff80000000 which is not
> we expected
> 
> The way to trigger the assertion error:
>    qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G

I wonder if it is worth an iotest addition to cover this.

> 
> This patch fix it by casting @i to uint64_t before doing left shift operation
> 
> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
> ---
>   block/qcow2-cluster.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

While this appears to be long-standing, rather than a regression in 5.1, 
this would be worth getting into -rc3 today if we still have time (if 
not, slipping to 5.2 is okay).

> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index a677ba9..550850b 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -980,7 +980,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
>   
>       assert(l2_index + m->nb_clusters <= s->l2_slice_size);
>       for (i = 0; i < m->nb_clusters; i++) {
> -        uint64_t offset = cluster_offset + (i << s->cluster_bits);
> +        uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);

We have:
offset = uint64_t + (int << int)

which is indeed a cause of unwanted sign extension.

If I'm not mistaken, it would also be feasible to fix this by changing 
qcow2.h to fix typedef struct BDRVQcow2State to use an unsigned type for 
cluster_bits (the way we already do for struct QCowHeader), avoiding the 
need for a cast here.

But if we're trying to get this in today, rather than auditing all other 
uses of BDRVQcow2State for incorrect typing,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org