Re: [PATCH] Fix possible memory corruption in xfs_readlink

Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Ben Hutchings]

[-- 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

Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Alex Elder]

I don't konw why, but I *think* the response I
thought I sent earlier didn't actually make it
out.

Just in case, I'm trying to recreate what I had
before, below.  Sorry if something like this
shows up twice.

On Tue, 2011-11-01 at 14:14 +0000, Ben Hutchings wrote:
> 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(-)

A few comments inline below, followed by Ben's original
message and some explanation from me.

> > 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.

I concur, di_size here is an xfs_fsize_t, which is defined
as __int64_t (a signed 64-bit integer).  pathlen is defined
as a (signed) int.

> So, even if di_size was verified to be non-negative earlier (is it?)...

More on this question below.

> >  	if (!pathlen)
> >  		goto out;
> >  
> > +	if (pathlen > MAXPATHLEN) {
> 
> ...pathlen may be negative here and will pass this check.
> 
> Ben.

You are right to call attention to this.  I think defining
pathlen to be an int here is a mistake in any case (the type
ought to match that of id.di_size), though in practice it
will not be a problem.

You mention two remaining issues:
- can a value held in ip->i_d.di_size result in a negative
  value in pathlen as a result of the assignment?
- is ip->i_d.di_size guaranteed (verified) to be non-negative?

On the first question, the C standard says that the result of
the assignment--if id.di_size exceeds what can be represented
by pathlen--is implementation defined, therefore it is not
safe.  So you're right, this needs to be fixed.

On the second question, ip->i_d.di_size is assigned
in a lot of places.  I started looking at all the
places where this field gets assigned.  In about half
of them I examined the assignment obviously left its
value non-negative, or only allowing a negative assignment
if the previous value was already negative.

But rather than complete this research task, I think
it will be better (for now) to simply check for a negative
ip->i_d.di_size, and if it's seen, either return
an error or initiate a forced shutdown (since it
represents corruption).

I'm interested in what others think.

					-Alex

> > +		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';
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs





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

Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Christoph Hellwig]

We should validate that the value isn't negative in xfs_iformat_*,
although we currently don't do that.  It already verified that it
fits into the XFS_DFORK_DSIZE, which should take care of fitting
into 32-bits.  Adding another explicit check probably won't hurt,
given that XFS_DFORK_DSIZE is calculated dynamically based on the
fork offset.

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

Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Alex Elder]

On Wed, 2011-11-02 at 15:45 -0400, Christoph Hellwig wrote:
> We should validate that the value isn't negative in xfs_iformat_*,
> although we currently don't do that.  It already verified that it
> fits into the XFS_DFORK_DSIZE, which should take care of fitting
> into 32-bits.  Adding another explicit check probably won't hurt,
> given that XFS_DFORK_DSIZE is calculated dynamically based on the
> fork offset.
> 

That's true, but there are other places where it gets
updated, yet not defensively validated.  For example,
in xfs_dir2_shrink_inode(), if:
    fsbno > (INT64_MAX >> mp->m_sb.sb_blocklog)
then the (signed) di_size field would be assigned
a value that exceeded its max representable value,
producing unreliable (implementation-defined) results.
That may well be an impossible situation, but it's
not obvious without really looking at the code.

It's a bit of a can of worms, which is why I suggested
just testing for this (unlikely) condition in
xfs_readlink() for now.

					-Alex

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

[PATCH, updated] xfs: Fix possible memory corruption in xfs_readlink

[Alex Elder]

From: Carlos Maiolino <cmaiolino@redhat.com>

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.

Updated to address concerns raised by Ben Hutchings about the loose
attention paid to 32- vs 64-bit values, and the lack of handling a
potentially negative pathlen value:
 - Changed type of "pathlen" to be xfs_fsize_t, to match that of
   ip->i_d.di_size
 - Added checking for a negative pathlen to the too-long pathlen
   test, and generalized the message that gets reported in that case
   to reflect the change
As a result, if a negative pathlen were encountered, this function
would return EFSCORRUPTED (and would fail an assertion for a debug
build)--just as would a too-long pathlen.

Signed-off-by Alex Elder <aelder@sgi.com>

---
 fs/xfs/xfs_vnodeops.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Index: b/fs/xfs/xfs_vnodeops.c
===================================================================
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -112,7 +112,7 @@ xfs_readlink(
 	char		*link)
 {
 	xfs_mount_t	*mp = ip->i_mount;
-	int		pathlen;
+	xfs_fsize_t	pathlen;
 	int		error = 0;
 
 	trace_xfs_readlink(ip);
@@ -122,13 +122,19 @@ 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;
 	if (!pathlen)
 		goto out;
 
+	if (pathlen < 0 || pathlen > MAXPATHLEN) {
+		xfs_alert(mp, "%s: inode (%llu) bad symlink length (%lld)",
+			 __func__, (unsigned long long) ip->i_ino,
+			 (long long) 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';



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

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

[Carlos Maiolino]

ACK, below my signoff
On Mon, Nov 07, 2011 at 10:10:24AM -0600, Alex Elder wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
> 
> 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.
> 
> Updated to address concerns raised by Ben Hutchings about the loose
> attention paid to 32- vs 64-bit values, and the lack of handling a
> potentially negative pathlen value:
>  - Changed type of "pathlen" to be xfs_fsize_t, to match that of
>    ip->i_d.di_size
>  - Added checking for a negative pathlen to the too-long pathlen
>    test, and generalized the message that gets reported in that case
>    to reflect the change
> As a result, if a negative pathlen were encountered, this function
> would return EFSCORRUPTED (and would fail an assertion for a debug
> build)--just as would a too-long pathlen.
> 
> Signed-off-by Alex Elder <aelder@sgi.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> ---
>  fs/xfs/xfs_vnodeops.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> Index: b/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -112,7 +112,7 @@ xfs_readlink(
>  	char		*link)
>  {
>  	xfs_mount_t	*mp = ip->i_mount;
> -	int		pathlen;
> +	xfs_fsize_t	pathlen;
>  	int		error = 0;
>  
>  	trace_xfs_readlink(ip);
> @@ -122,13 +122,19 @@ 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;
>  	if (!pathlen)
>  		goto out;
>  
> +	if (pathlen < 0 || pathlen > MAXPATHLEN) {
> +		xfs_alert(mp, "%s: inode (%llu) bad symlink length (%lld)",
> +			 __func__, (unsigned long long) ip->i_ino,
> +			 (long long) 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';
> 
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
--Carlos

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

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

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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