diff mbox series

namespace: Added pointer check in copy_mnt_ns()

Message ID 20221116091255.84576-1-arefev@swemel.ru
State New
Headers
Series namespace: Added pointer check in copy_mnt_ns() |

Commit Message

Denis Arefev Nov. 16, 2022, 9:12 a.m. UTC 
  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(-)

Comments

Matthew Wilcox Nov. 16, 2022, 6:16 p.m. UTC | #1 
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?
Dan Carpenter Nov. 17, 2022, 1:34 p.m. UTC | #2 
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 mbox series

Patch

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();