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.
Colin
