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

[Kevin Wolf]

Am 05.08.2020 um 15:44 hat Alberto Garcia geschrieben:
> On Wed 05 Aug 2020 11:22:58 AM CEST, Tuguoyi wrote:
> > This patch fix it by casting @i to uint64_t before doing left shift
> > operation
> 
> The patch seems fine and I also think that it's perhaps worth a test
> case (although it only seems to happen with preallocation=falloc or full
> so the test would need to generate very large files).
> 
> But I also wonder if there are other cases where this can happen.
> 
> nb_clusters is an int and there are more cases of
> 
>     nb_clusters << s->cluster_bits
> 
> I can see at least these: handle_alloc(), qcow2_free_any_clusters(),
> qcow2_alloc_cluster_abort().

Actuallyx, handle_alloc() and everything that comes from it should be
fine. It has a uint64_t nb_clusters locally and limits it:

    nb_clusters = MIN(nb_clusters, INT_MAX >> s->cluster_bits);

The problematic request that causes the crash comes actually from
qcow2_co_truncate(). It limits it only to s->l2_slice_size, which can be
larger than that, but will be at most 256k (= 2 MB / sizeof(uint64_t)).

cow_end.offset will get a wraparound then, too. This is harmless because
cow_end.nb_bytes = 0, so the offset will be ignored anyway.

I think the proper fix to be made in the 5.2 release cycle would revert
this one and instead fix the limit in qcow2_co_truncate().

But this one is good enough as a band-aid for 5.1.

Kevin