On Mon, Nov 20, 2023 at 09:50:45PM -0500, Sasha Levin wrote:
> 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))
> > > + 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
>
> That's odd. I actually ended up queueing these patches earlier, and I
> don't see any such warnings.
>
> Looking at the code, this is a bit weird too - do you see these warnings
> with the current 5.15 queue?
I did, that's where I saw this, so I dropped this commit from there, it
failed my builds using gcc-12.
thanks,
greg k-h
