Re: [PATCH] Fix possible memory corruption in xfs_readlink

[Alex Elder <aelder@sgi.com>]

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