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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-ploop: ploop_bat_clu_to_page_nr() still multiplies in 32 bits, wraps on 32-bit builds
--

commit 4d982aa9720b349cfdf860a1b465d9c33f3db9ff
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: fix u32 overflow of the delta BAT byte size

This patch computes the header and BAT byte size in u64 via
ploop_nr_bat_clusters() and in ploop_read_delta_metadata(). It also
rejects headers and resizes whose hb_nr exceeds PLOOP_MAX_HB_NR.

> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c

[ ... ]

> @@ -339,7 +340,7 @@ int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
>  	if (ret)
>  		goto out;
>  
> -	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.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009142316.801288-1-alexander.ivanov@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel