On Wed 05 Aug 2020 04:16:57 PM CEST, Kevin Wolf wrote: >> 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);
INT_MAX replaced with BDRV_REQUEST_MAX_BYTES in my subcluster allocation series, so it should still be fine. > 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. In that one nb_clusters is actually int64_t so there's no wraparound. > 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. The other one is just as simple, no? This line in the while() loop in qcow2_co_truncate(): nb_clusters = MIN(nb_clusters, BDRV_REQUEST_MAX_BYTES >> s->cluster_bits); Berto
