Re: [Devel] [PATCH hci-8.1 v4 01/11] 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 wraps size_t multiply before widening
--

commit 490ad8e2663849858d62f84d5062973178fffee9
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

This patch computes the byte size of the header and BAT in u64 and uses
ploop_nr_bat_clusters() everywhere for the BAT cluster count. It also
rejects a header or a resize whose hb_nr exceeds PLOOP_MAX_HB_NR.

https://virtuozzo.atlassian.net/browse/VSTOR-148171

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

[ ... ]

> @@ -410,9 +412,9 @@ static inline unsigned int ploop_nr_pages_in_cluster(struct ploop *ploop)
>  static inline unsigned int ploop_nr_bat_clusters(struct ploop *ploop,
>  						 u32 nr_bat_entries)
>  {
> -	unsigned long size, bat_clusters;
> +	u64 size, bat_clusters;
>  
> -	size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
> +	size = ((u64)nr_bat_entries + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
>  	bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
>  
>  	return bat_clusters;

[Severity: Medium]
Does the sibling helper ploop_bat_clu_to_page_nr() in dm-ploop.h still
wrap the same multiplication?

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;
}

The multiplication is evaluated in size_t before the result is widened to
u64. Where size_t is 32 bits, this wraps for clu >= 2^30 - PLOOP_MAP_OFFSET.
That is the same range the commit message says is being fixed ("It wraps
for m_Size >= 2^30 - PLOOP_MAP_OFFSET"), and the message also says the size
is now computed in u64 "everywhere".

The new PLOOP_MAX_HB_NR limit only caps nr_bat_entries plus bat_clusters
near 2^32. A header or a resize with nr_bat_entries >= 2^30 - 16 therefore
still passes the new checks in ploop_setup_metadata() and ploop_resize().

Can a wrapped page number then reach these callers?

ploop_resize()->ploop_prealloc_md_pages()
    new_nr_pages = ploop_bat_clu_to_page_nr(new_nr_bat_entries - 1) + 1;

This could allocate too few md pages.

init_be_iter()
    u32 rem, last_page = ploop_bat_clu_to_page_nr(nr_be - 1);

This could give a wrong iteration end for the BAT entries.

ploop_map_bat_entry()
    id = ploop_bat_clu_to_page_nr(clu);
    md = ploop_md_page_find(ploop, id);
    BUG_ON(!md);

A page id that wraps would then either miss the lookup and hit BUG_ON(!md),
or index past the pages that were allocated.

On 64-bit, size_t is 64 bits and clu + 16 cannot wrap in u32 because of the
PLOOP_MAX_HB_NR cap, so this looks specific to 32-bit builds. The DM_PLOOP
entry in drivers/md/Kconfig depends only on BLK_DEV_DM, so nothing prevents
such a build.

I have not confirmed whether other 32-bit limits make these sizes unusable
first. For example, 2^30 entries would need about 4 GiB of md pages, which
would probably fail to allocate.

Would it make sense to widen the multiplication in ploop_bat_clu_to_page_nr()
the same way, with ((u64)clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t)?

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

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