lkml.org 
[lkml]   [2021]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Subjectovl: uninitialized pointer read in ovl_lookup_real_one
Date
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

\
 
 \ /
  Last update: 2021-07-24 00:27    [W:0.052 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site