Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Christoph Hellwig <hch@infradead.org>]

This generally good, but you'll need to fix formatting a bit
for both the mail body and the patch itself.

On Mon, Oct 17, 2011 at 01:30:12PM -0200, Carlos Maiolino wrote:
> Fixes 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.

Please try to fill up ~ 75 characters for each line in the mail body,
e.g.

Fix a possible memory corruption when a symlink target is larger than
MAXPATHLEN and XFS_DEBUG is not enabled.  Also use S_IFLNK to check
against disk corruption in di_mode for non-debug mode.

(I've also update the content a little bit).

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

No need for the inner braces in both branches, but per kernel coding
style there should be one before the opening brace.  Also no spaces
before the closing round braces, please.  I also think it would be
cleanrer to split this into two checks, as it's two possible
corruptions, e.g.

	if (!S_ISLNK(ip->i_d.di_mode)) {
		xfs_emerg(mp, "inode (%lld) not a link in %s\n",
			  (unsigned long long)ip->i_ino), __func__);
		ASSERT(0);
		return XFS_ERROR(EFSCORRUPTED);
	}
	if (ip->i_d.di_size > MAXPATHLEN) {
		xfs_emerg(mp, "inode (%lld) larger than MAXPATHLEN in %s\n",
			  (unsigned long long)ip->i_ino), __func__);
		ASSERT(0);
		return XFS_ERROR(EFSCORRUPTED);
	}

It might also be useful to print the length in the second case as that
would help debugging potential corruptions. (e.g. single bit flips)

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