Re: [Devel] [PATCH hci-8.1 v2 3/6] dm-ploop: set delta is_raw before applying its mappings

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-ploop: zero-length raw base delta underflows nr_be - 1 in init_be_iter()
--

commit 8d9d7ed186e03b4389524be1bcdc31e23ab48455
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: set delta is_raw before applying its mappings

This patch moves the assignment of deltas[level].is_raw in
ploop_add_delta() so that it happens before
ploop_apply_delta_mappings() runs. Previously a raw delta was handled
as a non-raw one, which dereferenced a NULL d_md.

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

> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> index a7849de8aaa6..76fcfa64f875 100644
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c

[ ... ]

> @@ -501,6 +501,7 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
>  	if (level != ploop_top_level(ploop) && nr_be > deltas[level + 1].nr_be)
>  		goto out;
>  
> +	deltas[level].is_raw = is_raw;
>  	ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);

[Severity: Low]
With this change the raw branch of ploop_apply_delta_mappings() becomes
reachable, and it looks like it can misbehave for a zero-length raw base
delta.

For a raw delta, nr_be comes straight from the file size:

ploop_add_delta()
    nr_be = POS_TO_CLU(ploop, file_size);

ploop_check_delta_length() only rejects sizes that are not cluster
aligned, so a size of 0 passes. The check against deltas[level + 1].nr_be
cannot fail for 0 either.

Then, with nr_be == 0, init_be_iter() computes:

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

nr_be - 1 is 0xFFFFFFFF, and the u32 wrap in ploop_bat_clu_to_page_nr()
gives last_page = 0. Page 0 then gets start=16 and end=15 and is
skipped, while every other page iterates in full.

In ploop_apply_delta_mappings() the stop condition can never trigger:

    if (clu == nr_be - 1)
        stop = true;
    ...
    dst_clu = clu;

so can identity mappings be written at the raw level for every
unpopulated slot on pages above 0, past EOF of the raw file, including
tail slots of the last page beyond nr_bat_entries?

Should ploop_add_delta() reject nr_be == 0 for a raw delta (or should
init_be_iter() handle it)?

The BAT arrays do not appear to be overrun since the loop stays within
each md page. Reads of those clusters would hit EOF on the raw file and
likely return zeros, like a hole. I did not trace the write/COW path for
these mappings. Reaching this requires a table load with an empty
"raw@fd" base, so it needs CAP_SYS_ADMIN and is an unusual
misconfiguration. I did not find a check for this in later patches of
the series.

>  
>  	deltas[level].file = file;

[ ... ]

> @@ -521,7 +522,6 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
>  	deltas[level].file_size = file_size;
>  	deltas[level].file_preallocated_area_start = file_size;
>  	deltas[level].nr_be = nr_be;
> -	deltas[level].is_raw = is_raw;
>  	ret = 0;
>  out:
>  	ploop_free_md_pages_tree(&md_root);

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

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