[Devel] [PATCH hci-8.1] dm-ploop: fix u32 overflow of the delta BAT byte size

[Alexander Ivanov]

>> -	size = (PLOOP_MAP_OFFSET + delta_nr_be) * sizeof(map_index_t);
>> +	size = ((u64)delta_nr_be + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
>>   	size = ALIGN(size, PAGE_SIZE); /* file may be open as direct */
>>   	nr_segs = size / PAGE_SIZE;
>>   
> [Severity: Medium]
> Is the u64 conversion complete? ploop_bat_clu_to_page_nr() in
> dm-ploop.h still has the same wrap this patch fixes elsewhere:
>
> static inline u32 ploop_bat_clu_to_page_nr(u32 clu)
> {
> 	u64 byte;
>
> 	byte = (clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
> 	return byte >> PAGE_SHIFT;
> }
>
> Here clu is u32 and PLOOP_MAP_OFFSET is int, so (clu + 16) is a 32-bit
> value. It is multiplied by sizeof(map_index_t), a size_t. On a 32-bit
> build size_t is 32 bits, so the product wraps for clu >= 2^30 - 16
> before it is widened to u64 for byte. On 64-bit this does not wrap.
>
> The sizes this patch now accepts reach that range. ploop_setup_metadata()
> and ploop_resize() only reject sizes above PLOOP_MAX_HB_NR, which is
> about 2^32 - 32. The commit message also describes m_Size >= 2^30 -
> PLOOP_MAP_OFFSET as legal.
>
> For such a size, the page count used by ploop_prealloc_md_pages() would
> come from the wrapped helper:
>
> ploop_prealloc_md_pages()
>      new_nr_pages = ploop_bat_clu_to_page_nr(new_nr_bat_entries - 1) + 1;
>
> nr_segs in ploop_read_delta_metadata() is now computed from the u64
> size. Could nr_segs then exceed the number of md pages that were
> preallocated?
>
> If so, the rb_root_for_each_md_page() loop would fill only part of the
> bvec array. The remaining bvecs, allocated with kvmalloc(), would be
> passed uninitialised to vfs_iter_read().
>
> init_be_iter() also takes last_page from the same helper:
>
> init_be_iter()
>      u32 rem, last_page = ploop_bat_clu_to_page_nr(nr_be - 1);
>
> Could that give a wrong end index for large nr_be? Callers such as
> ploop_md_page_find() users could also fail to find a page, and the
> BUG_ON(!md) in the lookup path at the start of ploop_bat_clu_to_page_nr()
> callers would then be reachable.
>
> Would it make sense to widen the operand in ploop_bat_clu_to_page_nr()
> the same way, for example ((u64)clu + PLOOP_MAP_OFFSET) *
> sizeof(map_index_t)? Alternatively, 32-bit builds could be limited to
> sizes the helper can represent.
>
> The delta header comes from an image file and resize is command-driven,
> so the large m_Size can be supplied externally.
On 64-bit (clu + PLOOP_MAP_OFFSET) cannot wrap, since clu <
PLOOP_MAX_HB_NR <= U32_MAX - PLOOP_MAP_OFFSET, and the multiplication
by sizeof(map_index_t) is done in 64 bits. That is what
PLOOP_MAX_HB_NR is there for.

This kernel is not built for 32-bit targets.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/devel/attachments/20261010/e1c206b5/attachment-0001.html>