Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1.1: Type: text/plain, Size: 1528 bytes --]

On Tue, 2011-10-18 at 02:18 -0200, Carlos Maiolino wrote:
> Fixes a possible memory corruption when the link is larger than
> MAXPATHLEN and XFS_DEBUG is not enabled. This also remove the
> S_ISLNK assert, since the inode mode is checked previously in
> xfs_readlink_by_handle() and via VFS.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_vnodeops.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 51fc429..c3288be 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -123,13 +123,18 @@ xfs_readlink(
>  
>  	xfs_ilock(ip, XFS_ILOCK_SHARED);
>  
> -	ASSERT(S_ISLNK(ip->i_d.di_mode));
> -	ASSERT(ip->i_d.di_size <= MAXPATHLEN);
> -
>  	pathlen = ip->i_d.di_size;

pathlen is a signed int (32-bit) and di_size has signed 64-bit type.
So, even if di_size was verified to be non-negative earlier (is it?)...

>  	if (!pathlen)
>  		goto out;
>  
> +	if (pathlen > MAXPATHLEN) {

...pathlen may be negative here and will pass this check.

Ben.

> +		xfs_alert(mp, "%s: inode (%llu) symlink length (%d) too long",
> +			 __func__, (unsigned long long)ip->i_ino, pathlen);
> +		ASSERT(0);
> +		return XFS_ERROR(EFSCORRUPTED);
> +	}
> +
> +
>  	if (ip->i_df.if_flags & XFS_IFINLINE) {
>  		memcpy(link, ip->i_df.if_u1.if_data, pathlen);
>  		link[pathlen] = '\0';

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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