> On Thu, Oct 12, 2023 at 08:08:20AM +1100, Dave Chinner wrote:
> > On Wed, Oct 11, 2023 at 01:33:50PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > >
> > > The VFS inc_nlink function does not explicitly check for integer
> > > overflows in the i_nlink field. Instead, it checks the link count
> > > against s_max_links in the vfs_{link,create,rename} functions. XFS
> > > sets the maximum link count to 2.1 billion, so integer overflows should
> > > not be a problem.
> > >
> > > However. It's possible that online repair could find that a file has
> > > more than four billion links, particularly if the link count got
> > > corrupted while creating hardlinks to the file. The di_nlinkv2 field is
> > > not large enough to store a value larger than 2^32, so we ought to
> > > define a magic pin value of ~0U which means that the inode never gets
> > > deleted. This will prevent a UAF error if the repair finds this
> > > situation and users begin deleting links to the file.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_format.h | 6 ++++++
> > > fs/xfs/scrub/nlinks.c | 8 ++++----
> > > fs/xfs/scrub/repair.c | 12 ++++++------
> > > fs/xfs/xfs_inode.c | 28 +++++++++++++++++++++++-----
> > > 4 files changed, 39 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > > index 6409dd22530f2..320522b887bb3 100644
> > > --- a/fs/xfs/libxfs/xfs_format.h
> > > +++ b/fs/xfs/libxfs/xfs_format.h
> > > @@ -896,6 +896,12 @@ static inline uint xfs_dinode_size(int version)
> > > */
> > > #define XFS_MAXLINK ((1U << 31) - 1U)
> > >
> > > +/*
> > > + * Any file that hits the maximum ondisk link count should be pinned to avoid
> > > + * a use-after-free situation.
> > > + */
> > > +#define XFS_NLINK_PINNED (~0U)
> > > +
> > > /*
> > > * Values for di_format
> > > *
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 4db2c2a6538d6..30604e11182c4 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -910,15 +910,25 @@ xfs_init_new_inode(
> > > */
> > > static int /* error */
> > > xfs_droplink(
> > > - xfs_trans_t *tp,
> > > - xfs_inode_t *ip)
> > > + struct xfs_trans *tp,
> > > + struct xfs_inode *ip)
> > > {
> > > + struct inode *inode = VFS_I(ip);
> > > +
> > > xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
> > >
> > > - drop_nlink(VFS_I(ip));
> > > + if (inode->i_nlink == 0) {
> > > + xfs_info_ratelimited(tp->t_mountp,
> > > + "Inode 0x%llx link count dropped below zero. Pinning link count.",
> > > + ip->i_ino);
> > > + set_nlink(inode, XFS_NLINK_PINNED);
> > > + }
> > > + if (inode->i_nlink != XFS_NLINK_PINNED)
> > > + drop_nlink(inode);
> > > +
> > > xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >
> > I think the di_nlink field now needs to be checked by verifiers to
> > ensure the value is in the range of:
> >
> > (0 <= di_nlink <= XFS_MAXLINKS || di_nlink == XFS_NLINK_PINNED)
> >
> > And we need to ensure that in xfs_bumplink() - or earlier (top avoid
> > dirty xaction cancle shutdowns) - that adding a count to di_nlink is
> > not going to exceed XFS_MAXLINKS....
> I think the VFS needs to check that unlinking a nondirectory won't
> underflow its link count, and that rmdiring an (empty) subdirectory
> won't underflow the link counts of the parent or child.
> Cheng Lin, would you please fix all the filesystems at once instead of
> just XFS?
As FS infrastructure, its change may have a significant impact.
> --D
