On Sat, Jul 24, 2021 at 1:26 AM Colin Ian King <colin.king@xxxxxxxxxxxxx> 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@xxxxxxxxx> > 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