Re: [syzbot] memory leak in cap_inode_getsecurity

[Miklos Szeredi]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master

On Sun, 31 Jul 2022 at 17:13, syzbot
<syzbot+942d5390db2d9624ced8@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> syzbot has found a reproducer for the following issue on:
>
> HEAD commit:    6a010258447d Merge tag 'for-linus' of git://git.armlinux.o..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=15883fee080000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=2a1dcc1942e30704
> dashboard link: https://syzkaller.appspot.com/bug?extid=942d5390db2d9624ced8
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1568846a080000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10f5e536080000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+942d5390db2d9624ced8@xxxxxxxxxxxxxxxxxxxxxxxxx
>
> executing program
> BUG: memory leak
> unreferenced object 0xffff88810f0ac060 (size 32):
>   comm "syz-executor240", pid 3622, jiffies 4294961303 (age 14.040s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff814c6ecd>] __do_krealloc mm/slab_common.c:1185 [inline]
>     [<ffffffff814c6ecd>] krealloc+0x4d/0xb0 mm/slab_common.c:1218
>     [<ffffffff8162625c>] vfs_getxattr_alloc+0x13c/0x1c0 fs/xattr.c:379
>     [<ffffffff822374b2>] cap_inode_getsecurity+0xb2/0x500 security/commoncap.c:400
>     [<ffffffff8223d88c>] security_inode_getsecurity+0x7c/0xb0 security/security.c:1441
>     [<ffffffff81625a0a>] xattr_getsecurity fs/xattr.c:327 [inline]
>     [<ffffffff81625a0a>] vfs_getxattr+0x22a/0x290 fs/xattr.c:423
>     [<ffffffff81c0ab02>] ovl_xattr_get+0x62/0xa0 fs/overlayfs/inode.c:404
>     [<ffffffff81624742>] __vfs_getxattr+0x72/0xa0 fs/xattr.c:401
>     [<ffffffff82236f52>] cap_inode_need_killpriv+0x22/0x40 security/commoncap.c:301
>     [<ffffffff8223d773>] security_inode_need_killpriv+0x23/0x60 security/security.c:1419
>     [<ffffffff8161074e>] dentry_needs_remove_privs fs/inode.c:1992 [inline]
>     [<ffffffff8161074e>] dentry_needs_remove_privs+0x4e/0xa0 fs/inode.c:1982
>     [<ffffffff815cfead>] do_truncate+0x7d/0x130 fs/open.c:57
>     [<ffffffff815d0169>] vfs_truncate+0x209/0x240 fs/open.c:111
>     [<ffffffff815d0268>] do_sys_truncate.part.0+0xc8/0xe0 fs/open.c:134
>     [<ffffffff815d0303>] do_sys_truncate fs/open.c:128 [inline]
>     [<ffffffff815d0303>] __do_sys_truncate fs/open.c:146 [inline]
>     [<ffffffff815d0303>] __se_sys_truncate fs/open.c:144 [inline]
>     [<ffffffff815d0303>] __x64_sys_truncate+0x33/0x50 fs/open.c:144
>     [<ffffffff845b1955>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>     [<ffffffff845b1955>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>     [<ffffffff84600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>
---
 security/commoncap.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -401,8 +401,8 @@ int cap_inode_getsecurity(struct user_na
 				      &tmpbuf, size, GFP_NOFS);
 	dput(dentry);
 
-	if (ret < 0 || !tmpbuf)
-		return ret;
+	if (ret < 0)
+		goto out_free;
 
 	fs_ns = inode->i_sb->s_user_ns;
 	cap = (struct vfs_cap_data *) tmpbuf;
@@ -412,7 +412,7 @@ int cap_inode_getsecurity(struct user_na
 		nscap = (struct vfs_ns_cap_data *) tmpbuf;
 		root = le32_to_cpu(nscap->rootid);
 	} else {
-		size = -EINVAL;
+		ret = -EINVAL;
 		goto out_free;
 	}
 
@@ -431,7 +431,7 @@ int cap_inode_getsecurity(struct user_na
 				/* v2 -> v3 conversion */
 				nscap = kzalloc(size, GFP_ATOMIC);
 				if (!nscap) {
-					size = -ENOMEM;
+					ret = -ENOMEM;
 					goto out_free;
 				}
 				nsmagic = VFS_CAP_REVISION_3;
@@ -447,11 +447,11 @@ int cap_inode_getsecurity(struct user_na
 			nscap->rootid = cpu_to_le32(mappedroot);
 			*buffer = nscap;
 		}
-		goto out_free;
+		goto success;
 	}
 
 	if (!rootid_owns_currentns(kroot)) {
-		size = -EOVERFLOW;
+		ret = -EOVERFLOW;
 		goto out_free;
 	}
 
@@ -462,7 +462,7 @@ int cap_inode_getsecurity(struct user_na
 			/* v3 -> v2 conversion */
 			cap = kzalloc(size, GFP_ATOMIC);
 			if (!cap) {
-				size = -ENOMEM;
+				ret = -ENOMEM;
 				goto out_free;
 			}
 			magic = VFS_CAP_REVISION_2;
@@ -477,9 +477,11 @@ int cap_inode_getsecurity(struct user_na
 		}
 		*buffer = cap;
 	}
+success:
+	ret = size;
 out_free:
 	kfree(tmpbuf);
-	return size;
+	return ret;
 }
 
 /**