| Message ID | 20221116091255.84576-1-arefev@swemel.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | namespace: Added pointer check in copy_mnt_ns() | expand |
On Wed, Nov 16, 2022 at 12:12:55PM +0300, Denis Arefev wrote: > Return value of a function 'next_mnt' is dereferenced at > namespace.c:3377 without checking for null, > but it is usually checked for this function > > Found by Linux Verification Center (linuxtesting.org) with SVACE. You need to do human analysis, not just send the results from a bot. What conditions can lead to this function returning NULL? Do we already know those conditions can or cannot hold?
Hi Denis,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Denis-Arefev/namespace-Added-pointer-check-in-copy_mnt_ns/20221116-171424
patch link: https://lore.kernel.org/r/20221116091255.84576-1-arefev%40swemel.ru
patch subject: [PATCH] namespace: Added pointer check in copy_mnt_ns()
config: x86_64-randconfig-m001
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
New smatch warnings:
fs/namespace.c:3518 copy_mnt_ns() error: we previously assumed 'p' could be null (see line 3518)
Old smatch warnings:
fs/namespace.c:4059 mount_setattr_prepare() error: uninitialized symbol 'err'.
vim +/p +3518 fs/namespace.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 3494 /*
^1da177e4c3f41 Linus Torvalds 2005-04-16 3495 * Second pass: switch the tsk->fs->* elements and mark new vfsmounts
^1da177e4c3f41 Linus Torvalds 2005-04-16 3496 * as belonging to new namespace. We have already acquired a private
^1da177e4c3f41 Linus Torvalds 2005-04-16 3497 * fs_struct, so tsk->fs->lock is not needed.
^1da177e4c3f41 Linus Torvalds 2005-04-16 3498 */
909b0a88ef2dc8 Al Viro 2011-11-25 3499 p = old;
cb338d06e9716c Al Viro 2011-11-24 3500 q = new;
^1da177e4c3f41 Linus Torvalds 2005-04-16 3501 while (p) {
143c8c91cee7ef Al Viro 2011-11-25 3502 q->mnt_ns = new_ns;
d29216842a85c7 Eric W. Biederman 2016-09-28 3503 new_ns->mounts++;
9559f68915024e Al Viro 2013-09-28 3504 if (new_fs) {
9559f68915024e Al Viro 2013-09-28 3505 if (&p->mnt == new_fs->root.mnt) {
9559f68915024e Al Viro 2013-09-28 3506 new_fs->root.mnt = mntget(&q->mnt);
315fc83e56c699 Al Viro 2011-11-24 3507 rootmnt = &p->mnt;
315fc83e56c699 Al Viro 2011-11-24 3508 }
9559f68915024e Al Viro 2013-09-28 3509 if (&p->mnt == new_fs->pwd.mnt) {
9559f68915024e Al Viro 2013-09-28 3510 new_fs->pwd.mnt = mntget(&q->mnt);
315fc83e56c699 Al Viro 2011-11-24 3511 pwdmnt = &p->mnt;
^1da177e4c3f41 Linus Torvalds 2005-04-16 3512 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 3513 }
909b0a88ef2dc8 Al Viro 2011-11-25 3514 p = next_mnt(p, old);
909b0a88ef2dc8 Al Viro 2011-11-25 3515 q = next_mnt(q, new);
ff6985ba29d455 Denis Arefev 2022-11-16 3516 if (!q || !p)
4ce5d2b1a8fde8 Eric W. Biederman 2013-03-30 3517 break;
ff6985ba29d455 Denis Arefev 2022-11-16 @3518 while (!p && (p->mnt.mnt_root != q->mnt.mnt_root))
^
The ! needs to be removed.
4ce5d2b1a8fde8 Eric W. Biederman 2013-03-30 3519 p = next_mnt(p, old);
^1da177e4c3f41 Linus Torvalds 2005-04-16 3520 }
328e6d9014636a Al Viro 2013-03-16 3521 namespace_unlock();
^1da177e4c3f41 Linus Torvalds 2005-04-16 3522
^1da177e4c3f41 Linus Torvalds 2005-04-16 3523 if (rootmnt)
f03c65993b98ee Al Viro 2011-01-14 3524 mntput(rootmnt);
^1da177e4c3f41 Linus Torvalds 2005-04-16 3525 if (pwdmnt)
f03c65993b98ee Al Viro 2011-01-14 3526 mntput(pwdmnt);
^1da177e4c3f41 Linus Torvalds 2005-04-16 3527
741a2951306061 JANAK DESAI 2006-02-07 3528 return new_ns;
^1da177e4c3f41 Linus Torvalds 2005-04-16 3529 }
diff --git a/fs/namespace.c b/fs/namespace.c index cebaa3e81794..06472a110257 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3348,9 +3348,9 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns, } p = next_mnt(p, old); q = next_mnt(q, new); - if (!q) + if (!q || !p) break; - while (p->mnt.mnt_root != q->mnt.mnt_root) + while (!p && (p->mnt.mnt_root != q->mnt.mnt_root)) p = next_mnt(p, old); } namespace_unlock();
Return value of a function 'next_mnt' is dereferenced at namespace.c:3377 without checking for null, but it is usually checked for this function Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Denis Arefev <arefev@swemel.ru> --- fs/namespace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)