Fix possible memory corruption in xfs_readlink
Quoted 15 lines · Carlos Maiolino
Fixes a possible memory corruption when the link is larger than MAXPATHLEN and
XFS_DEBUGis not enabled. This also remove theS_ISLNKassert, since the inode mode is checked previously inxfs_readlink_by_handle()and via VFS.Signed-off-by Carlos Maiolino <[email protected]>--- fs/xfs/xfs_vnodeops.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)fs/xfs/xfs_vnodeops.c +0
-0
fs/xfs/xfs_vnodeops.c xfs_readlink( · line 126
Quoted 7 lines
fs/xfs/xfs_vnodeops.c +0
-3@@ -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?)...
fs/xfs/xfs_vnodeops.c xfs_readlink( · line 130
Quoted 4 lines
fs/xfs/xfs_vnodeops.c +1
-0if (!pathlen) goto out; +if (pathlen >MAXPATHLEN ) {
...pathlen may be negative here and will pass this check.
Ben.
Quoted 10 lines
fs/xfs/xfs_vnodeops.c +7
-0+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' ;
Signature · 2 lines
Ben Hutchings Computers are not intelligent. They only think they are.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
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.
Quoted 10 lines · Ben Hutchings
Quoted 9 lines · Carlos Maiolino
Fixes a possible memory corruption when the link is larger than MAXPATHLEN and
XFS_DEBUGis not enabled. This also remove theS_ISLNKassert, since the inode mode is checked previously inxfs_readlink_by_handle()and via VFS.Signed-off-by Carlos Maiolino <[email protected]>--- 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.
Quoted 6 lines
Quoted 6 lines
fs/xfs/xfs_vnodeops.c +0
-0@@ -123,13 +123,18 @@ xfs_readlink(
Quoted 7 lines
fs/xfs/xfs_vnodeops.c xfs_readlink( · line 126
Quoted 6 lines
fs/xfs/xfs_vnodeops.c +0
-3xfs_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_sizehas 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.
Quoted 1 line
So, even if
di_sizewas verified to be non-negative earlier (is it?)...
More on this question below.
Quoted 8 lines
fs/xfs/xfs_vnodeops.c xfs_readlink( · line 130
Quoted 4 lines
fs/xfs/xfs_vnodeops.c +1
-0if (!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
Quoted 7 lines
Quoted 7 lines
fs/xfs/xfs_vnodeops.c +7
-0+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 ); + } + +
Quoted 8 lines
fs/xfs/xfs_vnodeops.c xfs_readlink( · line 140
Quoted 3 lines
fs/xfs/xfs_vnodeops.c +0
-0if (ip->i_df.if_flags &XFS_IFINLINE ) {memcpy (link, ip->i_df.if_u1.if_data, pathlen); link[pathlen] ='\0' ;_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · 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 thatXFS_DFORK_DSIZEis 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/1 xfs: Fix possible memory corruption in xfs_readlink
In reply to Ben HutchingsFrom: Carlos Maiolino <[email protected]>
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 <[email protected]>
--- fs/xfs/xfs_vnodeops.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
fs/xfs/xfs_vnodeops.c +10 -4
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH, updated] xfs: Fix possible memory corruption in xfs_readlink
ACK, below my signoff
Quoted 19 lines · Alex Elder
From: Carlos Maiolino <[email protected]>
Fixes a possible memory corruption when the link is larger than MAXPATHLEN and
XFS_DEBUGis not enabled. This also remove theS_ISLNKassert, since the inode mode is checked previously inxfs_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 bexfs_fsize_t, to match that ofip->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.
Quoted 1 line
Signed-off-by Alex Elder <[email protected]>
Quoted 48 lines
--- fs/xfs/xfs_vnodeops.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)Index:
b/fs/xfs/xfs_vnodeops.c
===================================================================fs/xfs/xfs_vnodeops.c +10
-4@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Signature · 6 lines
--Carlos _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH, updated] xfs: Fix possible memory corruption in xfs_readlink
In reply to Alex ElderLooks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs