[PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Siddharth Chandrasekaran]

Due to lack of proper validation that cached inodes are free during allocation,
causes a crash (refer to CVE-2018-13093 for more details). To address this
issue, I'm backporting upstream commit [1] to 4.4 and 4.9 stable trees
(a backport of [1] to 4.14 already exists).

Also, commit [1] references another commit [2] which added checks only to
xfs_iget_cache_miss(). In this patch, those checks have been moved into a
dedicated checker method and both xfs_iget_cache_miss() and
xfs_iget_cache_hit() are made to call that method. This code reorg in commit
[1], makes commit [2] redundant in the history of the 4.9 and 4.4 stable
trees. So commit [2] is not being backported.

-- Sid

[1]: afca6c5b2595f ("xfs: validate cached inodes are free when allocated")
[2]: ee457001ed6c ("xfs: catch inode allocation state mismatch corruption")

[v4.9]
Dave Chinner (1):
  xfs: More robust inode extent count validation

 fs/xfs/libxfs/xfs_format.h    |   3 ++
 fs/xfs/libxfs/xfs_inode_buf.c | 112 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 112 insertions(+), 3 deletions(-)

[v.4.4]
Dave Chinner (1):
  xfs: validate cached inodes are free when allocated

 fs/xfs/xfs_icache.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v4.9] xfs: More robust inode extent count validation

[Siddharth Chandrasekaran]

From: Dave Chinner <dchinner@redhat.com>

commit 23fcb3340d033d9f081e21e6c12c2db7eaa541d3 upstream.

When the inode is in extent format, it can't have more extents that
fit in the inode fork. We don't currenty check this, and so this
corruption goes unnoticed by the inode verifiers. This can lead to
crashes operating on invalid in-memory structures.

Attempts to access such a inode will now error out in the verifier
rather than allowing modification operations to proceed.

Reported-by: Wen Xu <wen.xu@gatech.edu>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: fix a typedef, add some braces and breaks to shut up compiler warnings]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Siddharth Chandrasekaran <csiddharth@vmware.com>
---
 fs/xfs/libxfs/xfs_format.h    |   3 ++
 fs/xfs/libxfs/xfs_inode_buf.c | 112 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 6b7579e..9967d30 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -979,6 +979,9 @@ typedef enum xfs_dinode_fmt {
 		XFS_DFORK_DSIZE(dip, mp) : \
 		XFS_DFORK_ASIZE(dip, mp))
 
+#define XFS_DFORK_MAXEXT(dip, mp, w) \
+	(XFS_DFORK_SIZE(dip, mp, w) / sizeof(struct xfs_bmbt_rec))
+
 /*
  * Return pointers to the data or attribute forks.
  */
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 37ee7f0..ee035a4 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -381,7 +381,48 @@ xfs_log_dinode_to_disk(
 	}
 }
 
-static bool
+static xfs_failaddr_t
+xfs_dinode_verify_fork(
+	struct xfs_dinode	*dip,
+	struct xfs_mount	*mp,
+	int			whichfork)
+{
+	uint32_t		di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork);
+
+	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
+	case XFS_DINODE_FMT_LOCAL:
+		/*
+		 * no local regular files yet
+		 */
+		if (whichfork == XFS_DATA_FORK) {
+			if (S_ISREG(be16_to_cpu(dip->di_mode)))
+				return __this_address;
+			if (be64_to_cpu(dip->di_size) >
+					XFS_DFORK_SIZE(dip, mp, whichfork))
+				return __this_address;
+		}
+		if (di_nextents)
+			return __this_address;
+		break;
+	case XFS_DINODE_FMT_EXTENTS:
+		if (di_nextents > XFS_DFORK_MAXEXT(dip, mp, whichfork))
+			return __this_address;
+		break;
+	case XFS_DINODE_FMT_BTREE:
+		if (whichfork == XFS_ATTR_FORK) {
+			if (di_nextents > MAXAEXTNUM)
+				return __this_address;
+		} else if (di_nextents > MAXEXTNUM) {
+			return __this_address;
+		}
+		break;
+	default:
+		return __this_address;
+	}
+	return NULL;
+}
+
+xfs_failaddr_t
 xfs_dinode_verify(
 	struct xfs_mount	*mp,
 	struct xfs_inode	*ip,
@@ -403,8 +444,73 @@ xfs_dinode_verify(
 		return false;
 
 	/* No zero-length symlinks/dirs. */
-	if ((S_ISLNK(mode) || S_ISDIR(mode)) && dip->di_size == 0)
-		return false;
+	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
+		return __this_address;
+
+	/* Fork checks carried over from xfs_iformat_fork */
+	if (mode &&
+	    be32_to_cpu(dip->di_nextents) + be16_to_cpu(dip->di_anextents) >
+			be64_to_cpu(dip->di_nblocks))
+		return __this_address;
+
+	if (mode && XFS_DFORK_BOFF(dip) > mp->m_sb.sb_inodesize)
+		return __this_address;
+
+	flags = be16_to_cpu(dip->di_flags);
+
+	if (mode && (flags & XFS_DIFLAG_REALTIME) && !mp->m_rtdev_targp)
+		return __this_address;
+
+	/* Do we have appropriate data fork formats for the mode? */
+	switch (mode & S_IFMT) {
+	case S_IFIFO:
+	case S_IFCHR:
+	case S_IFBLK:
+	case S_IFSOCK:
+		if (dip->di_format != XFS_DINODE_FMT_DEV)
+			return __this_address;
+		break;
+	case S_IFREG:
+	case S_IFLNK:
+	case S_IFDIR:
+		fa = xfs_dinode_verify_fork(dip, mp, XFS_DATA_FORK);
+		if (fa)
+			return fa;
+		break;
+	case 0:
+		/* Uninitialized inode ok. */
+		break;
+	default:
+		return __this_address;
+	}
+
+	if (XFS_DFORK_Q(dip)) {
+		fa = xfs_dinode_verify_fork(dip, mp, XFS_ATTR_FORK);
+		if (fa)
+			return fa;
+	} else {
+		/*
+		 * If there is no fork offset, this may be a freshly-made inode
+		 * in a new disk cluster, in which case di_aformat is zeroed.
+		 * Otherwise, such an inode must be in EXTENTS format; this goes
+		 * for freed inodes as well.
+		 */
+		switch (dip->di_aformat) {
+		case 0:
+		case XFS_DINODE_FMT_EXTENTS:
+			break;
+		default:
+			return __this_address;
+		}
+		if (dip->di_anextents)
+			return __this_address;
+	}
+
+	/* extent size hint validation */
+	fa = xfs_inode_validate_extsize(mp, be32_to_cpu(dip->di_extsize),
+			mode, flags);
+	if (fa)
+		return fa;
 
 	/* only version 3 or greater inodes are extensively verified here */
 	if (dip->di_version < 3)
-- 
2.7.4

[PATCH v4.4] xfs: validate cached inodes are free when allocated

[Siddharth Chandrasekaran]

From: Dave Chinner <dchinner@redhat.com>

commit afca6c5b2595fc44383919fba740c194b0b76aff upstream.

A recent fuzzed filesystem image cached random dcache corruption
when the reproducer was run. This often showed up as panics in
lookup_slow() on a null inode->i_ops pointer when doing pathwalks.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
....
Call Trace:
 lookup_slow+0x44/0x60
 walk_component+0x3dd/0x9f0
 link_path_walk+0x4a7/0x830
 path_lookupat+0xc1/0x470
 filename_lookup+0x129/0x270
 user_path_at_empty+0x36/0x40
 path_listxattr+0x98/0x110
 SyS_listxattr+0x13/0x20
 do_syscall_64+0xf5/0x280
 entry_SYSCALL_64_after_hwframe+0x42/0xb7

but had many different failure modes including deadlocks trying to
lock the inode that was just allocated or KASAN reports of
use-after-free violations.

The cause of the problem was a corrupt INOBT on a v4 fs where the
root inode was marked as free in the inobt record. Hence when we
allocated an inode, it chose the root inode to allocate, found it in
the cache and re-initialised it.

We recently fixed a similar inode allocation issue caused by inobt
record corruption problem in xfs_iget_cache_miss() in commit
ee457001ed6c ("xfs: catch inode allocation state mismatch
corruption"). This change adds similar checks to the cache-hit path
to catch it, and turns the reproducer into a corruption shutdown
situation.

Reported-by: Wen Xu <wen.xu@gatech.edu>
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: fix typos in comment]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Siddharth Chandrasekaran <csiddharth@vmware.com>
---
 fs/xfs/xfs_icache.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index adbc1f5..7efeefb 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -135,6 +135,46 @@ xfs_inode_free(
 }
 
 /*
+ * If we are allocating a new inode, then check what was returned is
+ * actually a free, empty inode. If we are not allocating an inode,
+ * then check we didn't find a free inode.
+ *
+ * Returns:
+ *	0		if the inode free state matches the lookup context
+ *	-ENOENT		if the inode is free and we are not allocating
+ *	-EFSCORRUPTED	if there is any state mismatch at all
+ */
+static int
+xfs_iget_check_free_state(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	if (flags & XFS_IGET_CREATE) {
+		/* should be a free inode */
+		if (VFS_I(ip)->i_mode != 0) {
+			xfs_warn(ip->i_mount,
+"Corruption detected! Free inode 0x%llx not marked free! (mode 0x%x)",
+				ip->i_ino, VFS_I(ip)->i_mode);
+			return -EFSCORRUPTED;
+		}
+
+		if (ip->i_d.di_nblocks != 0) {
+			xfs_warn(ip->i_mount,
+"Corruption detected! Free inode 0x%llx has blocks allocated!",
+				ip->i_ino);
+			return -EFSCORRUPTED;
+		}
+		return 0;
+	}
+
+	/* should be an allocated inode */
+	if (VFS_I(ip)->i_mode == 0)
+		return -ENOENT;
+
+	return 0;
+}
+
+/*
  * Check the validity of the inode we just found it the cache
  */
 static int
@@ -183,12 +223,12 @@ xfs_iget_cache_hit(
 	}
 
 	/*
-	 * If lookup is racing with unlink return an error immediately.
+	 * Check the inode free state is valid. This also detects lookup
+	 * racing with unlinks.
 	 */
-	if (ip->i_d.di_mode == 0 && !(flags & XFS_IGET_CREATE)) {
-		error = -ENOENT;
+	error = xfs_iget_check_free_state(ip, flags);
+	if (error)
 		goto out_error;
-	}
 
 	/*
 	 * If IRECLAIMABLE is set, we've torn down the VFS inode already.
@@ -298,10 +338,13 @@ xfs_iget_cache_miss(
 
 	trace_xfs_iget_miss(ip);
 
-	if ((ip->i_d.di_mode == 0) && !(flags & XFS_IGET_CREATE)) {
-		error = -ENOENT;
+	/*
+	 * Check the inode free state is valid. This also detects lookup
+	 * racing with unlinks.
+	 */
+	error = xfs_iget_check_free_state(ip, flags);
+	if (error)
 		goto out_destroy;
-	}
 
 	/*
 	 * Preload the radix tree so we can insert safely under the
-- 
2.7.4

Re: [PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Siddharth Chandrasekaran]

Please ignore this patch set, I accidentally added another patch I was
working on. Will send v2 with the right patches.

Thanks!

-- Sid.

Re: [PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Greg KH]

On Fri, May 15, 2020 at 06:19:45PM +0530, Siddharth Chandrasekaran wrote:
> Please ignore this patch set, I accidentally added another patch I was
> working on. Will send v2 with the right patches.

What patch set?  I see nothing in this email, so I have no idea what you
are referring to :(

greg k-h

Re: [PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Siddharth Chandrasekaran]

On Fri, May 15, 2020 at 02:57:01PM +0200, Greg KH wrote:
> On Fri, May 15, 2020 at 06:19:45PM +0530, Siddharth Chandrasekaran wrote:
> > Please ignore this patch set, I accidentally added another patch I was
> > working on. Will send v2 with the right patches.
> 
> What patch set?  I see nothing in this email, so I have no idea what you
> are referring to :(

Apologies! Looks like my email thread was broken. I was referring to
the thread here: https://lkml.org/lkml/2020/5/14/1326 with subject:
  
  "[PATCH] Backport security fixe to 4.9 and 4.4 stable trees"

The corrected version (v2) of this patch should have reached you
(hopefully) with the subject:

  "[PATCH v2] Backport xfs security fix to 4.9 and 4.4 stable trees"

Thanks!

-- Sid.

Re: [PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Greg KH]

On Fri, May 15, 2020 at 06:59:43PM +0530, Siddharth Chandrasekaran wrote:
> On Fri, May 15, 2020 at 02:57:01PM +0200, Greg KH wrote:
> > On Fri, May 15, 2020 at 06:19:45PM +0530, Siddharth Chandrasekaran wrote:
> > > Please ignore this patch set, I accidentally added another patch I was
> > > working on. Will send v2 with the right patches.
> > 
> > What patch set?  I see nothing in this email, so I have no idea what you
> > are referring to :(
> 
> Apologies! Looks like my email thread was broken. I was referring to
> the thread here: https://lkml.org/lkml/2020/5/14/1326 with subject:
>   
>   "[PATCH] Backport security fixe to 4.9 and 4.4 stable trees"
> 
> The corrected version (v2) of this patch should have reached you
> (hopefully) with the subject:
> 
>   "[PATCH v2] Backport xfs security fix to 4.9 and 4.4 stable trees"

Sorry, I see no such email in my staging patch queue.

Do you have a link to the series on lore.kernel.org?  lkml.org is a mess
and not under our control.

greg k-h

Re: [PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Steven Rostedt]

On Fri, 15 May 2020 15:40:09 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> > 
> >   "[PATCH v2] Backport xfs security fix to 4.9 and 4.4 stable trees"  
> 
> Sorry, I see no such email in my staging patch queue.
> 
> Do you have a link to the series on lore.kernel.org?  lkml.org is a mess
> and not under our control.

That's because it Cc'd stable@kernel.org and not stable@vger.kernel.org.

Should Siddharth resend?

-- Steve

Re: [PATCH] Backport security fixe to 4.9 and 4.4 stable trees

[Greg KH]

On Fri, May 15, 2020 at 09:52:16AM -0400, Steven Rostedt wrote:
> On Fri, 15 May 2020 15:40:09 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > > 
> > >   "[PATCH v2] Backport xfs security fix to 4.9 and 4.4 stable trees"  
> > 
> > Sorry, I see no such email in my staging patch queue.
> > 
> > Do you have a link to the series on lore.kernel.org?  lkml.org is a mess
> > and not under our control.
> 
> That's because it Cc'd stable@kernel.org and not stable@vger.kernel.org.
> 
> Should Siddharth resend?

Yes, stable@kernel.org is a direct path to /dev/null on the kernel.org
mail server.

thanks,

greg k-h