[PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Benjamin Moody reported to Debian that XFS partially wedges when a chgrp
fails on account of being out of disk quota.  I ran his reproducer
script:

# adduser dummy
# adduser dummy plugdev

# dd if=/dev/zero bs=1M count=100 of=test.img
# mkfs.xfs test.img
# mount -t xfs -o gquota test.img /mnt
# mkdir -p /mnt/dummy
# chown -c dummy /mnt/dummy
# xfs_quota -xc 'limit -g bsoft=100k bhard=100k plugdev' /mnt

(and then as user dummy)

$ dd if=/dev/urandom bs=1M count=50 of=/mnt/dummy/foo
$ chgrp plugdev /mnt/dummy/foo

and saw:

================================================
WARNING: lock held when returning to user space!
5.3.0-rc5 #rc5 Tainted: G        W
------------------------------------------------
chgrp/47006 is leaving the kernel with locks still held!
1 lock held by chgrp/47006:
 #0: 000000006664ea2d (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0xd2/0x290 [xfs]

...which is clearly caused by xfs_setattr_nonsize failing to unlock the
ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
unlock.

Reported-by: benjamin.moody@gmail.com
Fixes: 253f4911f297 ("xfs: better xfs_trans_alloc interface")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_iops.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index dd4076ae228a..ea614b4ae052 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -804,6 +804,7 @@ xfs_setattr_nonsize(
 
 out_cancel:
 	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out_dqrele:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);

[PATCH] generic: test for failure to unlock inode after chgrp fails with EDQUOT

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

This is a regression test that checks for xfs drivers that fail to
unlock the inode after changing the group id fails with EDQUOT.  It
pairs with "xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails
due to EDQUOT".

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/generic/719     |   56 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/719.out |    2 ++
 tests/generic/group   |    1 +
 3 files changed, 59 insertions(+)
 create mode 100755 tests/generic/719
 create mode 100644 tests/generic/719.out

diff --git a/tests/generic/719 b/tests/generic/719
new file mode 100755
index 00000000..2771a1f3
--- /dev/null
+++ b/tests/generic/719
@@ -0,0 +1,56 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0-or-newer
+# Copyright (c) 2019, Oracle and/or its affiliates.  All Rights Reserved.
+#
+# FS QA Test No. 719
+#
+# Regression test for chgrp returning to userspace with ILOCK held after a
+# hard quota error.  This causes the filesystem to hang, so it is (for now)
+# a dangerous test.
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1    # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/quota
+. ./common/filter
+
+# real QA test starts here
+_supported_os Linux
+_supported_fs generic
+_require_scratch
+_require_quota
+_require_user
+
+rm -f $seqres.full
+
+_qmount_option "grpquota"
+_scratch_mkfs > $seqres.full
+_qmount
+
+dir="$SCRATCH_MNT/dummy"
+mkdir -p $dir
+chown $qa_user $dir
+$XFS_QUOTA_PROG -x -f -c "limit -g bsoft=100k bhard=100k $qa_user" $SCRATCH_MNT
+
+$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 1m' $dir/foo >> $seqres.full
+chown $qa_user "${dir}/foo"
+su $qa_user -c "chgrp $qa_user ${dir}/foo" 2>&1 | _filter_scratch
+ls -la ${dir} >> $seqres.full
+$XFS_QUOTA_PROG -x -f -c 'report -hag' $SCRATCH_MNT >> $seqres.full
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/719.out b/tests/generic/719.out
new file mode 100644
index 00000000..8f9d51b5
--- /dev/null
+++ b/tests/generic/719.out
@@ -0,0 +1,2 @@
+QA output created by 719
+chgrp: changing group of 'SCRATCH_MNT/dummy/foo': Disk quota exceeded
diff --git a/tests/generic/group b/tests/generic/group
index e998d1d5..bb93bccc 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -572,3 +572,4 @@
 716 dangerous_norepair
 717 auto quick rw swap
 718 auto quick rw swap
+719 dangerous

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Dave Chinner]

On Thu, Aug 22, 2019 at 08:55:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Benjamin Moody reported to Debian that XFS partially wedges when a chgrp
> fails on account of being out of disk quota.  I ran his reproducer
> script:
> 
> # adduser dummy
> # adduser dummy plugdev
> 
> # dd if=/dev/zero bs=1M count=100 of=test.img
> # mkfs.xfs test.img
> # mount -t xfs -o gquota test.img /mnt
> # mkdir -p /mnt/dummy
> # chown -c dummy /mnt/dummy
> # xfs_quota -xc 'limit -g bsoft=100k bhard=100k plugdev' /mnt
> 
> (and then as user dummy)
> 
> $ dd if=/dev/urandom bs=1M count=50 of=/mnt/dummy/foo
> $ chgrp plugdev /mnt/dummy/foo
> 
> and saw:
> 
> ================================================
> WARNING: lock held when returning to user space!
> 5.3.0-rc5 #rc5 Tainted: G        W
> ------------------------------------------------
> chgrp/47006 is leaving the kernel with locks still held!
> 1 lock held by chgrp/47006:
>  #0: 000000006664ea2d (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0xd2/0x290 [xfs]
> 
> ...which is clearly caused by xfs_setattr_nonsize failing to unlock the
> ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
> unlock.
> 
> Reported-by: benjamin.moody@gmail.com
> Fixes: 253f4911f297 ("xfs: better xfs_trans_alloc interface")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_iops.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index dd4076ae228a..ea614b4ae052 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -804,6 +804,7 @@ xfs_setattr_nonsize(
>  
>  out_cancel:
>  	xfs_trans_cancel(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out_dqrele:
>  	xfs_qm_dqrele(udqp);
>  	xfs_qm_dqrele(gdqp);

/me goes back an looks at 253f4911f297 ("xfs: better xfs_trans_alloc
interface")

Fmeh. The original patch posting did:

out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-out_trans_cancel:
-	xfs_trans_cancel(tp);
+out_dqrele:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);
 	return error;

Which leaked the transaction. Looks like I screwed up fixing that
up on commit - it no longer leaked the transaction, but leaked the
lock instead. And 3 and half years later someone notices it...

Oh, gawd that code is so grotty! I started saying "maybe we
should..." and then stopped when I realised just how much cleanup
needs to be done to that function...

The above patch fixes the issue, iso consider it:

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Linus Torvalds]

On Thu, Aug 22, 2019 at 8:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> ...which is clearly caused by xfs_setattr_nonsize failing to unlock the
> ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
> unlock.

Thanks for the quick fix.

I assume there's no real embargo on this, and we can just add it asap
to the xfs tree and mark it for stable, rather than do anything
outside the usual development path?

The security list rules do allow for a 5-working-day delay, but this
being "just" a local DoS thing I expect nobody really will argue for
it.

Anybody?

                Linus

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Benjamin Moody]

On 8/23/19, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, Aug 22, 2019 at 8:55 PM Darrick J. Wong <darrick.wong@oracle.com>
> wrote:
>>
>> ...which is clearly caused by xfs_setattr_nonsize failing to unlock the
>> ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
>> unlock.
>
> Thanks for the quick fix.
>
> I assume there's no real embargo on this, and we can just add it asap
> to the xfs tree and mark it for stable, rather than do anything
> outside the usual development path?

As the person who reported the issue, I don't see a problem with
that.  I reported it to the Debian security team just to be on
the safe side, in case the problem was something bigger than what
I was seeing.

I haven't yet tested the patch, but thank you, Darrick, for the
quick response!

Benjamin

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Salvatore Bonaccorso]

Hi Darrick,

On Thu, Aug 22, 2019 at 08:55:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Benjamin Moody reported to Debian that XFS partially wedges when a chgrp
> fails on account of being out of disk quota.  I ran his reproducer
> script:
> 
> # adduser dummy
> # adduser dummy plugdev
> 
> # dd if=/dev/zero bs=1M count=100 of=test.img
> # mkfs.xfs test.img
> # mount -t xfs -o gquota test.img /mnt
> # mkdir -p /mnt/dummy
> # chown -c dummy /mnt/dummy
> # xfs_quota -xc 'limit -g bsoft=100k bhard=100k plugdev' /mnt
> 
> (and then as user dummy)
> 
> $ dd if=/dev/urandom bs=1M count=50 of=/mnt/dummy/foo
> $ chgrp plugdev /mnt/dummy/foo
> 
> and saw:
> 
> ================================================
> WARNING: lock held when returning to user space!
> 5.3.0-rc5 #rc5 Tainted: G        W
> ------------------------------------------------
> chgrp/47006 is leaving the kernel with locks still held!
> 1 lock held by chgrp/47006:
>  #0: 000000006664ea2d (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0xd2/0x290 [xfs]
> 
> ...which is clearly caused by xfs_setattr_nonsize failing to unlock the
> ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
> unlock.
> 
> Reported-by: benjamin.moody@gmail.com
> Fixes: 253f4911f297 ("xfs: better xfs_trans_alloc interface")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_iops.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index dd4076ae228a..ea614b4ae052 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -804,6 +804,7 @@ xfs_setattr_nonsize(
>  
>  out_cancel:
>  	xfs_trans_cancel(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out_dqrele:
>  	xfs_qm_dqrele(udqp);
>  	xfs_qm_dqrele(gdqp);

Confirmed the fix work.

Feel free to add a Tested-by if wanted.

Can this be backported to the relevant stable versions as well?

Regards,
Salvatore

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Salvatore Bonaccorso]

Hi Linus,

On Fri, Aug 23, 2019 at 09:28:42AM -0700, Linus Torvalds wrote:
> On Thu, Aug 22, 2019 at 8:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > ...which is clearly caused by xfs_setattr_nonsize failing to unlock the
> > ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
> > unlock.
> 
> Thanks for the quick fix.
> 
> I assume there's no real embargo on this, and we can just add it asap
> to the xfs tree and mark it for stable, rather than do anything
> outside the usual development path?
> 
> The security list rules do allow for a 5-working-day delay, but this
> being "just" a local DoS thing I expect nobody really will argue for
> it.
> 
> Anybody?

Agreed, there is no real embargo on this, it is public anyway on the
xfs list and actually we just wanted to be on the safe side by asking
or bringing it to security@k.o.

Thanks for the quick fix.

Regards,
Salvatore

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Salvatore Bonaccorso]

Hi,

On Fri, Aug 23, 2019 at 09:26:12PM +0200, Salvatore Bonaccorso wrote:
> Hi Linus,
> 
> On Fri, Aug 23, 2019 at 09:28:42AM -0700, Linus Torvalds wrote:
> > On Thu, Aug 22, 2019 at 8:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > ...which is clearly caused by xfs_setattr_nonsize failing to unlock the
> > > ILOCK after the xfs_qm_vop_chown_reserve call fails.  Add the missing
> > > unlock.
> > 
> > Thanks for the quick fix.
> > 
> > I assume there's no real embargo on this, and we can just add it asap
> > to the xfs tree and mark it for stable, rather than do anything
> > outside the usual development path?
> > 
> > The security list rules do allow for a 5-working-day delay, but this
> > being "just" a local DoS thing I expect nobody really will argue for
> > it.
> > 
> > Anybody?
> 
> Agreed, there is no real embargo on this, it is public anyway on the
> xfs list and actually we just wanted to be on the safe side by asking
> or bringing it to security@k.o.
> 
> Thanks for the quick fix.

Not changing my mind on the above, given the fixing commit is public.

But thinking a bit more on it, I guess this should not considered
local DoS only. If the filesystem is exosed then a remote user might
trigger it serverside.

The issue could be reproduced on a NFS exported XFS as well. Just
export the filesystem rw to another host via NFS and mount it there. A
local user on the client could then trigger the issue on the server as
well.

Regards,
Salvatore

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Linus Torvalds]

On Fri, Aug 23, 2019 at 12:24 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
>
> Confirmed the fix work.
>
> Feel free to add a Tested-by if wanted.
>
> Can this be backported to the relevant stable versions as well?

It's out there in my tree now. It's not explicitly marked for stable
per se, but it does have the "Fixes:" tag which should mean that Greg
and Sasha will pick it up automatically.

But just to make it explicit, since Greg is on the security list,
Darrick's fix is commit 1fb254aa983b ("xfs: fix missing ILOCK unlock
when xfs_setattr_nonsize fails due to EDQUOT").

                Linus

Re: [PATCH] generic: test for failure to unlock inode after chgrp fails with EDQUOT

[Christoph Hellwig]

> diff --git a/tests/generic/group b/tests/generic/group
> index e998d1d5..bb93bccc 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -572,3 +572,4 @@
>  716 dangerous_norepair
>  717 auto quick rw swap
>  718 auto quick rw swap
> +719 dangerous

+ quota + metadata?  

Otherwise this looks fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

and after a while we should add it to auto.  I'm not even sure if we
shouldn't do that ASAP.  It's not that dangerous and we should test
for this everywhere..

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Greg KH]

On Sat, Aug 24, 2019 at 11:44:44AM -0700, Linus Torvalds wrote:
> On Fri, Aug 23, 2019 at 12:24 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
> >
> > Confirmed the fix work.
> >
> > Feel free to add a Tested-by if wanted.
> >
> > Can this be backported to the relevant stable versions as well?
> 
> It's out there in my tree now. It's not explicitly marked for stable
> per se, but it does have the "Fixes:" tag which should mean that Greg
> and Sasha will pick it up automatically.
> 
> But just to make it explicit, since Greg is on the security list,
> Darrick's fix is commit 1fb254aa983b ("xfs: fix missing ILOCK unlock
> when xfs_setattr_nonsize fails due to EDQUOT").

Thanks, I'll pick this up.

Note, "Fixes:" will never guarantee that a patch ends up in a stable
release.  Sasha's scripts usually do a good job of catching a lot of
them a week or so after-the-fact, but they are not guaranteed to do so.
A CC: stable@ will always be caught by me, and I try to ensure that
anything goes across the security@k.o list also gets picked up.

thanks,

greg k-h

Re: [PATCH] xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

[Salvatore Bonaccorso]

Hi Greg,

On Sun, Aug 25, 2019 at 05:13:18AM +0200, Greg KH wrote:
> On Sat, Aug 24, 2019 at 11:44:44AM -0700, Linus Torvalds wrote:
> > On Fri, Aug 23, 2019 at 12:24 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
> > >
> > > Confirmed the fix work.
> > >
> > > Feel free to add a Tested-by if wanted.
> > >
> > > Can this be backported to the relevant stable versions as well?
> > 
> > It's out there in my tree now. It's not explicitly marked for stable
> > per se, but it does have the "Fixes:" tag which should mean that Greg
> > and Sasha will pick it up automatically.
> > 
> > But just to make it explicit, since Greg is on the security list,
> > Darrick's fix is commit 1fb254aa983b ("xfs: fix missing ILOCK unlock
> > when xfs_setattr_nonsize fails due to EDQUOT").
> 
> Thanks, I'll pick this up.
> 
> Note, "Fixes:" will never guarantee that a patch ends up in a stable
> release.  Sasha's scripts usually do a good job of catching a lot of
> them a week or so after-the-fact, but they are not guaranteed to do so.
> A CC: stable@ will always be caught by me, and I try to ensure that
> anything goes across the security@k.o list also gets picked up.

Thank you.

FTR, for those interested in tracking this fix for the DoS potential
as vulnerability, MITRE has assigned CVE-2019-15538 for this issue.

Regards,
Salvatore