KASAN: use-after-free Read in tomoyo_realpath_from_path

KASAN: use-after-free Read in tomoyo_realpath_from_path

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    788a0249 Merge tag 'arc-5.2-rc4' of git://git.kernel.org/p..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=179848d4a00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=60564cb52ab29d5b
dashboard link: https://syzkaller.appspot.com/bug?extid=0341f6a4d729d4e0acf1
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13ac35baa00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: use-after-free in tomoyo_get_socket_name  
security/tomoyo/realpath.c:238 [inline]
BUG: KASAN: use-after-free in tomoyo_realpath_from_path+0x722/0x7a0  
security/tomoyo/realpath.c:284
Read of size 2 at addr ffff8880a91276d0 by task syz-executor.3/17397

CPU: 0 PID: 17397 Comm: syz-executor.3 Not tainted 5.2.0-rc3+ #12
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x172/0x1f0 lib/dump_stack.c:113
  print_address_description.cold+0x7c/0x20d mm/kasan/report.c:188
  __kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
  kasan_report+0x12/0x20 mm/kasan/common.c:614
  __asan_report_load2_noabort+0x14/0x20 mm/kasan/generic_report.c:130
  tomoyo_get_socket_name security/tomoyo/realpath.c:238 [inline]
  tomoyo_realpath_from_path+0x722/0x7a0 security/tomoyo/realpath.c:284
  tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
  tomoyo_check_open_permission+0x2a8/0x3f0 security/tomoyo/file.c:771
  tomoyo_file_open security/tomoyo/tomoyo.c:319 [inline]
  tomoyo_file_open+0xa9/0xd0 security/tomoyo/tomoyo.c:314
  security_file_open+0x71/0x300 security/security.c:1454
  do_dentry_open+0x373/0x1250 fs/open.c:765
  vfs_open+0xa0/0xd0 fs/open.c:887
  do_last fs/namei.c:3416 [inline]
  path_openat+0x10e9/0x46d0 fs/namei.c:3533
  do_filp_open+0x1a1/0x280 fs/namei.c:3563
  do_sys_open+0x3fe/0x5d0 fs/open.c:1070
  __do_sys_open fs/open.c:1088 [inline]
  __se_sys_open fs/open.c:1083 [inline]
  __x64_sys_open+0x7e/0xc0 fs/open.c:1083
  do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x413161
Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48  
83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f 05 <48> 8b 3c 24 48  
89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d 01
RSP: 002b:00007f65230f8bb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 0000000000413161
RDX: fffffffffffffffa RSI: 0000000000000000 RDI: 00007f65230f8bd0
RBP: 000000000075c060 R08: 0000000000000050 R09: 000000000000000f
R10: 0000000000000004 R11: 0000000000000293 R12: 00007f65230f96d4
R13: 00000000004c83f6 R14: 00000000004dea40 R15: 00000000ffffffff

Allocated by task 17373:
  save_stack+0x23/0x90 mm/kasan/common.c:71
  set_track mm/kasan/common.c:79 [inline]
  __kasan_kmalloc mm/kasan/common.c:489 [inline]
  __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:462
  kasan_kmalloc+0x9/0x10 mm/kasan/common.c:503
  __do_kmalloc mm/slab.c:3660 [inline]
  __kmalloc+0x15c/0x740 mm/slab.c:3669
  kmalloc include/linux/slab.h:552 [inline]
  sk_prot_alloc+0x19c/0x2e0 net/core/sock.c:1602
  sk_alloc+0x39/0xf70 net/core/sock.c:1656
  base_sock_create drivers/isdn/mISDN/socket.c:758 [inline]
  mISDN_sock_create+0xb4/0x3a0 drivers/isdn/mISDN/socket.c:780
  __sock_create+0x3d8/0x730 net/socket.c:1424
  sock_create net/socket.c:1475 [inline]
  __sys_socket+0x103/0x220 net/socket.c:1517
  __do_sys_socket net/socket.c:1526 [inline]
  __se_sys_socket net/socket.c:1524 [inline]
  __x64_sys_socket+0x73/0xb0 net/socket.c:1524
  do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 17371:
  save_stack+0x23/0x90 mm/kasan/common.c:71
  set_track mm/kasan/common.c:79 [inline]
  __kasan_slab_free+0x102/0x150 mm/kasan/common.c:451
  kasan_slab_free+0xe/0x10 mm/kasan/common.c:459
  __cache_free mm/slab.c:3432 [inline]
  kfree+0xcf/0x220 mm/slab.c:3755
  sk_prot_free net/core/sock.c:1639 [inline]
  __sk_destruct+0x4f7/0x6e0 net/core/sock.c:1725
  sk_destruct+0x7b/0x90 net/core/sock.c:1733
  __sk_free+0xce/0x300 net/core/sock.c:1744
  sk_free+0x42/0x50 net/core/sock.c:1755
  sock_put include/net/sock.h:1723 [inline]
  base_sock_release+0x269/0x279 drivers/isdn/mISDN/socket.c:628
  __sock_release+0xce/0x2a0 net/socket.c:601
  sock_close+0x1b/0x30 net/socket.c:1273
  __fput+0x2ff/0x890 fs/file_table.c:280
  ____fput+0x16/0x20 fs/file_table.c:313
  task_work_run+0x145/0x1c0 kernel/task_work.c:113
  tracehook_notify_resume include/linux/tracehook.h:185 [inline]
  exit_to_usermode_loop+0x273/0x2c0 arch/x86/entry/common.c:168
  prepare_exit_to_usermode arch/x86/entry/common.c:199 [inline]
  syscall_return_slowpath arch/x86/entry/common.c:279 [inline]
  do_syscall_64+0x58e/0x680 arch/x86/entry/common.c:304
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

The buggy address belongs to the object at ffff8880a91276c0
  which belongs to the cache kmalloc-2k of size 2048
The buggy address is located 16 bytes inside of
  2048-byte region [ffff8880a91276c0, ffff8880a9127ec0)
The buggy address belongs to the page:
page:ffffea0002a44980 refcount:1 mapcount:0 mapping:ffff8880aa400c40  
index:0x0 compound_mapcount: 0
flags: 0x1fffc0000010200(slab|head)
raw: 01fffc0000010200 ffffea00022c9f88 ffffea0002234408 ffff8880aa400c40
raw: 0000000000000000 ffff8880a91265c0 0000000100000003 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
  ffff8880a9127580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ffff8880a9127600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> ffff8880a9127680: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
                                                  ^
  ffff8880a9127700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ffff8880a9127780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: KASAN: use-after-free Read in tomoyo_realpath_from_path

[Tetsuo Handa]

Hello, Al.

syzbot found that SOCKET_I(d_backing_inode("struct path"->dentry))->sk
was already kfree()d when trying to calculate pathname for open().
"struct path"->dentry should remain valid but portion of memory
reachable via inode already became invalid. What should we do?

On 2019/06/06 3:42, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    788a0249 Merge tag 'arc-5.2-rc4' of git://git.kernel.org/p..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=179848d4a00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=60564cb52ab29d5b
> dashboard link: https://syzkaller.appspot.com/bug?extid=0341f6a4d729d4e0acf1
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13ac35baa00000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com
> 
> ==================================================================
> BUG: KASAN: use-after-free in tomoyo_get_socket_name security/tomoyo/realpath.c:238 [inline]
> BUG: KASAN: use-after-free in tomoyo_realpath_from_path+0x722/0x7a0 security/tomoyo/realpath.c:284
> Read of size 2 at addr ffff8880a91276d0 by task syz-executor.3/17397
> 
> CPU: 0 PID: 17397 Comm: syz-executor.3 Not tainted 5.2.0-rc3+ #12
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x172/0x1f0 lib/dump_stack.c:113
>  print_address_description.cold+0x7c/0x20d mm/kasan/report.c:188
>  __kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
>  kasan_report+0x12/0x20 mm/kasan/common.c:614
>  __asan_report_load2_noabort+0x14/0x20 mm/kasan/generic_report.c:130
>  tomoyo_get_socket_name security/tomoyo/realpath.c:238 [inline]
>  tomoyo_realpath_from_path+0x722/0x7a0 security/tomoyo/realpath.c:284
>  tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
>  tomoyo_check_open_permission+0x2a8/0x3f0 security/tomoyo/file.c:771
>  tomoyo_file_open security/tomoyo/tomoyo.c:319 [inline]
>  tomoyo_file_open+0xa9/0xd0 security/tomoyo/tomoyo.c:314
>  security_file_open+0x71/0x300 security/security.c:1454
>  do_dentry_open+0x373/0x1250 fs/open.c:765
>  vfs_open+0xa0/0xd0 fs/open.c:887
>  do_last fs/namei.c:3416 [inline]
>  path_openat+0x10e9/0x46d0 fs/namei.c:3533
>  do_filp_open+0x1a1/0x280 fs/namei.c:3563
>  do_sys_open+0x3fe/0x5d0 fs/open.c:1070
>  __do_sys_open fs/open.c:1088 [inline]
>  __se_sys_open fs/open.c:1083 [inline]
>  __x64_sys_open+0x7e/0xc0 fs/open.c:1083
>  do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x413161
> Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48 83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f 05 <48> 8b 3c 24 48 89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d 01
> RSP: 002b:00007f65230f8bb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
> RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 0000000000413161
> RDX: fffffffffffffffa RSI: 0000000000000000 RDI: 00007f65230f8bd0
> RBP: 000000000075c060 R08: 0000000000000050 R09: 000000000000000f
> R10: 0000000000000004 R11: 0000000000000293 R12: 00007f65230f96d4
> R13: 00000000004c83f6 R14: 00000000004dea40 R15: 00000000ffffffff
> 
> Allocated by task 17373:
>  save_stack+0x23/0x90 mm/kasan/common.c:71
>  set_track mm/kasan/common.c:79 [inline]
>  __kasan_kmalloc mm/kasan/common.c:489 [inline]
>  __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:462
>  kasan_kmalloc+0x9/0x10 mm/kasan/common.c:503
>  __do_kmalloc mm/slab.c:3660 [inline]
>  __kmalloc+0x15c/0x740 mm/slab.c:3669
>  kmalloc include/linux/slab.h:552 [inline]
>  sk_prot_alloc+0x19c/0x2e0 net/core/sock.c:1602
>  sk_alloc+0x39/0xf70 net/core/sock.c:1656
>  base_sock_create drivers/isdn/mISDN/socket.c:758 [inline]
>  mISDN_sock_create+0xb4/0x3a0 drivers/isdn/mISDN/socket.c:780
>  __sock_create+0x3d8/0x730 net/socket.c:1424
>  sock_create net/socket.c:1475 [inline]
>  __sys_socket+0x103/0x220 net/socket.c:1517
>  __do_sys_socket net/socket.c:1526 [inline]
>  __se_sys_socket net/socket.c:1524 [inline]
>  __x64_sys_socket+0x73/0xb0 net/socket.c:1524
>  do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Freed by task 17371:
>  save_stack+0x23/0x90 mm/kasan/common.c:71
>  set_track mm/kasan/common.c:79 [inline]
>  __kasan_slab_free+0x102/0x150 mm/kasan/common.c:451
>  kasan_slab_free+0xe/0x10 mm/kasan/common.c:459
>  __cache_free mm/slab.c:3432 [inline]
>  kfree+0xcf/0x220 mm/slab.c:3755
>  sk_prot_free net/core/sock.c:1639 [inline]
>  __sk_destruct+0x4f7/0x6e0 net/core/sock.c:1725
>  sk_destruct+0x7b/0x90 net/core/sock.c:1733
>  __sk_free+0xce/0x300 net/core/sock.c:1744
>  sk_free+0x42/0x50 net/core/sock.c:1755
>  sock_put include/net/sock.h:1723 [inline]
>  base_sock_release+0x269/0x279 drivers/isdn/mISDN/socket.c:628
>  __sock_release+0xce/0x2a0 net/socket.c:601
>  sock_close+0x1b/0x30 net/socket.c:1273
>  __fput+0x2ff/0x890 fs/file_table.c:280
>  ____fput+0x16/0x20 fs/file_table.c:313
>  task_work_run+0x145/0x1c0 kernel/task_work.c:113
>  tracehook_notify_resume include/linux/tracehook.h:185 [inline]
>  exit_to_usermode_loop+0x273/0x2c0 arch/x86/entry/common.c:168
>  prepare_exit_to_usermode arch/x86/entry/common.c:199 [inline]
>  syscall_return_slowpath arch/x86/entry/common.c:279 [inline]
>  do_syscall_64+0x58e/0x680 arch/x86/entry/common.c:304
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> The buggy address belongs to the object at ffff8880a91276c0
>  which belongs to the cache kmalloc-2k of size 2048
> The buggy address is located 16 bytes inside of
>  2048-byte region [ffff8880a91276c0, ffff8880a9127ec0)
> The buggy address belongs to the page:
> page:ffffea0002a44980 refcount:1 mapcount:0 mapping:ffff8880aa400c40 index:0x0 compound_mapcount: 0
> flags: 0x1fffc0000010200(slab|head)
> raw: 01fffc0000010200 ffffea00022c9f88 ffffea0002234408 ffff8880aa400c40
> raw: 0000000000000000 ffff8880a91265c0 0000000100000003 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  ffff8880a9127580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff8880a9127600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>> ffff8880a9127680: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>                                                  ^
>  ffff8880a9127700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff8880a9127780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================

Re: KASAN: use-after-free Read in tomoyo_realpath_from_path

[Tetsuo Handa]

Here is a reproducer.

The problem is that TOMOYO is accessing already freed socket from security_file_open()
which later fails with -ENXIO (because we can't get file descriptor of sockets via
/proc/pid/fd/n interface), and the file descriptor is getting released before
security_file_open() completes because we do not raise "struct file"->f_count of
the file which is accessible via /proc/pid/fd/n interface. We can avoid this problem
if we can avoid calling security_file_open() which after all fails with -ENXIO.
How should we handle this race? Let LSM modules check if security_file_open() was
called on a socket?

----------------------------------------
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..995ffcb37128 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -765,6 +765,12 @@ static int do_dentry_open(struct file *f,
 	error = security_file_open(f);
 	if (error)
 		goto cleanup_all;
+	if (!strcmp(current->comm, "a.out") &&
+	    f->f_path.dentry->d_sb->s_magic == SOCKFS_MAGIC) {
+		printk("Start open(socket) delay\n");
+		schedule_timeout_killable(HZ * 5);
+		printk("End open(socket) delay\n");
+	}
 
 	error = break_lease(locks_inode(f), f->f_flags);
 	if (error)
----------------------------------------

----------------------------------------
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/socket.h>

int main(int argc, char *argv[])
{
	pid_t pid = getpid();
	int fd = socket(AF_ISDN, SOCK_RAW, 0);
	char buffer[128] = { };
	if (fork() == 0) {
		close(fd);
		snprintf(buffer, sizeof(buffer) - 1, "/proc/%u/fd/%u", pid, fd);
		open(buffer, 3);
		_exit(0);
	}
	sleep(2);
	close(fd);
	return 0;
}
----------------------------------------

----------------------------------------
getpid()                                = 32504
socket(AF_ISDN, SOCK_RAW, 0)            = 3
clone(strace: Process 32505 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7efea30dda10) = 32505
[pid 32504] rt_sigprocmask(SIG_BLOCK, [CHLD],  <unfinished ...>
[pid 32505] close(3 <unfinished ...>
[pid 32504] <... rt_sigprocmask resumed> [], 8) = 0
[pid 32505] <... close resumed> )       = 0
[pid 32504] rt_sigaction(SIGCHLD, NULL, {SIG_DFL, [], 0}, 8) = 0
[pid 32505] open("/proc/32504/fd/3", O_ACCMODE <unfinished ...>
[pid 32504] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[pid 32504] nanosleep({2, 0}, 0x7ffd3c608150) = 0
[pid 32504] close(3)                    = 0
[pid 32504] exit_group(0)               = ?
[pid 32504] +++ exited with 0 +++
<... open resumed> )                    = -1 ENXIO (No such device or address)
exit_group(0)                           = ?
----------------------------------------

----------------------------------------
[   95.109628] Start open(socket) delay
[   97.113150] base_sock_release(00000000506a3239) sk=00000000016d0ceb
[  100.142235] End open(socket) delay
----------------------------------------

Re: KASAN: use-after-free Read in tomoyo_realpath_from_path

[Tetsuo Handa]

Tetsuo Handa wrote:
> The problem is that TOMOYO is accessing already freed socket from security_file_open()
> which later fails with -ENXIO (because we can't get file descriptor of sockets via
> /proc/pid/fd/n interface), and the file descriptor is getting released before
> security_file_open() completes because we do not raise "struct file"->f_count of
> the file which is accessible via /proc/pid/fd/n interface. We can avoid this problem
> if we can avoid calling security_file_open() which after all fails with -ENXIO.
> How should we handle this race? Let LSM modules check if security_file_open() was
> called on a socket?

Well, just refusing security_file_open() is not sufficient, for open(O_PATH) allows installing
file descriptor where SOCKET_I(inode)->sk can change at any moment, and TOMOYO cannot tell
whether it is safe to access SOCKET_I(inode)->sk from security_inode_getattr().

But refusing open(O_PATH) as well might break userspace programs. Oh, no...

----------------------------------------
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..ea69668e2cd8 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -728,6 +728,16 @@ static int do_dentry_open(struct file *f,
 	/* Ensure that we skip any errors that predate opening of the file */
 	f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
 
+	/*
+	 * Sockets must not be opened via /proc/pid/fd/n, even with O_PATH,
+	 * for SOCKET_I(inode)->sk can be kfree()d at any moment after a file
+	 * descriptor obtained by opening /proc/pid/fd/n was installed.
+	 */
+	if (unlikely(S_ISSOCK(inode->i_mode))) {
+		error = (f->f_flags & O_PATH) ? -ENOENT : -ENXIO;
+		goto cleanup_file;
+	}
+
 	if (unlikely(f->f_flags & O_PATH)) {
 		f->f_mode = FMODE_PATH | FMODE_OPENED;
 		f->f_op = &empty_fops;
----------------------------------------

----------------------------------------
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/socket.h>

int main(int argc, char *argv[])
{
        pid_t pid = getpid();
        int fd = socket(AF_INET, SOCK_STREAM, 0);
        char buffer[128] = { };
        if (fork() == 0) {
                struct stat buf = { };
                close(fd);
                snprintf(buffer, sizeof(buffer) - 1, "/proc/%u/fd/%u", pid, fd);
                fd = open(buffer, __O_PATH);
                sleep(5);
                fstat(fd, &buf);
                _exit(0);
        }
        sleep(2);
        close(fd);
        return 0;
}
----------------------------------------

[PATCH] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

syzbot is reporting that use of SOCKET_I()->sk from open() can result in
use after free problem [1], for socket's inode is still reachable via
/proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.

But there is no point with calling security_file_open() on sockets
because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.

There is some point with calling security_inode_getattr() on sockets
because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
are valid. If we want to access "struct sock"->sk_{family,type,protocol}
fields, we will need to use security_socket_post_create() hook and
security_inode_free() hook in order to remember these fields because
security_sk_free() hook is called before the inode is destructed. But
since information which can be protected by checking
security_inode_getattr() on sockets is trivial, let's not be bothered by
"struct inode"->i_security management.

There is point with calling security_file_ioctl() on sockets. Since
ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
on sockets should remain safe.

[1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
---
 security/tomoyo/tomoyo.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..9661b86 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
  */
 static int tomoyo_inode_getattr(const struct path *path)
 {
+	/* It is not safe to call tomoyo_get_socket_name(). */
+	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
+		return 0;
 	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
 }
 
@@ -316,6 +319,10 @@ static int tomoyo_file_open(struct file *f)
 	/* Don't check read permission here if called from do_execve(). */
 	if (current->in_execve)
 		return 0;
+	/* Sockets can't be opened by open(). */
+	if (f->f_path.dentry->d_inode &&
+	    S_ISSOCK(f->f_path.dentry->d_inode->i_mode))
+		return 0;
 	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
 					    f->f_flags);
 }
-- 
1.8.3.1

Re: [PATCH] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Hello, Al.

Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
    management.

Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
    Do we need to use d_backing_inode() or d_inode() ?

Regards.

On 2019/06/09 15:41, Tetsuo Handa wrote:
> syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> use after free problem [1], for socket's inode is still reachable via
> /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> 
> But there is no point with calling security_file_open() on sockets
> because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> 
> There is some point with calling security_inode_getattr() on sockets
> because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> fields, we will need to use security_socket_post_create() hook and
> security_inode_free() hook in order to remember these fields because
> security_sk_free() hook is called before the inode is destructed. But
> since information which can be protected by checking
> security_inode_getattr() on sockets is trivial, let's not be bothered by
> "struct inode"->i_security management.
> 
> There is point with calling security_file_ioctl() on sockets. Since
> ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> on sockets should remain safe.
> 
> [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
> ---
>  security/tomoyo/tomoyo.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92e..9661b86 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
>   */
>  static int tomoyo_inode_getattr(const struct path *path)
>  {
> +	/* It is not safe to call tomoyo_get_socket_name(). */
> +	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
> +		return 0;
>  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
>  }
>  
> @@ -316,6 +319,10 @@ static int tomoyo_file_open(struct file *f)
>  	/* Don't check read permission here if called from do_execve(). */
>  	if (current->in_execve)
>  		return 0;
> +	/* Sockets can't be opened by open(). */
> +	if (f->f_path.dentry->d_inode &&
> +	    S_ISSOCK(f->f_path.dentry->d_inode->i_mode))
> +		return 0;
>  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
>  					    f->f_flags);
>  }
> 

Re: [PATCH] tomoyo: Don't check open/getattr permission on sockets.

[Al Viro]

On Sun, Jun 16, 2019 at 03:49:00PM +0900, Tetsuo Handa wrote:
> Hello, Al.
> 
> Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
>     management.

You do realize that sockets are not unique in that respect, right?
All kinds of interesting stuff can be accessed via /proc/*/fd/*, and
it _can_ be closed under you.  So I'd suggest checking how your code
copes with similar for pipes, FIFOs, epoll, etc., accessed that way...

We are _not_ going to be checking that in fs/open.c - the stuff found
via /proc/*/fd/* can have the associated file closed by the time
we get to calling ->open() and we won't know that until said call.

> Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
>     Do we need to use d_backing_inode() or d_inode() ?

Huh?  What's wrong with file_inode(f), in the first place?  And
just when can that be NULL, while we are at it?

> >  static int tomoyo_inode_getattr(const struct path *path)
> >  {
> > +	/* It is not safe to call tomoyo_get_socket_name(). */
> > +	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
> > +		return 0;

Can that be called for a negative?

[PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/06/19 5:49, Al Viro wrote:
> On Sun, Jun 16, 2019 at 03:49:00PM +0900, Tetsuo Handa wrote:
>> Hello, Al.
>>
>> Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
>>     management.
> 
> You do realize that sockets are not unique in that respect, right?
> All kinds of interesting stuff can be accessed via /proc/*/fd/*, and
> it _can_ be closed under you.  So I'd suggest checking how your code
> copes with similar for pipes, FIFOs, epoll, etc., accessed that way...

I know all kinds of interesting stuff can be accessed via /proc/*/fd/*,
and it _can_ be closed under me.

Regarding sockets, I was accessing "struct socket" memory and
"struct sock" memory which are outside of "struct inode" memory.

But regarding other objects, I am accessing "struct dentry" memory,
"struct super_block" memory and "struct inode" memory. I'm expecting
that these memory can't be kfree()d as long as "struct path" holds
a reference. Is my expectation correct?

> 
> We are _not_ going to be checking that in fs/open.c - the stuff found
> via /proc/*/fd/* can have the associated file closed by the time
> we get to calling ->open() and we won't know that until said call.

OK. Then, fixing TOMOYO side is the correct way.

> 
>> Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
>>     Do we need to use d_backing_inode() or d_inode() ?
> 
> Huh?  What's wrong with file_inode(f), in the first place?  And
> just when can that be NULL, while we are at it?

Oh, I was not aware of file_inode(). Thanks.

> 
>>>  static int tomoyo_inode_getattr(const struct path *path)
>>>  {
>>> +	/* It is not safe to call tomoyo_get_socket_name(). */
>>> +	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
>>> +		return 0;
> 
> Can that be called for a negative?
> 

I check for NULL when I'm not sure it is guaranteed to hold a valid pointer.
You meant "we are sure that path->dentry->d_inode is valid", don't you?

By the way, "negative" associates with IS_ERR() range. I guess that
"NULL" is the better name...

Anyway, here is V2 patch.

From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 22 Jun 2019 13:14:26 +0900
Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

syzbot is reporting that use of SOCKET_I()->sk from open() can result in
use after free problem [1], for socket's inode is still reachable via
/proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.

But there is no point with calling security_file_open() on sockets
because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.

There is some point with calling security_inode_getattr() on sockets
because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
are valid. If we want to access "struct sock"->sk_{family,type,protocol}
fields, we will need to use security_socket_post_create() hook and
security_inode_free() hook in order to remember these fields because
security_sk_free() hook is called before the inode is destructed. But
since information which can be protected by checking
security_inode_getattr() on sockets is trivial, let's not be bothered by
"struct inode"->i_security management.

There is point with calling security_file_ioctl() on sockets. Since
ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
on sockets should remain safe.

[1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
---
 security/tomoyo/tomoyo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..8ea3f5d 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
  */
 static int tomoyo_inode_getattr(const struct path *path)
 {
+	/* It is not safe to call tomoyo_get_socket_name(). */
+	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
+		return 0;
 	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
 }
 
@@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
 	/* Don't check read permission here if called from do_execve(). */
 	if (current->in_execve)
 		return 0;
+	/* Sockets can't be opened by open(). */
+	if (S_ISSOCK(file_inode(f)->i_mode))
+		return 0;
 	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
 					    f->f_flags);
 }
-- 
1.8.3.1

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Hello.

Since it seems that Al has no comments, I'd like to send this patch to
linux-next.git . What should I do? Do I need to set up a git tree?

On 2019/06/22 13:45, Tetsuo Handa wrote:
> From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 22 Jun 2019 13:14:26 +0900
> Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
> 
> syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> use after free problem [1], for socket's inode is still reachable via
> /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> 
> But there is no point with calling security_file_open() on sockets
> because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> 
> There is some point with calling security_inode_getattr() on sockets
> because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> fields, we will need to use security_socket_post_create() hook and
> security_inode_free() hook in order to remember these fields because
> security_sk_free() hook is called before the inode is destructed. But
> since information which can be protected by checking
> security_inode_getattr() on sockets is trivial, let's not be bothered by
> "struct inode"->i_security management.
> 
> There is point with calling security_file_ioctl() on sockets. Since
> ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> on sockets should remain safe.
> 
> [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
> ---
>  security/tomoyo/tomoyo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92e..8ea3f5d 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
>   */
>  static int tomoyo_inode_getattr(const struct path *path)
>  {
> +	/* It is not safe to call tomoyo_get_socket_name(). */
> +	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
> +		return 0;
>  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
>  }
>  
> @@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
>  	/* Don't check read permission here if called from do_execve(). */
>  	if (current->in_execve)
>  		return 0;
> +	/* Sockets can't be opened by open(). */
> +	if (S_ISSOCK(file_inode(f)->i_mode))
> +		return 0;
>  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
>  					    f->f_flags);
>  }
> 

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[James Morris]

On Thu, 4 Jul 2019, Tetsuo Handa wrote:

> Hello.
> 
> Since it seems that Al has no comments, I'd like to send this patch to
> linux-next.git . What should I do? Do I need to set up a git tree?

Yes, you can create one at github or similar.


> 
> On 2019/06/22 13:45, Tetsuo Handa wrote:
> > From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Date: Sat, 22 Jun 2019 13:14:26 +0900
> > Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
> > 
> > syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> > use after free problem [1], for socket's inode is still reachable via
> > /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> > 
> > But there is no point with calling security_file_open() on sockets
> > because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> > 
> > There is some point with calling security_inode_getattr() on sockets
> > because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> > are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> > fields, we will need to use security_socket_post_create() hook and
> > security_inode_free() hook in order to remember these fields because
> > security_sk_free() hook is called before the inode is destructed. But
> > since information which can be protected by checking
> > security_inode_getattr() on sockets is trivial, let's not be bothered by
> > "struct inode"->i_security management.
> > 
> > There is point with calling security_file_ioctl() on sockets. Since
> > ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> > on sockets should remain safe.
> > 
> > [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> > 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
> > ---
> >  security/tomoyo/tomoyo.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> > index 716c92e..8ea3f5d 100644
> > --- a/security/tomoyo/tomoyo.c
> > +++ b/security/tomoyo/tomoyo.c
> > @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
> >   */
> >  static int tomoyo_inode_getattr(const struct path *path)
> >  {
> > +	/* It is not safe to call tomoyo_get_socket_name(). */
> > +	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
> > +		return 0;
> >  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
> >  }
> >  
> > @@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
> >  	/* Don't check read permission here if called from do_execve(). */
> >  	if (current->in_execve)
> >  		return 0;
> > +	/* Sockets can't be opened by open(). */
> > +	if (S_ISSOCK(file_inode(f)->i_mode))
> > +		return 0;
> >  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
> >  					    f->f_flags);
> >  }
> > 
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[James Morris]

On Sat, 6 Jul 2019, James Morris wrote:

> On Thu, 4 Jul 2019, Tetsuo Handa wrote:
> 
> > Hello.
> > 
> > Since it seems that Al has no comments, I'd like to send this patch to
> > linux-next.git . What should I do? Do I need to set up a git tree?
> 
> Yes, you can create one at github or similar.

Also notify Stephen Rothwell of the location of your -next branch, so it 
gets pulled into his tree.



-- 
James Morris
<jmorris@namei.org>

[PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

syzbot is reporting that use of SOCKET_I()->sk from open() can result in
use after free problem [1], for socket's inode is still reachable via
/proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.

But there is no point with calling security_file_open() on sockets
because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.

There is some point with calling security_inode_getattr() on sockets
because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
are valid. If we want to access "struct sock"->sk_{family,type,protocol}
fields, we will need to use security_socket_post_create() hook and
security_inode_free() hook in order to remember these fields because
security_sk_free() hook is called before the inode is destructed. But
since information which can be protected by checking
security_inode_getattr() on sockets is trivial, let's not be bothered by
"struct inode"->i_security management.

There is point with calling security_file_ioctl() on sockets. Since
ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
on sockets should remain safe.

[1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
---
 security/tomoyo/tomoyo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..8ea3f5d 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
  */
 static int tomoyo_inode_getattr(const struct path *path)
 {
+	/* It is not safe to call tomoyo_get_socket_name(). */
+	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
+		return 0;
 	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
 }
 
@@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
 	/* Don't check read permission here if called from do_execve(). */
 	if (current->in_execve)
 		return 0;
+	/* Sockets can't be opened by open(). */
+	if (S_ISSOCK(file_inode(f)->i_mode))
+		return 0;
 	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
 					    f->f_flags);
 }
-- 
1.8.3.1

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/07/07 11:50, James Morris wrote:
> On Sat, 6 Jul 2019, James Morris wrote:
> 
>> On Thu, 4 Jul 2019, Tetsuo Handa wrote:
>>
>>> Hello.
>>>
>>> Since it seems that Al has no comments, I'd like to send this patch to
>>> linux-next.git . What should I do? Do I need to set up a git tree?
>>
>> Yes, you can create one at github or similar.
> 
> Also notify Stephen Rothwell of the location of your -next branch, so it 
> gets pulled into his tree.
> 

I executed commands shown below. Since I'm not familiar with git management,
I want to use only master branch. Is this sequence correct?

# Upon initialization
git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
cd tomoyo-test1/
git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update upstream
git merge upstream/master
git push -u origin master

# When making changes
git remote update upstream
git merge upstream/master
git am 0001-tomoyo-Don-t-check-open-getattr-permission-on-socket.patch
git push -u origin master

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Hello, Stephen Rothwell.

Can you add https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git#master
into the list for linux-next.git tree?

On 2019/09/03 15:52, Tetsuo Handa wrote:
> On 2019/07/07 11:50, James Morris wrote:
>> On Sat, 6 Jul 2019, James Morris wrote:
>>
>>> On Thu, 4 Jul 2019, Tetsuo Handa wrote:
>>>
>>>> Hello.
>>>>
>>>> Since it seems that Al has no comments, I'd like to send this patch to
>>>> linux-next.git . What should I do? Do I need to set up a git tree?
>>>
>>> Yes, you can create one at github or similar.
>>
>> Also notify Stephen Rothwell of the location of your -next branch, so it 
>> gets pulled into his tree.
>>
> 
> I executed commands shown below. Since I'm not familiar with git management,
> I want to use only master branch. Is this sequence correct?
> 
> # Upon initialization
> git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
> cd tomoyo-test1/
> git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git remote update upstream
> git merge upstream/master
> git push -u origin master
> 
> # When making changes
> git remote update upstream
> git merge upstream/master
> git am 0001-tomoyo-Don-t-check-open-getattr-permission-on-socket.patch
> git push -u origin master
> 

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/09/14 16:36, Stephen Rothwell wrote:
> Hi,
> 
> I am on vacation until after the merge window closes, so I will add it then.
> Please remind me if I don't.

Did you return from the vacation?

> 
> Cheers,
> Stephen Rothwell 
> 
> On 13 September 2019 2:41:24 pm GMT+01:00, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> Hello, Stephen Rothwell.
>>
>> Can you add https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git#master
>> into the list for linux-next.git tree?

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Hi Tetsuo,

On Wed, 2 Oct 2019 19:50:48 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/09/14 16:36, Stephen Rothwell wrote:
> > 
> > I am on vacation until after the merge window closes, so I will add it then.
> > Please remind me if I don't.  
> 
> Did you return from the vacation?

I knew I'd missed one (but I have too much email :-().

I don't think the back merges of Linus' tree add anything useful to
your tree.  At this point it probably makes sense to just rebase the
single patch onto v5.4-rc1 and then not back merge Linus' tree at all
(unless some really complex conflict arises).
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Hello.

On 2019/10/03 7:25, Stephen Rothwell wrote:
> Hi Tetsuo,
> 
> On Wed, 2 Oct 2019 19:50:48 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> On 2019/09/14 16:36, Stephen Rothwell wrote:
>>>
>>> I am on vacation until after the merge window closes, so I will add it then.
>>> Please remind me if I don't.  
>>
>> Did you return from the vacation?
> 
> I knew I'd missed one (but I have too much email :-().

Thank you for adding my tree.

> 
> I don't think the back merges of Linus' tree add anything useful to
> your tree.  At this point it probably makes sense to just rebase the
> single patch onto v5.4-rc1 and then not back merge Linus' tree at all
> (unless some really complex conflict arises).
> 

This is my first time using persistent git tree.
I made my tree using the sequence shown below.

  # Upon initialization
  git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
  cd tomoyo-test1/
  git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  git remote update upstream
  git merge upstream/master
  git push -u origin master

According to
https://lkml.kernel.org/r/CAHk-=wg=io4rX2qzupdd4XdYy6okMx5jjzEwXe_UvLQgLsSUFA@mail.gmail.com
I should not try "git rebase" and "git merge" because I don't understand what they do. But
I guess I need to use "git merge" in order to update my tree before making changes.
Is the sequence shown below appropriate?

  # When making changes
  git remote update upstream
  git merge upstream/master
  edit files
  git commit
  git push -u origin master

Since I'm not familiar with git management, I want to use only master branch.
Do I need to make branches or another git tree for asking Linus to pull for linux.git ?

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Hello, Andrew and James.

I have difficulty setting up environments for sending pull request to linux.git
(nobody around me knows Linux kernel maintainer's workflow at the command line level).
Can you pick up the following commit via mmotm or linux-security.git tree?

commit a5f9bda81cb4fa513f74707083b1eeee8735547f
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date:   Sat Jun 22 13:14:26 2019 +0900

    tomoyo: Don't check open/getattr permission on sockets.

On 2019/10/03 18:59, Tetsuo Handa wrote:
> Hello.
> 
> On 2019/10/03 7:25, Stephen Rothwell wrote:
>> Hi Tetsuo,
>>
>> On Wed, 2 Oct 2019 19:50:48 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>
>>> On 2019/09/14 16:36, Stephen Rothwell wrote:
>>>>
>>>> I am on vacation until after the merge window closes, so I will add it then.
>>>> Please remind me if I don't.  
>>>
>>> Did you return from the vacation?
>>
>> I knew I'd missed one (but I have too much email :-().
> 
> Thank you for adding my tree.
> 
>>
>> I don't think the back merges of Linus' tree add anything useful to
>> your tree.  At this point it probably makes sense to just rebase the
>> single patch onto v5.4-rc1 and then not back merge Linus' tree at all
>> (unless some really complex conflict arises).
>>
> 
> This is my first time using persistent git tree.
> I made my tree using the sequence shown below.
> 
>   # Upon initialization
>   git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
>   cd tomoyo-test1/
>   git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>   git remote update upstream
>   git merge upstream/master
>   git push -u origin master
> 
> According to
> https://lkml.kernel.org/r/CAHk-=wg=io4rX2qzupdd4XdYy6okMx5jjzEwXe_UvLQgLsSUFA@mail.gmail.com
> I should not try "git rebase" and "git merge" because I don't understand what they do. But
> I guess I need to use "git merge" in order to update my tree before making changes.
> Is the sequence shown below appropriate?
> 
>   # When making changes
>   git remote update upstream
>   git merge upstream/master
>   edit files
>   git commit
>   git push -u origin master
> 
> Since I'm not familiar with git management, I want to use only master branch.
> Do I need to make branches or another git tree for asking Linus to pull for linux.git ?
> 

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[James Morris]

On Wed, 13 Nov 2019, Tetsuo Handa wrote:

> Hello, Andrew and James.
> 
> I have difficulty setting up environments for sending pull request to linux.git
> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
> Can you pick up the following commit via mmotm or linux-security.git tree?

Not sure if your fix is complete.

Are there other potential paths to trigger this via tomoyo_path_perm() ?

e.g. call unlink(2) on /proc/pid/fd/sockfd


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/11/21 16:21, James Morris wrote:
> On Wed, 13 Nov 2019, Tetsuo Handa wrote:
> 
>> Hello, Andrew and James.
>>
>> I have difficulty setting up environments for sending pull request to linux.git
>> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
>> Can you pick up the following commit via mmotm or linux-security.git tree?
> 
> Not sure if your fix is complete.
> 
> Are there other potential paths to trigger this via tomoyo_path_perm() ?
> 
> e.g. call unlink(2) on /proc/pid/fd/sockfd

I think they are safe. For example, unlink(2) checks that
inode is valid before calling security_path_unlink().

        dentry = __lookup_hash(&last, path.dentry, lookup_flags);
        error = PTR_ERR(dentry);
        if (!IS_ERR(dentry)) {
                /* Why not before? Because we want correct error value */
                if (last.name[last.len])
                        goto slashes;
                inode = dentry->d_inode;
                if (d_is_negative(dentry))
                        goto slashes;
                ihold(inode);
                error = security_path_unlink(&path, dentry);
                if (error)
                        goto exit2;
                error = vfs_unlink(path.dentry->d_inode, dentry, &delegated_inode);
exit2:
                dput(dentry);
        }

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/11/21 19:18, Tetsuo Handa wrote:
> On 2019/11/21 16:21, James Morris wrote:
>> On Wed, 13 Nov 2019, Tetsuo Handa wrote:
>>
>>> Hello, Andrew and James.
>>>
>>> I have difficulty setting up environments for sending pull request to linux.git
>>> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
>>> Can you pick up the following commit via mmotm or linux-security.git tree?
>>
>> Not sure if your fix is complete.
>>
>> Are there other potential paths to trigger this via tomoyo_path_perm() ?
>>
>> e.g. call unlink(2) on /proc/pid/fd/sockfd
> 
> I think they are safe. For example, unlink(2) checks that
> inode is valid before calling security_path_unlink().

Hmm, since unlink(2) locks parent's inode instead of inode to be removed itself,
there is indeed possibility that tomoyo_path_perm() races with __sock_release().
We need another patch...

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/11/21 22:59, Tetsuo Handa wrote:
> On 2019/11/21 19:18, Tetsuo Handa wrote:
>> On 2019/11/21 16:21, James Morris wrote:
>>> On Wed, 13 Nov 2019, Tetsuo Handa wrote:
>>>
>>>> Hello, Andrew and James.
>>>>
>>>> I have difficulty setting up environments for sending pull request to linux.git
>>>> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
>>>> Can you pick up the following commit via mmotm or linux-security.git tree?
>>>
>>> Not sure if your fix is complete.
>>>
>>> Are there other potential paths to trigger this via tomoyo_path_perm() ?
>>>
>>> e.g. call unlink(2) on /proc/pid/fd/sockfd
>>
>> I think they are safe. For example, unlink(2) checks that
>> inode is valid before calling security_path_unlink().
> 
> Hmm, since unlink(2) locks parent's inode instead of inode to be removed itself,
> there is indeed possibility that tomoyo_path_perm() races with __sock_release().
> We need another patch...
> 

I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?

commit c39593ab0500fcd6db290b311c120349927ddc04
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date:   Mon Nov 25 10:46:51 2019 +0900

    tomoyo: Don't use nifty names on sockets.

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[James Morris]

On Wed, 4 Dec 2019, Tetsuo Handa wrote:

> 
> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
> 
> commit c39593ab0500fcd6db290b311c120349927ddc04
> Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date:   Mon Nov 25 10:46:51 2019 +0900
> 
>     tomoyo: Don't use nifty names on sockets.
> 

From where?

Please send a patch.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

On 2019/12/10 6:37, James Morris wrote:
> On Wed, 4 Dec 2019, Tetsuo Handa wrote:
> 
>>
>> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
>>
>> commit c39593ab0500fcd6db290b311c120349927ddc04
>> Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Date:   Mon Nov 25 10:46:51 2019 +0900
>>
>>     tomoyo: Don't use nifty names on sockets.
>>
> 
>>From where?
> 
> Please send a patch.
> 

Patch is at https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1 and was tested on linux-next.git .
But if you pick up c39593ab0500, what do I need to do (in order to avoid trying to apply the same
patch) ? Could you explain me (using command line) how I can send only c39593ab0500 to linux.git ?
https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits has only master branch.

c39593ab0500 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1
fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
19768fdc4025 Revert "printk: Monitor change of console loglevel."
07fca3f339d7 printk: Monitor change of console loglevel.
df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.
219d54332a09 (tag: v5.4, upstream/master) Linux 5.4

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]

Hi Tetsuo,

On Tue, 10 Dec 2019 19:21:08 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/12/10 6:37, James Morris wrote:
> > On Wed, 4 Dec 2019, Tetsuo Handa wrote:
> >   
> >>
> >> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
> >>
> >> commit c39593ab0500fcd6db290b311c120349927ddc04
> >> Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Date:   Mon Nov 25 10:46:51 2019 +0900
> >>
> >>     tomoyo: Don't use nifty names on sockets.
> >>  
> >   
> >>From where?  
> > 
> > Please send a patch.
> >   
> 
> Patch is at https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1 and was tested on linux-next.git .
> But if you pick up c39593ab0500, what do I need to do (in order to avoid trying to apply the same
> patch) ? Could you explain me (using command line) how I can send only c39593ab0500 to linux.git ?
> https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits has only master branch.
> 
> c39593ab0500 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
> cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1
> fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
> 19768fdc4025 Revert "printk: Monitor change of console loglevel."
> 07fca3f339d7 printk: Monitor change of console loglevel.
> df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.
> 219d54332a09 (tag: v5.4, upstream/master) Linux 5.4

You should start by cleaning up your tree:

remove

fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
19768fdc4025 Revert "printk: Monitor change of console loglevel."
07fca3f339d7 printk: Monitor change of console loglevel.
df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.

since they end up cancelling each other out

cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1

only introduces these commits:

79c8ca578dbf Revert "printk: Monitor change of console loglevel."
23641a048089 printk: Monitor change of console loglevel.
a5f9bda81cb4 tomoyo: Don't check open/getattr permission on sockets.

and the first 2 above cancel each other out.

so you are left with these:

c39593ab0500 tomoyo: Don't use nifty names on sockets.
a5f9bda81cb4 tomoyo: Don't check open/getattr permission on sockets.

you should rebase these onto v5.5-rc1.

If you want James to just take the first of these, then the easiest way
is probably to just send him a patch that you generate using "git
format-patch" and then remove it from your tree.

Since there are no other changes to the only file affected by that
commit since v5.4, you could just do this:

$ git format-patch -o <some file> -1 c39593ab0500

and then <some file> to James using your email client.

Having done that, you should just do this (and forget the cleanups
above):

$ git checkout master
$ git remote update upstream
$ git reset --hard upstream/master
$ git cherry-pick a5f9bda81cb4
$ git push -f origin master

After that you will have a nice clean tree (based on Linus' tree) to
continue development on that just contains the one patch "tomoyo: Don't
check open/getattr permission on sockets."

If, however, you intend to only send patche via James tree, then you
should be using his tree
(git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
branch next-testing) as your upstream tree, not Linus' tree.  Then you
can ask him to merge your tree before the merge window opens during
each cycle.  You may want to rebase your tree on top of James tree
after he applies your patch from above.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Hello, Stephen Rothwell.

Thank you for the command line.

I was wondering why "git push -f" does not work. But I finally found
there is denyNonFastforwards option, and I was able to clean up.

$ git format-patch -1
0001-tomoyo-Don-t-use-nifty-names-on-sockets.patch
$ git reset --hard v5.5-rc1
$ git push -f origin master
$ git pull upstream master
$ git am 0001-tomoyo-Don-t-use-nifty-names-on-sockets.patch
$ git push origin master

On 2019/12/11 8:02, Stephen Rothwell wrote:
> Having done that, you should just do this (and forget the cleanups
> above):
> 
> $ git checkout master
> $ git remote update upstream
> $ git reset --hard upstream/master
> $ git cherry-pick a5f9bda81cb4
> $ git push -f origin master
> 
> After that you will have a nice clean tree (based on Linus' tree) to
> continue development on that just contains the one patch "tomoyo: Don't
> check open/getattr permission on sockets."

Now the history looks like below.

6f7c41374b62 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
6794862a16ef (upstream/master) Merge tag 'for-5.5-rc1-kconfig-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
184b8f7f91ca Merge tag 'printk-for-5.5-pr-warning-removal' of git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk
316eaf170252 Merge tag 'thermal-5.5-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux
78f926f72e43 btrfs: add Kconfig dependency for BLAKE2B
e42617b825f8 (tag: v5.5-rc1) Linux 5.5-rc1

> 
> If, however, you intend to only send patche via James tree, then you
> should be using his tree
> (git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
> branch next-testing) as your upstream tree, not Linus' tree.  Then you
> can ask him to merge your tree before the merge window opens during
> each cycle.  You may want to rebase your tree on top of James tree
> after he applies your patch from above.
> 

I was previously using linux-security.git . But after experiencing confusion of
linux-security.git management, LSM users (including me) were suggested to have
their own git tree and were suggested to directly send patches to Linus.
And I am currently experiencing confusion of my tree management (because I have
never maintained a tree for "git push" purpose)...

Next step is how to create a pull request. If I recall correctly, I need a
GPG key for signing my request? I have a GPG key but I have never attended
key signing party.

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]

Hi Tetsuo,

On Fri, 13 Sep 2019 22:41:24 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Can you add https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git#master
> into the list for linux-next.git tree?

Added from today.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgement of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
        Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Eric Biggers]

Hi Tetsuo,

On Sat, Jun 22, 2019 at 01:45:30PM +0900, Tetsuo Handa wrote:
> On 2019/06/19 5:49, Al Viro wrote:
> > On Sun, Jun 16, 2019 at 03:49:00PM +0900, Tetsuo Handa wrote:
> >> Hello, Al.
> >>
> >> Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
> >>     management.
> > 
> > You do realize that sockets are not unique in that respect, right?
> > All kinds of interesting stuff can be accessed via /proc/*/fd/*, and
> > it _can_ be closed under you.  So I'd suggest checking how your code
> > copes with similar for pipes, FIFOs, epoll, etc., accessed that way...
> 
> I know all kinds of interesting stuff can be accessed via /proc/*/fd/*,
> and it _can_ be closed under me.
> 
> Regarding sockets, I was accessing "struct socket" memory and
> "struct sock" memory which are outside of "struct inode" memory.
> 
> But regarding other objects, I am accessing "struct dentry" memory,
> "struct super_block" memory and "struct inode" memory. I'm expecting
> that these memory can't be kfree()d as long as "struct path" holds
> a reference. Is my expectation correct?
> 
> > 
> > We are _not_ going to be checking that in fs/open.c - the stuff found
> > via /proc/*/fd/* can have the associated file closed by the time
> > we get to calling ->open() and we won't know that until said call.
> 
> OK. Then, fixing TOMOYO side is the correct way.
> 
> > 
> >> Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
> >>     Do we need to use d_backing_inode() or d_inode() ?
> > 
> > Huh?  What's wrong with file_inode(f), in the first place?  And
> > just when can that be NULL, while we are at it?
> 
> Oh, I was not aware of file_inode(). Thanks.
> 
> > 
> >>>  static int tomoyo_inode_getattr(const struct path *path)
> >>>  {
> >>> +	/* It is not safe to call tomoyo_get_socket_name(). */
> >>> +	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
> >>> +		return 0;
> > 
> > Can that be called for a negative?
> > 
> 
> I check for NULL when I'm not sure it is guaranteed to hold a valid pointer.
> You meant "we are sure that path->dentry->d_inode is valid", don't you?
> 
> By the way, "negative" associates with IS_ERR() range. I guess that
> "NULL" is the better name...
> 
> Anyway, here is V2 patch.
> 
> From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 22 Jun 2019 13:14:26 +0900
> Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
> 
> syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> use after free problem [1], for socket's inode is still reachable via
> /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> 
> But there is no point with calling security_file_open() on sockets
> because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> 
> There is some point with calling security_inode_getattr() on sockets
> because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> fields, we will need to use security_socket_post_create() hook and
> security_inode_free() hook in order to remember these fields because
> security_sk_free() hook is called before the inode is destructed. But
> since information which can be protected by checking
> security_inode_getattr() on sockets is trivial, let's not be bothered by
> "struct inode"->i_security management.
> 
> There is point with calling security_file_ioctl() on sockets. Since
> ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> on sockets should remain safe.
> 
> [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
> ---
>  security/tomoyo/tomoyo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92e..8ea3f5d 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
>   */
>  static int tomoyo_inode_getattr(const struct path *path)
>  {
> +	/* It is not safe to call tomoyo_get_socket_name(). */
> +	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
> +		return 0;
>  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
>  }
>  
> @@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
>  	/* Don't check read permission here if called from do_execve(). */
>  	if (current->in_execve)
>  		return 0;
> +	/* Sockets can't be opened by open(). */
> +	if (S_ISSOCK(file_inode(f)->i_mode))
> +		return 0;
>  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
>  					    f->f_flags);
>  }
> -- 

What happened to this patch?

Also, isn't the same bug in other places too?:

	- tomoyo_path_chmod()
	- tomoyo_path_chown()
	- smack_inode_getsecurity()
	- smack_inode_setsecurity()

- Eric

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Eric Biggers wrote:
> What happened to this patch?

I have to learn how to manage a git tree for sending
pull requests, but I can't find time to try.

> 
> Also, isn't the same bug in other places too?:
> 
> 	- tomoyo_path_chmod()
> 	- tomoyo_path_chown()
> 	- smack_inode_getsecurity()
> 	- smack_inode_setsecurity()

What's the bug? The file descriptor returned by open(O_PATH) cannot be
passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Eric Biggers]

On Thu, Aug 22, 2019 at 03:55:31PM +0900, Tetsuo Handa wrote:
> Eric Biggers wrote:
> > What happened to this patch?
> 
> I have to learn how to manage a git tree for sending
> pull requests, but I can't find time to try.
> 
> > 
> > Also, isn't the same bug in other places too?:
> > 
> > 	- tomoyo_path_chmod()
> > 	- tomoyo_path_chown()
> > 	- smack_inode_getsecurity()
> > 	- smack_inode_setsecurity()
> 
> What's the bug? The file descriptor returned by open(O_PATH) cannot be
> passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.
> 

chmod(2), chown(2), getxattr(2), and setxattr(2) take a path, not a fd.

- Eric

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Tetsuo Handa]

Eric Biggers wrote:
> On Thu, Aug 22, 2019 at 03:55:31PM +0900, Tetsuo Handa wrote:
> > > Also, isn't the same bug in other places too?:
> > > 
> > > 	- tomoyo_path_chmod()
> > > 	- tomoyo_path_chown()
> > > 	- smack_inode_getsecurity()
> > > 	- smack_inode_setsecurity()
> > 
> > What's the bug? The file descriptor returned by open(O_PATH) cannot be
> > passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.
> > 
> 
> chmod(2), chown(2), getxattr(2), and setxattr(2) take a path, not a fd.
> 

OK. Then, is the correct fix

  inode_lock(inode);
  if (SOCKET_I(inode)->sk) {
    // Can access SOCKET_I(sock)->sk->*
  } else {
    // Already close()d. Don't touch.
  }
  inode_unlock(inode);

thanks to

  commit 6d8c50dcb029872b ("socket: close race condition between sock_close() and sockfs_setattr()")
  commit ff7b11aa481f682e ("net: socket: set sock->sk to NULL after calling proto_ops::release()")

changes?

Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

[Eric Biggers]

On Thu, Aug 22, 2019 at 04:42:26PM +0900, Tetsuo Handa wrote:
> Eric Biggers wrote:
> > On Thu, Aug 22, 2019 at 03:55:31PM +0900, Tetsuo Handa wrote:
> > > > Also, isn't the same bug in other places too?:
> > > > 
> > > > 	- tomoyo_path_chmod()
> > > > 	- tomoyo_path_chown()
> > > > 	- smack_inode_getsecurity()
> > > > 	- smack_inode_setsecurity()
> > > 
> > > What's the bug? The file descriptor returned by open(O_PATH) cannot be
> > > passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.
> > > 
> > 
> > chmod(2), chown(2), getxattr(2), and setxattr(2) take a path, not a fd.
> > 
> 
> OK. Then, is the correct fix
> 
>   inode_lock(inode);
>   if (SOCKET_I(inode)->sk) {
>     // Can access SOCKET_I(sock)->sk->*
>   } else {
>     // Already close()d. Don't touch.
>   }
>   inode_unlock(inode);
> 
> thanks to
> 
>   commit 6d8c50dcb029872b ("socket: close race condition between sock_close() and sockfs_setattr()")
>   commit ff7b11aa481f682e ("net: socket: set sock->sk to NULL after calling proto_ops::release()")
> 
> changes?

inode_lock() is already held during security_path_chmod(),
security_path_chown(), and security_inode_setxattr().
So you can't just take it again.

- Eric