xfs
[Top] [All Lists]

Re: [PATCH] Fix possible memory corruption in xfs_readlink

To: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Subject: Re: [PATCH] Fix possible memory corruption in xfs_readlink
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 17 Oct 2011 10:00:30 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1318865412-4655-1-git-send-email-cmaiolino@xxxxxxxxxx>
References: <1318865412-4655-1-git-send-email-cmaiolino@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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)

<Prev in Thread] Current Thread [Next in Thread>