Re: [PATCH 3/4] md/md-llbitmap: unmap sb page before freeing it

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

Pre-existing issues:
- [High] Integer Truncation of 64-bit event counter causes spurious array recoveries on 32-bit architectures
- [High] Use-After-Free in teardown sequence due to re-arming timer
- [High] Lost Write to `sb->sync_size` during superblock load
- [High] Integer overflow causing division by zero when calculating minimum chunksize
- [High] Integer overflow in page count calculation leading to out-of-bounds heap access
--

--- Patch [3]: [PATCH 3/4] md/md-llbitmap: unmap sb page before freeing it ---
commit 6be442dfef7ad93b44cadb093b6d078f152b2522
Author: Mykola Marzhan <[email protected]>

md/md-llbitmap: unmap sb page before freeing it

The patch swaps __free_page(sb_page) and kunmap_local(sb) in
llbitmap_read_sb() to avoid unmapping a page after it has been freed.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 2d3de38f9b784..2d6c7ad0923a1 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1035,8 +1035,8 @@ static int llbitmap_read_sb(struct llbitmap *llbitmap)
>  	ret = llbitmap_cache_pages(llbitmap);

[Severity: High]
This is a pre-existing issue, but is there an integer overflow when
calculating nr_pages on 32-bit architectures inside llbitmap_cache_pages()?

drivers/md/md-llbitmap.c:llbitmap_cache_pages() {
    ...
    unsigned int nr_pages = DIV_ROUND_UP(llbitmap->chunks +
                                         BITMAP_DATA_OFFSET, PAGE_SIZE);
    ...
    llbitmap->pctl = kmalloc_array(nr_pages, sizeof(void *),
                                   GFP_KERNEL | __GFP_ZERO);
    ...
}

If a malicious superblock sets a high resync_max_sectors and chunksize of 2,
could llbitmap->chunks + BITMAP_DATA_OFFSET overflow to 0? Would this result
in a zero-sized array allocation and subsequent out-of-bounds access?

>  
>  out_put_page:
> -	__free_page(sb_page);
>  	kunmap_local(sb);
> +	__free_page(sb_page);

[Severity: High]
This is a pre-existing issue, but is the update to sb->sync_size lost here?

Earlier in llbitmap_read_sb(), sb->sync_size is updated:

    sb->sync_size = cpu_to_le64(mddev->resync_max_sectors);

Since sb_page is unmapped and unconditionally freed here without being
written back to disk or retained in memory, does the runtime state lose
this update?

>  	return ret;
>  }

There are a few other pre-existing issues in this file that might need
attention.

[Severity: High]
This is a pre-existing issue, but does assigning the 64-bit sb->events to the
unsigned long events variable truncate it on 32-bit architectures?

drivers/md/md-llbitmap.c:llbitmap_read_sb() {
    unsigned long events;
    ...
    events = le64_to_cpu(sb->events);
    if (events < mddev->events) {
        ...
    }
}

When the 64-bit counter exceeds 4 billion, would this truncation cause a
spurious full array resync by incorrectly making the bitmap appear stale?


[Severity: High]
This is a pre-existing issue, but can the timer fire on freed memory during
teardown?

drivers/md/md-llbitmap.c:llbitmap_destroy() {
    ...
    timer_delete_sync(&llbitmap->pending_timer);
    flush_workqueue(md_llbitmap_io_wq);
    ...
}

drivers/md/md-llbitmap.c:md_llbitmap_daemon_fn() {
    ...
    if (restart)
        mod_timer(&llbitmap->pending_timer, ...);
}

If llbitmap_destroy() deletes the timer and flushes the workqueue, could
md_llbitmap_daemon_fn() unconditionally re-arm the timer at the end of its
execution, leading to a use-after-free when llbitmap is subsequently freed?


[Severity: High]
This is a pre-existing issue, but can this overflow and cause a division by
zero panic?

drivers/md/md-llbitmap.c:llbitmap_read_sb() {
    ...
    if (chunksize < DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors,
                                          mddev->bitmap_info.space << SECTOR_SHIFT)) {
    ...
}

On 32-bit systems, mddev->bitmap_info.space is a 32-bit unsigned long. If an
attacker-controlled superblock sets a value where space << 9 overflows to 0,
would DIV_ROUND_UP_SECTOR_T divide by zero?

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