linux-kernel - Re: [BUG] possible race between md_free_disk and md_notify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7iZh56mykLW82SN@bender.morinfr.org>
Date: Fri, 21 Feb 2025 16:19:35 +0100
From: Guillaume Morin <guillaume@...infr.org>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: Guillaume Morin <guillaume@...infr.org>, linux-raid@...r.kernel.org,
	linux-kernel@...r.kernel.org, song@...nel.org,
	"yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [BUG] possible race between md_free_disk and md_notify_reboot

On 21 Feb  9:27, Yu Kuai wrote:
> > The issue with the next pointer seems to be fixed with your change.
> > Though I am still unclear how the 2nd potential issue I mentioned -
> > where the current item would be freed concurrently by mddev_free() - is
> > prevented. I am not finding anything in the code that seems to prevent a
> > concurrent call to mddev_free() for the current item in the
> > list_for_each_entry() loop (and therefore accessing mddev after the
> > kfree()).
> > 
> > I understand that we are getting a reference through the active atomic
> > in mddev_get() under the lock in md_notify_reboot() but how is that
> > preventing mddev_free() from freeing the mddev as soon as we release the
> > all_mddevs_lock in the loop?
> > 
> > I am not not familiar with this code so I am most likely missing
> > osmething but if you had the time to explain, that would be very
> > helpful.
> 
> I'm not quite sure what you're confused. mddev lifetime are both
> protected by lock and reference.
> 
> In this case:
> 
> hold lock
> get first mddev
> release lock
> // handle first mddev
> 
> hold lock
> release mddev
> get next mddev
> release lock
> -> mddev can be freed now
> // handle the next mddev
> ...
> 

In my original message, I mentioned 2 potential issues
Let's say md_notify_reboot() is handling mddev N from the all_mddevs
list.

1) The GPF we pasted in the original message happened when mddev_free()
is called concurrently for mddev N+1. This lead to the GPF since the cpu
would try to load the list poisoned values from the 'n' pointer.

Your patch definitely fixes this race and we cannot reproduce the GPF
anymore.

2) Instead of mddev_free() being called for mddev N+1 like in 1, I wonder
what's preventing mddev_free() being called for mddev N (the one we're
iterating about). Something like

CPU1							CPU2
list_for_each_entry(mddev, &all_mddevs, all_mddevs) {
if (!mddev_get(mddev))
    continue;
spin_unlock(&all_mddevs_lock);
						        mddev_free(mddev) (same mddev as CPU1)

mddev_free() does not check the active atomic, or acquire the
reconfig_mutex/md_lock and will kfree() mddev. So the loop execution
on CPU1 after spin_unlock() could be a UAF.

So I was wondering if you could clarify what is preventing race 2?
i.e what is preventing mddev_free(mddev) from being calling kfree(mddev)
while the md_notify_reboot() loop is handling mddev.

-- 
Guillaume Morin <guillaume@...infr.org>