* Re: [PATCH] Fix possible memory corruption in xfs_readlink
@ 2011-11-01 14:14 Ben Hutchings
2011-11-02 17:52 ` Alex Elder
2011-11-07 16:10 ` [PATCH, updated] xfs: " Alex Elder
0 siblings, 2 replies; 18+ messages in thread
From: Ben Hutchings @ 2011-11-01 14:14 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
[-- 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-11-01 14:14 [PATCH] Fix possible memory corruption in xfs_readlink Ben Hutchings
@ 2011-11-02 17:52 ` Alex Elder
2011-11-02 19:45 ` Christoph Hellwig
2011-11-07 16:10 ` [PATCH, updated] xfs: " Alex Elder
1 sibling, 1 reply; 18+ messages in thread
From: Alex Elder @ 2011-11-02 17:52 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Carlos Maiolino, 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.
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-11-02 17:52 ` Alex Elder
@ 2011-11-02 19:45 ` Christoph Hellwig
2011-11-02 20:22 ` Alex Elder
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2011-11-02 19:45 UTC (permalink / raw)
To: Alex Elder; +Cc: Ben Hutchings, Carlos Maiolino, 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-11-02 19:45 ` Christoph Hellwig
@ 2011-11-02 20:22 ` Alex Elder
0 siblings, 0 replies; 18+ messages in thread
From: Alex Elder @ 2011-11-02 20:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Ben Hutchings, Carlos Maiolino, xfs
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH, updated] xfs: Fix possible memory corruption in xfs_readlink
2011-11-01 14:14 [PATCH] Fix possible memory corruption in xfs_readlink Ben Hutchings
2011-11-02 17:52 ` Alex Elder
@ 2011-11-07 16:10 ` Alex Elder
2011-11-07 16:31 ` Carlos Maiolino
2011-11-08 14:38 ` Christoph Hellwig
1 sibling, 2 replies; 18+ messages in thread
From: Alex Elder @ 2011-11-07 16:10 UTC (permalink / raw)
To: XFS Mailing List; +Cc: Ben Hutchings, Carlos Maiolino
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH, updated] xfs: Fix possible memory corruption in xfs_readlink
2011-11-07 16:10 ` [PATCH, updated] xfs: " Alex Elder
@ 2011-11-07 16:31 ` Carlos Maiolino
2011-11-08 14:38 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Carlos Maiolino @ 2011-11-07 16:31 UTC (permalink / raw)
To: Alex Elder; +Cc: Ben Hutchings, XFS Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH, updated] xfs: Fix possible memory corruption in xfs_readlink
2011-11-07 16:10 ` [PATCH, updated] xfs: " Alex Elder
2011-11-07 16:31 ` Carlos Maiolino
@ 2011-11-08 14:38 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2011-11-08 14:38 UTC (permalink / raw)
To: Alex Elder; +Cc: Ben Hutchings, Carlos Maiolino, XFS Mailing List
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] Fix possible memory corruption in xfs_readlink
@ 2011-10-18 4:18 Carlos Maiolino
2011-10-18 6:52 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Carlos Maiolino @ 2011-10-18 4:18 UTC (permalink / raw)
To: xfs; +Cc: Carlos Maiolino
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;
if (!pathlen)
goto out;
+ if (pathlen > MAXPATHLEN) {
+ 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';
--
1.7.6.2
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-18 4:18 [PATCH] " Carlos Maiolino
@ 2011-10-18 6:52 ` Christoph Hellwig
2011-10-18 13:59 ` Alex Elder
2011-10-18 14:25 ` Eric Sandeen
2 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2011-10-18 6:52 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-18 4:18 [PATCH] " Carlos Maiolino
2011-10-18 6:52 ` Christoph Hellwig
@ 2011-10-18 13:59 ` Alex Elder
2011-10-18 14:25 ` Eric Sandeen
2 siblings, 0 replies; 18+ messages in thread
From: Alex Elder @ 2011-10-18 13:59 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
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>
OK, looks good. I'll commit it soon.
Reviewed-by: Alex Elder <aelder@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-18 4:18 [PATCH] " Carlos Maiolino
2011-10-18 6:52 ` Christoph Hellwig
2011-10-18 13:59 ` Alex Elder
@ 2011-10-18 14:25 ` Eric Sandeen
2 siblings, 0 replies; 18+ messages in thread
From: Eric Sandeen @ 2011-10-18 14:25 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
On 10/17/11 11:18 PM, 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>
Thanks!
Discussed-to-death-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Eric Sandeen <sandeen@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;
> if (!pathlen)
> goto out;
>
> + if (pathlen > MAXPATHLEN) {
> + 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] Fix possible memory corruption in xfs_readlink
@ 2011-10-17 21:05 Carlos Maiolino
2011-10-17 22:39 ` Alex Elder
2011-10-17 22:43 ` Dave Chinner
0 siblings, 2 replies; 18+ messages in thread
From: Carlos Maiolino @ 2011-10-17 21:05 UTC (permalink / raw)
To: xfs; +Cc: Carlos Maiolino
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..9ca6676 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;
if (!pathlen)
goto out;
+ if (pathlen > MAXPATHLEN) {
+ xfs_emerg(mp, "%s: inode (%lld) 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';
--
1.7.6.2
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-17 21:05 Carlos Maiolino
@ 2011-10-17 22:39 ` Alex Elder
2011-10-17 22:43 ` Dave Chinner
1 sibling, 0 replies; 18+ messages in thread
From: Alex Elder @ 2011-10-17 22:39 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
On Mon, 2011-10-17 at 19:05 -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>
I know this was discussed to death on IRC. But I didn't
get a chance to be a part of that committee so I have
a suggested change: use %llu format, not %lld.
Just to clarify, this is addressing something that could
happen if a corrupt filesystem led to an inode whose flags
indicate it's a symlink has a size that exceeds the maximum
path length. And without your fix, the memcpy() in
xfs_readlink() could overflow the memory it's provided.
I can implement the format string fix before I commit your
change. But I'll wait for your permission before doing so.
Reviewed-by: Alex Elder <aelder@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-17 21:05 Carlos Maiolino
2011-10-17 22:39 ` Alex Elder
@ 2011-10-17 22:43 ` Dave Chinner
2011-10-18 1:28 ` Carlos Maiolino
1 sibling, 1 reply; 18+ messages in thread
From: Dave Chinner @ 2011-10-17 22:43 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
On Mon, Oct 17, 2011 at 07:05:28PM -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>
Looks good. One minor thing for consistency, but consider it:
Reviewed-by: Dave Chinner <dchinner@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..9ca6676 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;
> if (!pathlen)
> goto out;
>
> + if (pathlen > MAXPATHLEN) {
> + xfs_emerg(mp, "%s: inode (%lld) symlink length (%d) too long",
> + __func__, (unsigned long long)ip->i_ino, pathlen);
xfs_alert() is generally used for such messages - it's not a fatal
error (yet)....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-17 22:43 ` Dave Chinner
@ 2011-10-18 1:28 ` Carlos Maiolino
0 siblings, 0 replies; 18+ messages in thread
From: Carlos Maiolino @ 2011-10-18 1:28 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Hey, Alex and Dave
I did both changes and sent the path again, so, hopefuly its ok now :-)
Alex, if is there anything else you need to change, please, feel free to
do. I just read your email and since you should not be online now, I did
the changes and submitted the patch again.
The discussion was nice. At least everyone survived :D
Thanks hch and sandeen by the nice discussion about this patch.
--
--Carlos
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] Fix possible memory corruption in xfs_readlink
@ 2011-10-17 15:30 Carlos Maiolino
2011-10-17 14:00 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Carlos Maiolino @ 2011-10-17 15:30 UTC (permalink / raw)
To: xfs; +Cc: Carlos Maiolino
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.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
fs/xfs/xfs_vnodeops.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 51fc429..3bc4fda 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -123,8 +123,13 @@ xfs_readlink(
xfs_ilock(ip, XFS_ILOCK_SHARED);
- 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);
+ }
pathlen = ip->i_d.di_size;
if (!pathlen)
--
1.7.6.2
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-17 15:30 Carlos Maiolino
@ 2011-10-17 14:00 ` Christoph Hellwig
2011-10-17 17:24 ` Eric Sandeen
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2011-10-17 14:00 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: xfs
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] Fix possible memory corruption in xfs_readlink
2011-10-17 14:00 ` Christoph Hellwig
@ 2011-10-17 17:24 ` Eric Sandeen
0 siblings, 0 replies; 18+ messages in thread
From: Eric Sandeen @ 2011-10-17 17:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Carlos Maiolino, xfs
On 10/17/11 9:00 AM, Christoph Hellwig wrote:
> 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);
> }
We could get here via xfs_readlink_by_handle, but that tests
S_ISLNK(dentry->d_inode->i_mode) before calling xfs_readlink.
I'm guessing that we wouldn't get here through normal paths
if the inode in question weren't a symlink, so is there any need
to re-test ip->i_d.di_mode here at runtime?
I'm not sure both ASSERTS necessarily need to be turned into runtime tests.
The 2nd does, clearly, so we don't overflow the buffer. Just not sure about
the first.
> 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);
> }
Since we assign "pathlen" a little later, it might be prettier to use that
variable at that point? It's there, and it's descriptive.
Also, the current xfs_emerg() convention seems to be:
function: problem description
It might we worth following that, i.e.
xfs_emerg(mp, "%s: symlink target in inode %lld too long (%lld)", __func__, ip->i_ino, pathlen)
-Eric
> 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
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2011-11-08 14:38 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-01 14:14 [PATCH] Fix possible memory corruption in xfs_readlink Ben Hutchings
2011-11-02 17:52 ` Alex Elder
2011-11-02 19:45 ` Christoph Hellwig
2011-11-02 20:22 ` Alex Elder
2011-11-07 16:10 ` [PATCH, updated] xfs: " Alex Elder
2011-11-07 16:31 ` Carlos Maiolino
2011-11-08 14:38 ` Christoph Hellwig
-- strict thread matches above, loose matches on Subject: below --
2011-10-18 4:18 [PATCH] " Carlos Maiolino
2011-10-18 6:52 ` Christoph Hellwig
2011-10-18 13:59 ` Alex Elder
2011-10-18 14:25 ` Eric Sandeen
2011-10-17 21:05 Carlos Maiolino
2011-10-17 22:39 ` Alex Elder
2011-10-17 22:43 ` Dave Chinner
2011-10-18 1:28 ` Carlos Maiolino
2011-10-17 15:30 Carlos Maiolino
2011-10-17 14:00 ` Christoph Hellwig
2011-10-17 17:24 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox