On Mon, Nov 20, 2023 at 04:38:24PM +0100, Greg KH wrote:
> On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote:
> > From: ChenXiaoSong <chenxiaosong2@xxxxxxxxxx>
> >
> > [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ]
> >
> > Reproducer:
> > 1. fallocate -l 100M image
> > 2. mkfs.xfs -f image
> > 3. mount image /mnt
> > 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE)
> > 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00"
> > "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7";
> > fd = open("/mnt", O_RDONLY|O_DIRECTORY);
> > ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg);
> >
> > NULL pointer dereference will occur when race happens between xfs_getbmap()
> > and xfs_bmap_set_attrforkoff():
> >
> > ioctl | setxattr
> > ----------------------------|---------------------------
> > xfs_getbmap |
> > xfs_ifork_ptr |
> > xfs_inode_has_attr_fork |
> > ip->i_forkoff == 0 |
> > return NULL |
> > ifp == NULL |
> > | xfs_bmap_set_attrforkoff
> > | ip->i_forkoff > 0
> > xfs_inode_has_attr_fork |
> > ip->i_forkoff > 0 |
> > ifp == NULL |
> > ifp->if_format |
> >
> > Fix this by locking i_lock before xfs_ifork_ptr().
> >
> > Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers")
> > Signed-off-by: ChenXiaoSong <chenxiaosong2@xxxxxxxxxx>
> > Signed-off-by: Guo Xuenan <guoxuenan@xxxxxxxxxx>
> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > [djwong: added fixes tag]
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > Signed-off-by: Leah Rumancik <leah.rumancik@xxxxxxxxx>
> > Acked-by: Chandan Babu R <chandanbabu@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_bmap_util.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index fd2ad6a3019c..bea6cc26abf9 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -439,29 +439,28 @@ xfs_getbmap(
> > whichfork = XFS_COW_FORK;
> > else
> > whichfork = XFS_DATA_FORK;
> > - ifp = XFS_IFORK_PTR(ip, whichfork);
> >
> > xfs_ilock(ip, XFS_IOLOCK_SHARED);
> > switch (whichfork) {
> > case XFS_ATTR_FORK:
> > + lock = xfs_ilock_attr_map_shared(ip);
> > if (!XFS_IFORK_Q(ip))
> > - goto out_unlock_iolock;
> > + goto out_unlock_ilock;
> >
> > max_len = 1LL << 32;
> > - lock = xfs_ilock_attr_map_shared(ip);
> > break;
> > case XFS_COW_FORK:
> > + lock = XFS_ILOCK_SHARED;
> > + xfs_ilock(ip, lock);
> > +
> > /* No CoW fork? Just return */
> > - if (!ifp)
> > - goto out_unlock_iolock;
> > + if (!XFS_IFORK_PTR(ip, whichfork))
Is this the line 457 that the compiler complains about below?
If so, then whichfork == XFS_COW_FORK here, because the code just
switch()d on that. The XFS_IFORK_PTR macro decomposes into:
#define XFS_IFORK_PTR(ip,w) \
((w) == XFS_DATA_FORK ? \
&(ip)->i_df : \
((w) == XFS_ATTR_FORK ? \
(ip)->i_afp : \
(ip)->i_cowfp))
which means this test /should/ be turning into:
if (!(ip->i_cowfp))
goto out_unlock_ilock;
I'm not sure why your compiler is whining about &ip->i_df; that's not
what's being selected here for testing. Unless your compiler is somehow
deciding that XFS_DATA_FORK == XFS_COW_FORK? This should not ever be
possible.
--D
> > + goto out_unlock_ilock;
> >
> > if (xfs_get_cowextsz_hint(ip))
> > max_len = mp->m_super->s_maxbytes;
> > else
> > max_len = XFS_ISIZE(ip);
> > -
> > - lock = XFS_ILOCK_SHARED;
> > - xfs_ilock(ip, lock);
> > break;
> > case XFS_DATA_FORK:
> > if (!(iflags & BMV_IF_DELALLOC) &&
> > @@ -491,6 +490,8 @@ xfs_getbmap(
> > break;
> > }
> >
> > + ifp = XFS_IFORK_PTR(ip, whichfork);
> > +
> > switch (ifp->if_format) {
> > case XFS_DINODE_FMT_EXTENTS:
> > case XFS_DINODE_FMT_BTREE:
> > --
> > 2.43.0.rc0.421.g78406f8d94-goog
> >
>
> This patch breaks the build, how was it tested?
>
> fs/xfs/xfs_bmap_util.c: In function ‘xfs_getbmap’:
> fs/xfs/xfs_bmap_util.c:457:21: error: the comparison will always evaluate as ‘true’ for the address of ‘i_df’ will never be NULL [-Werror=address]
> 457 | if (!XFS_IFORK_PTR(ip, whichfork))
> | ^
> In file included from fs/xfs/xfs_bmap_util.c:16:
> fs/xfs/xfs_inode.h:38:33: note: ‘i_df’ declared here
> 38 | struct xfs_ifork i_df; /* data fork */
> | ^~~~
> cc1: all warnings being treated as errors
>
>
