Re: [PATCH v3 05/21] md/md-llbitmap: track bitmap sync_size explicitly

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Missing upper and lower bounds validation on the newly tracked `sync_size` read from the superblock leads to arbitrary out-of-bounds memory accesses.
- [High] Validation of `chunksize` in `llbitmap_read_sb` incorrectly uses `mddev->resync_max_sectors` instead of `sync_size`.
- [High] Torn read of the 64-bit `sync_size` field leading to superblock corruption and memory allocation bugs on reboot, along with unprotected bitmap geometry updates.

Pre-existing issues:
- [High] A teardown race condition in `llbitmap_destroy()` fails to prevent a workqueue task from re-arming a timer after it has been deleted, causing a UAF.
--

--- Patch [5]: [PATCH v3 05/21] md/md-llbitmap: track bitmap sync_size explicitly ---
commit e1b87a51dfe616ddb9c0f12bcc69bdac5d5044d5
Author: Yu Kuai <[email protected]>

md/md-llbitmap: track bitmap sync_size explicitly
    
Track llbitmap's own sync_size instead of always using
mddev->resync_max_sectors directly.

This is the minimal bookkeeping needed before llbitmap can track old
and new reshape geometry independently.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index f0c20afa354eb..e3500a1a52c07 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -987,6 +991,9 @@ static int llbitmap_read_sb(struct llbitmap *llbitmap)
>  		goto out_put_page;
>  	}
>  
> +	sync_size = le64_to_cpu(sb->sync_size);

[Severity: Critical]
Is it necessary to validate the upper and lower bounds of the sync_size
read from the superblock here?

If sync_size is manipulated to be near U64_MAX, calculating nr_pages
might truncate to a small 32-bit integer, while chunks remains huge.
Could this cause llbitmap_init_state() to perform massive out-of-bounds
writes into the undersized pctl array?

Alternatively, if sync_size is too small, nr_pages is small, but incoming
IO up to mddev->resync_max_sectors calculates a page_start that exceeds
the bounds of pctl:

drivers/md/md-llbitmap.c:llbitmap_start_write() {
    ...
    int page_start = (start + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;
    ...
    while (page_start <= page_end) {
        llbitmap_raise_barrier(llbitmap, page_start);
    ...
}

Could this lead to out-of-bounds memory accesses if a maliciously crafted
bitmap superblock is mounted?


[Severity: High]
Should the chunksize validation further down in llbitmap_read_sb() be
updated to use sync_size as well?

    if (chunksize < DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors,
                                          mddev->bitmap_info.space << SECTOR_SHIFT)) {

If an array is grown and stopped before the bitmap is resized,
mddev->resync_max_sectors reflects the new larger size, but the bitmap
superblock retains the old sync_size and chunksize.

Will checking if the old chunksize can cover the new resync_max_sectors
cause a false failure and abort bitmap initialization in this scenario?

> +	if (!sync_size)
> +		sync_size = mddev->resync_max_sectors;
>  	chunksize = le32_to_cpu(sb->chunksize);
>  	if (!is_power_of_2(chunksize)) {

[ ... ]

> @@ -1153,6 +1161,7 @@ static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
>  	llbitmap->chunkshift = ffz(~chunksize);
>  	llbitmap->chunksize = chunksize;
>  	llbitmap->chunks = chunks;
> +	llbitmap->sync_size = blocks;
>  
>  	return 0;
>  }

[Severity: High]
Are these updates to the 64-bit sync_size and geometry variables
adequately protected from concurrent access?

Unlike the legacy bitmap which uses mddev->pers->quiesce() to pause IO,
llbitmap_resize() does not pause IO. This allows active IO paths to read
a mix of old and new geometry values.

Additionally, llbitmap_update_sb() concurrently reads llbitmap->sync_size
locklessly:

drivers/md/md-llbitmap.c:llbitmap_update_sb() {
    ...
    sb->sync_size = cpu_to_le64(llbitmap->sync_size);
    ...
}

On 32-bit platforms, might this race result in a torn read of the 64-bit
value, causing a corrupted value to be written to the disk superblock?

[ ... ]

This is a pre-existing issue, but does the teardown sequence in
llbitmap_destroy() allow the pending_timer to be re-armed after deletion?

In llbitmap_destroy():

    timer_delete_sync(&llbitmap->pending_timer);
    flush_workqueue(md_llbitmap_io_wq);

If the daemon work is concurrently running in the workqueue,
timer_delete_sync() returns immediately because the timer is no longer
pending.

The workqueue is then flushed, but at the end of the worker function
md_llbitmap_daemon_fn(), it unconditionally re-arms the timer:

    if (restart)
        mod_timer(&llbitmap->pending_timer,
                  jiffies + mddev->bitmap_info.daemon_sleep * HZ);

When llbitmap is subsequently freed, will the re-armed timer dereference
the freed memory when it fires?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5