Re: [PATCH v5 08/29] md/md-llbitmap: prevent create failure bitmap UAF

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

Pre-existing issues:
- [High] Early publication of `mddev->bitmap` in `llbitmap_create()` races =
with lockless deferred IO (unplug callbacks), leading to UAF or NULL pointe=
r dereference.
- [Medium] Improper cleanup sequence in `llbitmap_read_sb()`: `__free_page(=
)` is called before `kunmap_local()`.
--

--- Patch [8]: [PATCH v5 08/29] md/md-llbitmap: prevent create failure bitm=
ap UAF ---
commit f0da60fcfbc2742e91e414951699e4a59d581559
Author: Yu Kuai <[email protected]>

md/md-llbitmap: prevent create failure bitmap UAF

llbitmap_create() publishes mddev->bitmap before reading the bitmap
superblock. If llbitmap_read_sb() fails, the old cleanup dropped
bitmap_info.mutex and freed llbitmap before clearing mddev->bitmap. This
commit clears mddev->bitmap while still holding the mutex.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 948bf64c5ad24..af80a630bd21a 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1126,10 +1126,11 @@ static int llbitmap_create(struct mddev *mddev)
>  	mutex_lock(&mddev->bitmap_info.mutex);
>  	mddev->bitmap =3D llbitmap;
>  	ret =3D llbitmap_read_sb(llbitmap);

[Severity: Medium]
This is a pre-existing issue, but is the cleanup sequence in
llbitmap_read_sb() correct when returning an error?

When llbitmap_read_sb() encounters an error, it jumps to out_put_page
and calls __free_page() before kunmap_local():

drivers/md/md-llbitmap.c:llbitmap_read_sb() {
    ...
out_put_page:
    __free_page(sb_page);
    kunmap_local(sb);
    return ret;
}

Does calling __free_page() before kunmap_local() violate the kmap_local API
contract and potentially cause memory debugging warnings?

> +	if (ret)
> +		mddev->bitmap =3D NULL;
>  	mutex_unlock(&mddev->bitmap_info.mutex);

[Severity: High]
This is a pre-existing issue, but does publishing and clearing mddev->bitmap
here still leave a race with lockless deferred IO?

If mddev->bitmap is set to NULL on failure, but mddev->bitmap_ops is still
active, a concurrent unplug callback executing locklessly might trigger a
NULL pointer dereference.

For example, if a user task unplugs and calls raid1_prepare_flush_writes(),
it can execute mddev->bitmap_ops->unplug() locklessly. llbitmap_unplug()
unconditionally reads mddev->bitmap and passes it to llbitmap_dirty():

drivers/md/md-llbitmap.c:llbitmap_unplug() {
    struct llbitmap *llbitmap =3D mddev->bitmap;
    if (!llbitmap_dirty(llbitmap))
        return;
    ...
}

Can llbitmap_unplug() crash if it observes the NULL bitmap?

Also, if llbitmap_unplug() runs during llbitmap_cache_pages(), can it
observe llbitmap->nr_pages being set before the llbitmap->pctl[i]
pointers are initialized, leading to a NULL dereference in
llbitmap_dirty() when it accesses llbitmap->pctl[i]->flags?

drivers/md/md-llbitmap.c:llbitmap_cache_pages() {
    ...
    llbitmap->nr_pages =3D nr_pages;
    for (i =3D 0; i < nr_pages; i++, pctl =3D (void *)pctl + size) {
        struct page *page =3D llbitmap_read_page(llbitmap, i);
        llbitmap->pctl[i] =3D pctl;
        ...
}

>  	if (ret) {
>  		kfree(llbitmap);
> -		mddev->bitmap =3D NULL;
>  	}
> =20
>  	return ret;

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802195038.1642=
[email protected]?part=3D8