Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <737687ee-3449-aa3d-ee29-bd75ca0a18a9@canonical.com>
In-Reply-To: <737687ee-3449-aa3d-ee29-bd75ca0a18a9@canonical.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Sat, 24 Jul 2021 10:36:05 +0300
Message-ID: <CAOQ4uxgbqyyhQ+78j0L+GxkEJ8rOW43X9ann_kMs1098WkNe8Q@mail.gmail.com>
Subject: Re: ovl: uninitialized pointer read in ovl_lookup_real_one
To:     Colin Ian King <colin.king@canonical.com>
Cc:     Miklos Szeredi <miklos@szeredi.hu>,
        overlayfs <linux-unionfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Jul 24, 2021 at 1:26 AM Colin Ian King <colin.king@canonical.com> wrote:
>
> Hi,
>
> Static analysis with Coverity has detected an uninitialized pointer read
> in function ovl_lookup_real_one in fs/overlayfs/export.c
>
> The issue was introduced with the following commit:
>
> commit 3985b70a3e3f58109dc6ae347eafe6e8610be41e
> Author: Amir Goldstein <amir73il@gmail.com>
> Date:   Thu Dec 28 18:36:16 2017 +0200
>
>     ovl: decode connected upper dir file handles
>
> The analysis is as follows:
>
> 365static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> 366                                          struct dentry *real,
> 367                                          const struct ovl_layer *layer)
> 368{
> 369        struct inode *dir = d_inode(connected);
> 370        struct dentry *this, *parent = NULL;
>
>    1. var_decl: Declaring variable name without initializer.
>
> 371        struct name_snapshot name;
> 372        int err;
> 373
> 374        /*
> 375         * Lookup child overlay dentry by real name. The dir mutex
> protects us
> 376         * from racing with overlay rename. If the overlay dentry
> that is above
> 377         * real has already been moved to a parent that is not under the
> 378         * connected overlay dir, we return -ECHILD and restart the
> lookup of
> 379         * connected real path from the top.
> 380         */
> 381        inode_lock_nested(dir, I_MUTEX_PARENT);
> 382        err = -ECHILD;
> 383        parent = dget_parent(real);
>
>    2. Condition ovl_dentry_real_at(connected, layer->idx) != parent,
> taking true branch.
>
> 384        if (ovl_dentry_real_at(connected, layer->idx) != parent)
>
>    3. Jumping to label fail.
>
> 385                goto fail;
> 386
> 387        /*
> 388         * We also need to take a snapshot of real dentry name to
> protect us
> 389         * from racing with underlying layer rename. In this case, we
> don't
> 390         * care about returning ESTALE, only from dereferencing a
> free name
> 391         * pointer because we hold no lock on the real dentry.
> 392         */
> 393        take_dentry_name_snapshot(&name, real);
> 394        this = lookup_one_len(name.name.name, connected, name.name.len);
> 395        err = PTR_ERR(this);
> 396        if (IS_ERR(this)) {
> 397                goto fail;
> 398        } else if (!this || !this->d_inode) {
> 399                dput(this);
> 400                err = -ENOENT;
> 401                goto fail;
> 402        } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> 403                dput(this);
> 404                err = -ESTALE;
> 405                goto fail;
> 406        }
> 407
> 408out:
>
>    Uninitialized pointer read
>    6. uninit_use_in_call: Using uninitialized value name.name.name when
> calling release_dentry_name_snapshot.
>
> 409        release_dentry_name_snapshot(&name);
> 410        dput(parent);
> 411        inode_unlock(dir);
> 412        return this;
> 413
> 414fail:
>
>    4. Condition ___ratelimit(&_rs, <anonymous>), taking false branch
> .
> 415        pr_warn_ratelimited("failed to lookup one by real (%pd2,
> layer=%d, connected=%pd2, err=%i)\n",
> 416                            real, layer->idx, connected, err);
> 417        this = ERR_PTR(err);
>
>    5. Jumping to label out.
>
> 418        goto out;
> 419}
>
> The error exit path on line 395 ends up with an uninitialized structure
> name being passed to function release_dentry_name_snapshot() on line 409
> and this accesses the pointer name.name.name, see /fs/dcache.c as follows:
>
> 303void release_dentry_name_snapshot(struct name_snapshot *name)
> 304{
>
>    1. read_value: Reading value name->name.name.
>    2. Condition !!(name->name.name != name->inline_name), taking true
> branch.
>
> 305        if (unlikely(name->name.name != name->inline_name)) {
> 306                struct external_name *p;
>
>    3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
>
>
> I suspect name should be initialized in line 371, e.g. name = { } and a
> null name check should be performed on line 409 before calling
> release_dentry_name_snapshot, but this seems a bit message as a fix.
>

Thanks for the report.
A simpler fix is to move take_dentry_name_snapshot() to top of the
function before goto fail.

Thanks,
Amir.

> Colin