Re: [PATCH 2/4] md/md-llbitmap: mask sb->state on load to drop runtime-only bits

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

Pre-existing issues:
- [High] Teardown symmetry bug: `llbitmap_destroy()` deletes the daemon timer and waits for the daemon work, but the work can re-arm the timer, leading to a use-after-free when the timer fires on the freed `llbitmap` object.
- [High] Preexisting UAF in llbitmap_create() error path due to clearing mddev->bitmap outside the mutex after kfree.
- [High] Preexisting race condition in llbitmap_write_page() iterating mddev->disks without required locks.
- [High] A maliciously crafted `sectors_reserved` value in the llbitmap superblock can cause an integer overflow during a bitwise shift on 32-bit systems, resulting in a zero divisor and a kernel panic.
--

--- Patch [2]: [PATCH 2/4] md/md-llbitmap: mask sb->state on load to drop runtime-only bits ---

commit 48600506c718f0300f8ce2473c95dff7a9e47cc2
Author: Mykola Marzhan [off-list ref]

md/md-llbitmap: mask sb->state on load to drop runtime-only bits

This commit masks the loaded superblock state in llbitmap_read_sb() to only
keep STALE, FIRST_USE, and CLEAN bits. It prevents runtime-only flags like
WRITE_ERROR from being loaded from disk and permanently disabling the bitmap.

Cc: stable@vger.kernel.org # v6.18+

[Severity: High]
This isn't a bug introduced by this patch, but can this sectors_reserved
check lead to an integer overflow?

In llbitmap_read_sb(), we read the untrusted value:

    int room = le32_to_cpu(sb->sectors_reserved);
    if (room)
        mddev->bitmap_info.space = room;

Later in the same function:

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

Could a crafted superblock with sectors_reserved = 0x00800000 cause
(mddev->bitmap_info.space << SECTOR_SHIFT) to wrap to 0 on 32-bit systems,
resulting in a division by zero panic?

-	llbitmap->flags = le32_to_cpu(sb->state);
+	/*
+	 * Mask the loaded state to the bits this loader consumes.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the teardown path have a symmetry
issue?

In llbitmap_destroy():

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

If md_llbitmap_daemon_fn() is actively running and finds unexpired pages:

    if (restart)
        mod_timer(&llbitmap->pending_timer, ...);

Does this allow the daemon to blindly re-arm the timer after
timer_delete_sync() has finished, leading to a use-after-free when the timer
fires on the freed llbitmap object?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free in the
llbitmap_create() error path?

When llbitmap_create() fails, it does this:

    mutex_unlock(&mddev->bitmap_info.mutex);
    if (ret) {
        kfree(llbitmap);
        mddev->bitmap = NULL;
    }

Can a concurrent reader like metadata_show() acquire the lock and dereference
the freed object?

metadata_show() does:

    mutex_lock(&mddev->bitmap_info.mutex);
    llbitmap = mddev->bitmap;
    if (!llbitmap) {
    ...

[Severity: High]
This isn't a bug introduced by this patch, but does llbitmap_write_page()
iterate the disks without required locks?

In llbitmap_write_page(), executed from workqueues:

        rdev_for_each(rdev, mddev) {
            sector_t sector;

This iterates a dynamic, RCU-protected linked list without taking
rcu_read_lock() or reconfig_mutex.

If a disk is concurrently hot-removed, can list_del_rcu() and
synchronize_rcu() free the element while llbitmap_write_page() is traversing
it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719144409.940492-1-mykola@meshstor.io?part=2