From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, chandanbabu@kernel.org
Subject: Re: [PATCH 4/4] xfs: validate block count for XFS_IOC_SET_RESBLKS
Date: Wed, 3 Apr 2024 17:55:24 +1100 [thread overview]
Message-ID: <Zgz9XG0p8vioG3UB@dread.disaster.area> (raw)
In-Reply-To: <20240403035314.GL6390@frogsfrogsfrogs>
On Tue, Apr 02, 2024 at 08:53:14PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 03, 2024 at 08:38:19AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Userspace can pass anything it wants in the reserved block count
> > and we simply pass that to the reservation code. If a value that is
> > far too large is passed, we can overflow the free space counter
> > and df reports things like:
> >
> > Filesystem Size Used Avail Use% Mounted on
> > /dev/loop0 14M -27Z 27Z - /home/dave/bugs/file0
> >
> > As reserving space requires CAP_SYS_ADMIN, this is not a problem
> > that will ever been seen in production systems. However, fuzzers are
> > running with CAP_SYS_ADMIN, and so they able to run filesystem code
> > with out-of-band free space accounting.
> >
> > Stop the fuzzers ifrom being able to do this by validating that the
> > count is within the bounds of the filesystem size and reject
> > anything outside those bounds as invalid.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > fs/xfs/xfs_ioctl.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index d0e2cec6210d..18a225d884dd 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1892,6 +1892,9 @@ xfs_ioctl_getset_resblocks(
> > if (copy_from_user(&fsop, arg, sizeof(fsop)))
> > return -EFAULT;
> >
> > + if (fsop.resblks >= mp->m_sb.sb_dblocks)
> > + return -EINVAL;
>
> Why isn't xfs_reserve_blocks catching this?
xfs_reserve_blocks() assumes that values have already been bounds
checked and are valid, so calculations won't overflow. Nothing in
the internal calls to xfs_reserve_blocks() will pass an out-of-range
value, but the value coming from userspace via the ioctl is
completely unbounded. Bounding checks should be done in the code
processing the unbounded input, not the internal functions.
> Is this due to the odd
> behavior that a failed xfs_mod_fdblocks is undone and m_resblks simply
> allowed to remain?
I don't know - I couldn't work out where the overflow was occurring
from reading the code, and once I realised that all the internal
calls are using sanitised values and the ioctl didn't sanitise the
user input, the fix was obvious....
> Also why wouldn't we limit m_resblks to something smaller, like 10% of
> the fs or half an AG or something like that?
Because now we bikeshed over what is a useful limit for userspace to be
setting, rather than focussing on fixing the bug. i.e. this bug fix
doesn't limit the actual usable range that userspace can reserve,
but it prevents the overflow from occurring.i
If you want to set a limit on this value and update the code,
manpages, etc to document the new behaviour and then have to change
it when someone inevitably says "I have a workflow that temporarily
reserves 75% of the disk space because ....", then do it as a
separate "new feature" patchset. In the mean time, we just need the
overflow to go away....
-Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2024-04-03 6:55 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 21:38 [PATCH 0/4] xfs: fixes for 6.9-rcX Dave Chinner
2024-04-02 21:38 ` [PATCH 1/4] xfs: use kvmalloc for xattr buffers Dave Chinner
2024-04-03 3:43 ` Darrick J. Wong
2024-04-03 4:39 ` Christoph Hellwig
2024-04-03 6:16 ` Dave Chinner
2024-04-03 6:19 ` Christoph Hellwig
2024-04-17 14:35 ` Pankaj Raghav (Samsung)
2024-04-02 21:38 ` [PATCH 2/4] xfs: xfs_alloc_file_space() fails to detect ENOSPC Dave Chinner
2024-04-03 3:46 ` Darrick J. Wong
2024-04-03 4:40 ` Christoph Hellwig
2024-04-03 6:34 ` Dave Chinner
2024-04-03 18:23 ` Christoph Hellwig
2024-04-02 21:38 ` [PATCH 3/4] xfs: handle allocation failure in xfs_dquot_disk_alloc() Dave Chinner
2024-04-03 3:48 ` Darrick J. Wong
2024-04-03 4:41 ` Christoph Hellwig
2024-04-03 4:54 ` Darrick J. Wong
2024-04-03 4:56 ` Christoph Hellwig
2024-04-03 5:04 ` Darrick J. Wong
2024-04-03 6:41 ` Dave Chinner
2024-04-03 14:06 ` Christoph Hellwig
2024-04-03 21:49 ` Dave Chinner
2024-04-02 21:38 ` [PATCH 4/4] xfs: validate block count for XFS_IOC_SET_RESBLKS Dave Chinner
2024-04-03 3:53 ` Darrick J. Wong
2024-04-03 6:55 ` Dave Chinner [this message]
2024-04-03 4:43 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zgz9XG0p8vioG3UB@dread.disaster.area \
--to=david@fromorbit.com \
--cc=chandanbabu@kernel.org \
--cc=djwong@kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).