* [PATCH] xfs: Fix possible memory corruption in xfs_readlink @ 2011-10-17 1:26 Carlos Maiolino 2011-10-16 23:41 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: Carlos Maiolino @ 2011-10-17 1:26 UTC (permalink / raw) To: xfs; +Cc: Carlos Maiolino This patch fix 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 | 18 ++++++++++++++++-- 1 files changed, 16 insertions(+), 2 deletions(-) diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c index 51fc429..3f4fbd5 100644 --- a/fs/xfs/xfs_vnodeops.c +++ b/fs/xfs/xfs_vnodeops.c @@ -123,8 +123,22 @@ xfs_readlink( xfs_ilock(ip, XFS_ILOCK_SHARED); - ASSERT(S_ISLNK(ip->i_d.di_mode)); - ASSERT(ip->i_d.di_size <= MAXPATHLEN); + if (unlikely(!(S_ISLNK(ip->i_d.di_mode))) || + unlikely(!(ip->i_d.di_size <= MAXPATHLEN ))){ + + XFS_CORRUPTION_ERROR("xfs_readlink", + XFS_ERRLEVEL_HIGH, mp, ip); + +#ifdef DEBUG + xfs_emerg(mp, "inode (%lld), link too long or not a link." + (unsigned long long)ip->i_no); + + ASSERT(S_ISLNK(ip->i_d.di_mode)); + ASSERT(ip->i_d.di_size <= MAXPATHLEN); +#endif + + 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] 4+ messages in thread
* Re: [PATCH] xfs: Fix possible memory corruption in xfs_readlink 2011-10-17 1:26 [PATCH] xfs: Fix possible memory corruption in xfs_readlink Carlos Maiolino @ 2011-10-16 23:41 ` Dave Chinner 2011-10-17 14:52 ` Carlos Maiolino 0 siblings, 1 reply; 4+ messages in thread From: Dave Chinner @ 2011-10-16 23:41 UTC (permalink / raw) To: Carlos Maiolino; +Cc: xfs On Sun, Oct 16, 2011 at 11:26:34PM -0200, Carlos Maiolino wrote: > This patch fix 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> Well found, Carlos. A few comments on the fix below. > --- > fs/xfs/xfs_vnodeops.c | 18 ++++++++++++++++-- > 1 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c > index 51fc429..3f4fbd5 100644 > --- a/fs/xfs/xfs_vnodeops.c > +++ b/fs/xfs/xfs_vnodeops.c > @@ -123,8 +123,22 @@ xfs_readlink( > > xfs_ilock(ip, XFS_ILOCK_SHARED); > > - ASSERT(S_ISLNK(ip->i_d.di_mode)); > - ASSERT(ip->i_d.di_size <= MAXPATHLEN); > + if (unlikely(!(S_ISLNK(ip->i_d.di_mode))) || > + unlikely(!(ip->i_d.di_size <= MAXPATHLEN ))){ No need for the unlikely - the hardware generally handles branch prediction better than we can. ;) > + > + XFS_CORRUPTION_ERROR("xfs_readlink", > + XFS_ERRLEVEL_HIGH, mp, ip); At first glance this looks like the "right" error reporting macro to use, but it realy isn't: XFS_CORRUPTION_ERROR() is for reporting details of the on-disk structure that is corrupted. Basically it is used to dump the first chunk of the disk buffer/structure that the error was found in, but we only have in-memory state here so there's nothing to dump. Yes, we can dump the first 16 bytes of ip, but that just gets us a couple of memory pointers which has nothing to do with the corruption just detected. > +#ifdef DEBUG > + xfs_emerg(mp, "inode (%lld), link too long or not a link." > + (unsigned long long)ip->i_no); ^^^^ i_ino > + > + ASSERT(S_ISLNK(ip->i_d.di_mode)); > + ASSERT(ip->i_d.di_size <= MAXPATHLEN); Not much point putting a second print for only debug kernels - if it's useful information about the corruption (e.g. the inode number), then it should be emitted for non-debug kernels, too. Yeah, I know you probably copied it from somewhere else (like xfs_imap_to_bmap()), but for all new detected corruptions having some indication on normal production machines of where the corruption was detected is very important. IOWs, I'd simply be replacing the ASSERT()s with something like this: if (!(S_ISLNK(ip->i_d.di_mode) && ip->i_d.di_size <= MAXPATHLEN)) { xfs_alert(mp, "inode %lld: link too long (%) or not a link." (unsigned long long)ip->i_ino, i_d.di_size); ASSERT(0); return XFS_ERROR(EFSCORRUPTED); } It will still assert fail on a debug kernel, with enough information to tell us exactly which condition failed. And it will detect and return an error on a non-debug kernel, too. 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] 4+ messages in thread
* [PATCH] xfs: Fix possible memory corruption in xfs_readlink 2011-10-16 23:41 ` Dave Chinner @ 2011-10-17 14:52 ` Carlos Maiolino 2011-10-17 12:24 ` Carlos Maiolino 0 siblings, 1 reply; 4+ messages in thread From: Carlos Maiolino @ 2011-10-17 14:52 UTC (permalink / raw) To: xfs; +Cc: Carlos Maiolino This patch fix 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..c9721d7 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_no); + 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] 4+ messages in thread
* Re: [PATCH] xfs: Fix possible memory corruption in xfs_readlink 2011-10-17 14:52 ` Carlos Maiolino @ 2011-10-17 12:24 ` Carlos Maiolino 0 siblings, 0 replies; 4+ messages in thread From: Carlos Maiolino @ 2011-10-17 12:24 UTC (permalink / raw) To: xfs Hi, On Mon, Oct 17, 2011 at 12:52:48PM -0200, Carlos Maiolino wrote: > This patch fix 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, disconsider this last patch. I did two mistakes here and I'll be sending a fixed version of it. My apologies for this -- --Carlos _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-10-17 12:25 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-17 1:26 [PATCH] xfs: Fix possible memory corruption in xfs_readlink Carlos Maiolino 2011-10-16 23:41 ` Dave Chinner 2011-10-17 14:52 ` Carlos Maiolino 2011-10-17 12:24 ` Carlos Maiolino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox