diff mbox

[Precise,CVE-2013-1819] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

Message ID 1378284391-7044-1-git-send-email-luis.henriques@canonical.com
State New
Headers show

Commit Message

Luis Henriques Sept. 4, 2013, 8:46 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

CVE-2013-1819

BugLink: http://bugs.launchpad.net/bugs/1151527

When _xfs_buf_find is passed an out of range address, it will fail
to find a relevant struct xfs_perag and oops with a null
dereference. This can happen when trying to walk a filesystem with a
metadata inode that has a partially corrupted extent map (i.e. the
block number returned is corrupt, but is otherwise intact) and we
try to read from the corrupted block address.

In this case, just fail the lookup. If it is readahead being issued,
it will simply not be done, but if it is real read that fails we
will get an error being reported.  Ideally this case should result
in an EFSCORRUPTED error being reported, but we cannot return an
error through xfs_buf_read() or xfs_buf_get() so this lookup failure
may result in ENOMEM or EIO errors being reported instead.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Ben Myers <bpm@sgi.com>
Signed-off-by: Ben Myers <bpm@sgi.com>
(back ported from commit eb178619f930fa2ba2348de332a1ff1c66a31424)
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 fs/xfs/xfs_buf.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Stefan Bader Sept. 4, 2013, 11:47 a.m. UTC | #1
Brad Figg Sept. 4, 2013, 1:29 p.m. UTC | #2 
On 09/04/2013 01:46 AM, Luis Henriques wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> CVE-2013-1819
> 
> BugLink: http://bugs.launchpad.net/bugs/1151527
> 
> When _xfs_buf_find is passed an out of range address, it will fail
> to find a relevant struct xfs_perag and oops with a null
> dereference. This can happen when trying to walk a filesystem with a
> metadata inode that has a partially corrupted extent map (i.e. the
> block number returned is corrupt, but is otherwise intact) and we
> try to read from the corrupted block address.
> 
> In this case, just fail the lookup. If it is readahead being issued,
> it will simply not be done, but if it is real read that fails we
> will get an error being reported.  Ideally this case should result
> in an EFSCORRUPTED error being reported, but we cannot return an
> error through xfs_buf_read() or xfs_buf_get() so this lookup failure
> may result in ENOMEM or EIO errors being reported instead.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Ben Myers <bpm@sgi.com>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> (back ported from commit eb178619f930fa2ba2348de332a1ff1c66a31424)
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
>  fs/xfs/xfs_buf.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 2f5a8f7..c13fff4 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -434,6 +434,7 @@ _xfs_buf_find(
>  	struct rb_node		**rbp;
>  	struct rb_node		*parent;
>  	xfs_buf_t		*bp;
> +	xfs_daddr_t		eofs;
>  
>  	range_base = (ioff << BBSHIFT);
>  	range_length = (isize << BBSHIFT);
> @@ -442,6 +443,23 @@ _xfs_buf_find(
>  	ASSERT(!(range_length < (1 << btp->bt_sshift)));
>  	ASSERT(!(range_base & (xfs_off_t)btp->bt_smask));
>  
> +	/*
> +	 * Corrupted block numbers can get through to here, unfortunately, so we
> +	 * have to check that the buffer falls within the filesystem bounds.
> +	 */
> +	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
> +	if (ioff >= eofs) {
> +		/*
> +		 * XXX (dgc): we should really be returning EFSCORRUPTED here,
> +		 * but none of the higher level infrastructure supports
> +		 * returning a specific error on buffer lookup failures.
> +		 */
> +		xfs_alert(btp->bt_mount,
> +			  "%s: Block out of range: block 0x%llx, EOFS 0x%llx ",
> +			  __func__, ioff, eofs);
> +		return NULL;
> +	}
> +
>  	/* get tree root */
>  	pag = xfs_perag_get(btp->bt_mount,
>  				xfs_daddr_to_agno(btp->bt_mount, ioff));
>
Tim Gardner Sept. 4, 2013, 4:26 p.m. UTC | #3
Thomas GL Oct. 8, 2013, 8:34 p.m. UTC | #4 
Hi,

It seems that this patch actually breaks xfs_growfs on kernels prior to
3.8.0, and should not have been backported as-is.
This launchpad bug describes the issue I've seen too:
https://bugs.launchpad.net/ubuntu/+source/xfsprogs/+bug/1236041

Here is a reference explaining why it was not meant to be backported
to pre-3.8 kernels:
http://oss.sgi.com/archives/xfs/2013-02/msg00201.html
And the corresponding revert in 3.7.9 (after it was added in a
previous revision):
https://lkml.org/lkml/2013/2/15/638

Also, there is an alternative patch in this SUSE bug which might be a
better option for fixing CVE-2013-1819 on kernels prior to 3.8:
https://bugzilla.novell.com/show_bug.cgi?id=807471#c25
(not tested here, and must at least be edited since the patch is for
3.0, when xfs_buf.c was in a "linux-2.6" subdirectory)

Thanks,
Thomas.


On 2013/09/04, Luis Henriques <luis.henriques@canonical.com> wrote:

> From: Dave Chinner <dchinner@redhat.com>
> 
> CVE-2013-1819
> 
> BugLink: http://bugs.launchpad.net/bugs/1151527
> 
> When _xfs_buf_find is passed an out of range address, it will fail
> to find a relevant struct xfs_perag and oops with a null
> dereference. This can happen when trying to walk a filesystem with a
> metadata inode that has a partially corrupted extent map (i.e. the
> block number returned is corrupt, but is otherwise intact) and we
> try to read from the corrupted block address.
> 
> In this case, just fail the lookup. If it is readahead being issued,
> it will simply not be done, but if it is real read that fails we
> will get an error being reported.  Ideally this case should result
> in an EFSCORRUPTED error being reported, but we cannot return an
> error through xfs_buf_read() or xfs_buf_get() so this lookup failure
> may result in ENOMEM or EIO errors being reported instead.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Ben Myers <bpm@sgi.com>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> (back ported from commit eb178619f930fa2ba2348de332a1ff1c66a31424)
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
>  fs/xfs/xfs_buf.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 2f5a8f7..c13fff4 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -434,6 +434,7 @@ _xfs_buf_find(
>  	struct rb_node		**rbp;
>  	struct rb_node		*parent;
>  	xfs_buf_t		*bp;
> +	xfs_daddr_t		eofs;
>  
>  	range_base = (ioff << BBSHIFT);
>  	range_length = (isize << BBSHIFT);
> @@ -442,6 +443,23 @@ _xfs_buf_find(
>  	ASSERT(!(range_length < (1 << btp->bt_sshift)));
>  	ASSERT(!(range_base & (xfs_off_t)btp->bt_smask));
>  
> +	/*
> +	 * Corrupted block numbers can get through to here,
> unfortunately, so we
> +	 * have to check that the buffer falls within the filesystem
> bounds.
> +	 */
> +	eofs = XFS_FSB_TO_BB(btp->bt_mount,
> btp->bt_mount->m_sb.sb_dblocks);
> +	if (ioff >= eofs) {
> +		/*
> +		 * XXX (dgc): we should really be returning
> EFSCORRUPTED here,
> +		 * but none of the higher level infrastructure
> supports
> +		 * returning a specific error on buffer lookup
> failures.
> +		 */
> +		xfs_alert(btp->bt_mount,
> +			  "%s: Block out of range: block 0x%llx,
> EOFS 0x%llx ",
> +			  __func__, ioff, eofs);
> +		return NULL;
> +	}
> +
>  	/* get tree root */
>  	pag = xfs_perag_get(btp->bt_mount,
>  				xfs_daddr_to_agno(btp->bt_mount,
> ioff));
Luis Henriques Oct. 9, 2013, 9:14 a.m. UTC | #5 
Hi Thomas,

Thomas GL <tom.gl@free.fr> writes:

> Hi,
>
> It seems that this patch actually breaks xfs_growfs on kernels prior to
> 3.8.0, and should not have been backported as-is.
> This launchpad bug describes the issue I've seen too:
> https://bugs.launchpad.net/ubuntu/+source/xfsprogs/+bug/1236041
>
> Here is a reference explaining why it was not meant to be backported
> to pre-3.8 kernels:
> http://oss.sgi.com/archives/xfs/2013-02/msg00201.html
> And the corresponding revert in 3.7.9 (after it was added in a
> previous revision):
> https://lkml.org/lkml/2013/2/15/638
>
> Also, there is an alternative patch in this SUSE bug which might be a
> better option for fixing CVE-2013-1819 on kernels prior to 3.8:
> https://bugzilla.novell.com/show_bug.cgi?id=807471#c25
> (not tested here, and must at least be edited since the patch is for
> 3.0, when xfs_buf.c was in a "linux-2.6" subdirectory)

Thank you for bringing this issue to my attention and for providing
all these references.  I will revisit this backport and report back on
bug #1236041.

Cheers,
diff mbox

Patch

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2f5a8f7..c13fff4 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -434,6 +434,7 @@  _xfs_buf_find(
 	struct rb_node		**rbp;
 	struct rb_node		*parent;
 	xfs_buf_t		*bp;
+	xfs_daddr_t		eofs;
 
 	range_base = (ioff << BBSHIFT);
 	range_length = (isize << BBSHIFT);
@@ -442,6 +443,23 @@  _xfs_buf_find(
 	ASSERT(!(range_length < (1 << btp->bt_sshift)));
 	ASSERT(!(range_base & (xfs_off_t)btp->bt_smask));
 
+	/*
+	 * Corrupted block numbers can get through to here, unfortunately, so we
+	 * have to check that the buffer falls within the filesystem bounds.
+	 */
+	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
+	if (ioff >= eofs) {
+		/*
+		 * XXX (dgc): we should really be returning EFSCORRUPTED here,
+		 * but none of the higher level infrastructure supports
+		 * returning a specific error on buffer lookup failures.
+		 */
+		xfs_alert(btp->bt_mount,
+			  "%s: Block out of range: block 0x%llx, EOFS 0x%llx ",
+			  __func__, ioff, eofs);
+		return NULL;
+	}
+
 	/* get tree root */
 	pag = xfs_perag_get(btp->bt_mount,
 				xfs_daddr_to_agno(btp->bt_mount, ioff));