Messages in this thread | | | From | Colin Ian King <> | Subject | ovl: uninitialized pointer read in ovl_lookup_real_one | Date | Fri, 23 Jul 2021 23:26:34 +0100 |
| |
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
| |