ovl: uninitialized pointer read in ovl_lookup_real_one

* ovl: uninitialized pointer read in ovl_lookup_real_one
@ 2021-07-23 22:26 Colin Ian King
  2021-07-24  7:36 ` Amir Goldstein
  0 siblings, 1 reply; 3+ messages in thread
From: Colin Ian King @ 2021-07-23 22:26 UTC (permalink / raw)
  To: Miklos Szeredi, Amir Goldstein; +Cc: overlayfs, linux-kernel

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.

Colin

^ permalink raw reply	[flat|nested] 3+ messages in thread