| Message ID | 20240402221127.1200501-5-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: fixes for 6.9-rcX | expand |
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? Is this due to the odd behavior that a failed xfs_mod_fdblocks is undone and m_resblks simply allowed to remain? Also why wouldn't we limit m_resblks to something smaller, like 10% of the fs or half an AG or something like that? --D > + > error = mnt_want_write_file(filp); > if (error) > return error; > -- > 2.43.0 > >
On Wed, Apr 03, 2024 at 08:38:19AM +1100, Dave Chinner wrote: > Stop the fuzzers ifrom being able to do this by validating that the s/ifrom/from/ > + if (fsop.resblks >= mp->m_sb.sb_dblocks) Even all of dblocks seems too much. A quarter of all blocks still feels very generous.
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.
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; + error = mnt_want_write_file(filp); if (error) return error;