Re: [PATCH] xfs: Fix possible memory corruption in xfs_readlink

[Dave Chinner <david@fromorbit.com>]

On Sun, Oct 16, 2011 at 11:26:34PM -0200, Carlos Maiolino wrote:
> This patch fix a possible memory corruption when
> the link is larger than MAXPATHLEN and XFS_DEBUG
> is not enabled. This also uses S_IFLNK to check
> link not only in DEBUG mode.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Well found, Carlos. A few comments on the fix below.

> ---
>  fs/xfs/xfs_vnodeops.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 51fc429..3f4fbd5 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -123,8 +123,22 @@ xfs_readlink(
>  
>  	xfs_ilock(ip, XFS_ILOCK_SHARED);
>  
> -	ASSERT(S_ISLNK(ip->i_d.di_mode));
> -	ASSERT(ip->i_d.di_size <= MAXPATHLEN);
> +	if (unlikely(!(S_ISLNK(ip->i_d.di_mode))) ||
> +	    unlikely(!(ip->i_d.di_size <= MAXPATHLEN ))){

No need for the unlikely - the hardware generally handles branch
prediction better than we can. ;)

> +
> +		XFS_CORRUPTION_ERROR("xfs_readlink",
> +		XFS_ERRLEVEL_HIGH, mp, ip);

At first glance this looks like the "right" error reporting macro to
use, but it realy isn't: XFS_CORRUPTION_ERROR() is for reporting
details of the on-disk structure that is corrupted. Basically it is
used to dump the first chunk of the disk buffer/structure that the
error was found in, but we only have in-memory state here so there's
nothing to dump. Yes, we can dump the first 16 bytes of ip, but that
just gets us a couple of memory pointers which has nothing to do
with the corruption just detected.

> +#ifdef DEBUG
> +		xfs_emerg(mp, "inode (%lld), link too long or not a link."
> +			 (unsigned long long)ip->i_no);
                                                 ^^^^ i_ino
> +
> +		ASSERT(S_ISLNK(ip->i_d.di_mode));
> +		ASSERT(ip->i_d.di_size <= MAXPATHLEN);

Not much point putting a second print for only debug kernels - if
it's useful information about the corruption (e.g. the inode
number), then it should be emitted for non-debug kernels, too. Yeah,
I know you probably copied it from somewhere else (like
xfs_imap_to_bmap()), but for all new detected corruptions having
some indication on normal production machines of where the
corruption was detected is very important.

IOWs, I'd simply be replacing the ASSERT()s with something like this:

	if (!(S_ISLNK(ip->i_d.di_mode) && ip->i_d.di_size <= MAXPATHLEN)) {
		xfs_alert(mp, "inode %lld: link too long (%) or not a link."
			 (unsigned long long)ip->i_ino, i_d.di_size);
		ASSERT(0);
		return XFS_ERROR(EFSCORRUPTED);
	}

It will still assert fail on a debug kernel, with enough information
to tell us exactly which condition failed. And it will detect and
return an error on a non-debug kernel, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs