Re: [Devel] [PATCH hci-8.1 v5 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:
- [Low] dm-ploop: ploop_bat_clu_to_page_nr() still wraps in size_t on 32-bit
--

commit 101e85108bc98e48077f0a87fa099eedec6fc95d
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

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

Link: 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
> @@ -12,6 +12,8 @@
>  #define PLOOP_MAP_OFFSET 16
> +/* Keeps DIV_ROUND_UP(hb_nr, 8) and clu + PLOOP_MAP_OFFSET within u32 */
> +#define PLOOP_MAX_HB_NR round_down(U32_MAX - PLOOP_MAP_OFFSET, 8)
>  typedef u32 map_index_t;

[ ... ]

> @@ -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: Low]
The commit message says the u32 overflow is fixed "everywhere", but the
sibling helper ploop_bat_clu_to_page_nr() in this same header still has
the same pattern:

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 the multiplication is evaluated at size_t width, and only the
result is widened to u64.  On a 32-bit build this wraps for
clu >= 2^30 - PLOOP_MAP_OFFSET, which is the same range the commit
message describes.

DM_PLOOP in drivers/md/Kconfig depends only on BLK_DEV_DM, with no 64BIT
restriction, so a 32-bit build appears possible.  PLOOP_MAX_HB_NR is
about 2^32 - 24, so ploop_setup_metadata() and ploop_resize() would still
accept a BAT in that range.

Can ploop_prealloc_md_pages() then under-allocate md pages on 32-bit?

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

The same wrapped value would also feed init_be_iter(),
ploop_bat_entries() and the other callers of this helper.

This is probably not reachable in practice, since a BAT of 2^30 entries
needs more than 4 GiB of md pages.  Would it make sense to cast clu to
u64 before the multiplication, as is done in ploop_nr_bat_clusters(), so
the helpers stay consistent with the commit message?

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

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