linux-kernel - Re: [PATCH] reiserfs: propagate errors from fill_with

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG48ez1eD=govPAZcO0C3Q8VCS0SqqsZFNpV72OvvxouhZt9eQ@mail.gmail.com>
Date:   Mon, 3 Sep 2018 18:04:08 +0200
From:   Jann Horn <jannh@...gle.com>
To:     reiserfs-devel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     kernel list <linux-kernel@...r.kernel.org>,
        Jeff Mahoney <jeffm@...e.com>,
        Eric Biggers <ebiggers@...gle.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] reiserfs: propagate errors from fill_with_dentries properly

On Thu, Aug 2, 2018 at 6:33 PM Jann Horn <jannh@...gle.com> wrote:
>
> fill_with_dentries() failed to propagate errors up to
> reiserfs_for_each_xattr() properly. Plumb them through.
>
> Note that reiserfs_for_each_xattr() is only used by
> reiserfs_delete_xattrs() and reiserfs_chown_xattrs().
> The result of reiserfs_delete_xattrs() is discarded anyway, the only
> difference there is whether a warning is printed to dmesg.
> The result of reiserfs_chown_xattrs() does matter because it can block
> chowning of the file to which the xattrs belong; but either way, the
> resulting state can have misaligned ownership, so my patch doesn't improve
> things greatly.
>
> Credit for making me look at this code goes to Al Viro, who pointed
> out that the ->actor calling convention is suboptimal and should be
> changed.
>
> Signed-off-by: Jann Horn <jannh@...gle.com>

Ping.

> ---
> I have tested this by manually patching error injection into
> fill_with_dentries().
>
> Opinions? Is this a sensible change?
>
> Because the changes in this patch are more superficial than the changes
> in the other one, I split this out so that the security patch is a
> clean change that obviously belongs in stable and can hopefully go in
> quickly.
>
> After the cases I can see where errors are returned improperly are
> cleaned up, I plan to change the calling convention for ->actor as
> suggested by Al Viro.
>
>  fs/reiserfs/xattr.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
> index ff94fad477e4..ae4a28410dbd 100644
> --- a/fs/reiserfs/xattr.c
> +++ b/fs/reiserfs/xattr.c
> @@ -185,6 +185,7 @@ struct reiserfs_dentry_buf {
>         struct dir_context ctx;
>         struct dentry *xadir;
>         int count;
> +       int err;
>         struct dentry *dentries[8];
>  };
>
> @@ -207,6 +208,7 @@ fill_with_dentries(struct dir_context *ctx, const char *name, int namelen,
>
>         dentry = lookup_one_len(name, dbuf->xadir, namelen);
>         if (IS_ERR(dentry)) {
> +               dbuf->err = PTR_ERR(dentry);
>                 return PTR_ERR(dentry);
>         } else if (d_really_is_negative(dentry)) {
>                 /* A directory entry exists, but no file? */
> @@ -215,6 +217,7 @@ fill_with_dentries(struct dir_context *ctx, const char *name, int namelen,
>                                "not found for file %pd.\n",
>                                dentry, dbuf->xadir);
>                 dput(dentry);
> +               dbuf->err = -EIO;
>                 return -EIO;
>         }
>
> @@ -262,6 +265,10 @@ static int reiserfs_for_each_xattr(struct inode *inode,
>                 err = reiserfs_readdir_inode(d_inode(dir), &buf.ctx);
>                 if (err)
>                         break;
> +               if (buf.err) {
> +                       err = buf.err;
> +                       break;
> +               }
>                 if (!buf.count)
>                         break;
>                 for (i = 0; !err && i < buf.count && buf.dentries[i]; i++) {
> --
> 2.18.0.597.ga71716f1ad-goog
>