Re: [Devel] [PATCH hci-8.1 v6 01/12] 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 u32 on 32-bit builds
--

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

commit 637ffe4e923149b9a169a498b933c3d5f2b39ef7
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.

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 @@
>  #include <linux/bio.h>
>  
>  #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;
>  #define BAT_ENTRIES_PER_PAGE (PAGE_SIZE / sizeof(map_index_t))

[ ... ]

>  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]
This isn't addressed by the hunk above, but a related helper in the same
header appears to have been left with the same u32 wrap:

drivers/md/dm-ploop.h:ploop_bat_clu_to_page_nr() {
	u64 byte;

	byte = (clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
	return byte >> PAGE_SHIFT;
}

Here clu is u32 and sizeof(map_index_t) is size_t, which is 32 bits on
32-bit architectures.  Doesn't the multiply wrap before the result is
widened to the u64 byte?

DM_PLOOP depends only on BLK_DEV_DM, so 32-bit builds are not excluded.
With PLOOP_MAX_HB_NR now allowing hb_nr up to 0xFFFFFFE8, a delta header
m_Size or a resize request with clu + 16 >= 2^30 would make this helper
return a too-small page number on a 32-bit kernel.

That value feeds ploop_prealloc_md_pages():

    new_nr_pages = ploop_bat_clu_to_page_nr(new_nr_bat_entries - 1) + 1;

which is called from ploop_resize() and ploop_read_delta_metadata(), so
too few md pages could be allocated for a large BAT.  init_be_iter() uses
the same helper to compute last_page, so the BAT page coverage could
become inconsistent with nr_bat_entries.

The commit message says the byte size is computed in u64 "in several
places" and that the u32 wrap is fixed everywhere.  Should this helper
use ((u64)clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t) as well?

On 64-bit builds the multiply does not wrap, so this only affects 32-bit
kernels.

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

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