[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_c

From: Alberto Garcia
Subject: Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()
Date: Wed, 05 Aug 2020 16:32:48 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]