Re: [PATCH] gfs2: Fix slab-use-after-free in gfs2_qd_dispose

Re: [PATCH] gfs2: Fix slab-use-after-free in gfs2_qd_dispose

[syzbot]

> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git

This crash does not have a reproducer. I cannot test it.

> gfs2-qd-put
>

[PATCH] gfs2: Fix slab-use-after-free in gfs2_qd_dispose

[Deepanshu Kartikey]

In gfs2_qd_dispose(), the quota data (qd) is removed from qd_list and
qd_hlist, but not removed from the LRU list before call_rcu() is
invoked to free the object.

This can lead to a use-after-free when the shrinker (gfs2_qd_shrink_scan)
runs concurrently with filesystem unmount. The shrinker attempts to walk
the LRU list via list_lru_shrink_walk(), accessing quota data that has
already been freed by the RCU callback (gfs2_qd_dealloc).

Fix this by calling list_lru_del_obj() to remove the quota data from
the LRU list before scheduling the RCU callback.

Fixes: faada74a90563 ("gfs2: Factor out duplicate quota data disposal code")
Reported-by: syzbot+046b605f01802054bff0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=046b605f01802054bff0
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
Note: Only compile-tested. No reproducer available from syzbot.
---
 fs/gfs2/quota.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index b1692f12a602..85b0b5105b4b 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -125,6 +125,8 @@ static void gfs2_qd_dispose(struct gfs2_quota_data *qd)
 	hlist_bl_del_rcu(&qd->qd_hlist);
 	spin_unlock_bucket(qd->qd_hash);
 
+	list_lru_del_obj(&gfs2_qd_lru, &qd->qd_lru);
+
 	if (!gfs2_withdrawn(sdp)) {
 		gfs2_assert_warn(sdp, !qd->qd_change);
 		gfs2_assert_warn(sdp, !qd->qd_slot_ref);
-- 
2.43.0

Re: [PATCH] gfs2: Fix slab-use-after-free in gfs2_qd_dispose

[Andreas Gruenbacher]

Hello,

> In gfs2_qd_dispose(), the quota data (qd) is removed from qd_list and
> qd_hlist, but not removed from the LRU list before call_rcu() is
> invoked to free the object.
> 
> This can lead to a use-after-free when the shrinker (gfs2_qd_shrink_scan)
> runs concurrently with filesystem unmount. The shrinker attempts to walk
> the LRU list via list_lru_shrink_walk(), accessing quota data that has
> already been freed by the RCU callback (gfs2_qd_dealloc).
> 
> Fix this by calling list_lru_del_obj() to remove the quota data from
> the LRU list before scheduling the RCU callback.
> 
> Fixes: faada74a90563 ("gfs2: Factor out duplicate quota data disposal code")
> Reported-by: syzbot+046b605f01802054bff0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=046b605f01802054bff0
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
> Note: Only compile-tested. No reproducer available from syzbot.
> ---
>  fs/gfs2/quota.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index b1692f12a602..85b0b5105b4b 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -125,6 +125,8 @@ static void gfs2_qd_dispose(struct gfs2_quota_data *qd)
>       hlist_bl_del_rcu(&qd->qd_hlist);
>       spin_unlock_bucket(qd->qd_hash);
>  
> +     list_lru_del_obj(&gfs2_qd_lru, &qd->qd_lru);
> +

thanks for the suggestion, but this will call list_lru_del_obj() a
second time on the gfs2_qd_shrink_scan() -> gfs2_qd_list_dispose() ->
gfs2_qd_dispose() code path.  The bug also isn't introduced by the
commit you've identified.

The below fix should be better.

Andreas

--

Subject: gfs2: Fix slab-use-after-free in qd_put

Commit a475c5dd16e5 ("gfs2: Free quota data objects synchronously")
started freeing quota data objects during filesystem shutdown instead of
putting them back onto the LRU list, but it failed to remove these
objects from the LRU list, causing LRU list corruption.  This caused
use-after-free when the shrinker (gfs2_qd_shrink_scan) tried to access
already-freed objects on the LRU list.

Fix this by removing qd objects from the LRU list before freeing them in
qd_put().

Initial fix from Deepanshu Kartikey <kartikey406@gmail.com>.

Fixes: a475c5dd16e5 ("gfs2: Free quota data objects synchronously")
Reported-by: syzbot+046b605f01802054bff0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=046b605f01802054bff0
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/quota.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 21dfe1e48da6..1c3455093ae8 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -334,6 +334,7 @@ static void qd_put(struct gfs2_quota_data *qd)
 		lockref_mark_dead(&qd->qd_lockref);
 		spin_unlock(&qd->qd_lockref.lock);
 
+		list_lru_del_obj(&gfs2_qd_lru, &qd->qd_lru);
 		gfs2_qd_dispose(qd);
 		return;
 	}
-- 
2.51.0