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 <[email protected]>
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
On Sat, Jul 24, 2021 at 1:26 AM Colin Ian King <[email protected]> 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 <[email protected]>
> 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
On Sat, 24 Jul 2021 at 09:36, Amir Goldstein <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 1:26 AM Colin Ian King <[email protected]> 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 <[email protected]>
> > 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.
Even simpler: move the release_dentry_name_snapshot to just after lookup.
Commit 89741437981a ("ovl: fix uninitialized pointer read in
ovl_lookup_real_one()") pushed to #overlayfs-next.
Thanks,
Miklos