Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
* KASAN: use-after-free Read in tomoyo_realpath_from_path
@ 2019-06-05 18:42 syzbot
  2019-06-05 22:09 ` Tetsuo Handa
                   ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: syzbot @ 2019-06-05 18:42 UTC (permalink / raw)
  To: jmorris, linux-kernel, linux-security-module, penguin-kernel,
	serge, syzkaller-bugs, takedakn

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: KASAN: use-after-free Read in tomoyo_realpath_from_path
  2019-06-05 18:42 KASAN: use-after-free Read in tomoyo_realpath_from_path syzbot
@ 2019-06-05 22:09 ` Tetsuo Handa
  2019-06-06  2:08 ` Tetsuo Handa
  2019-06-06  5:20 ` Tetsuo Handa
  2 siblings, 0 replies; 30+ messages in thread
From: Tetsuo Handa @ 2019-06-05 22:09 UTC (permalink / raw)
  To: Alexander Viro
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, takedakn, David S. Miller

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
> ==================================================================


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: KASAN: use-after-free Read in tomoyo_realpath_from_path
  2019-06-05 18:42 KASAN: use-after-free Read in tomoyo_realpath_from_path syzbot
  2019-06-05 22:09 ` Tetsuo Handa
@ 2019-06-06  2:08 ` Tetsuo Handa
  2019-06-06  5:20 ` Tetsuo Handa
  2 siblings, 0 replies; 30+ messages in thread
From: Tetsuo Handa @ 2019-06-06  2:08 UTC (permalink / raw)
  To: Al Viro, linux-fsdevel
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, takedakn

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
----------------------------------------

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: KASAN: use-after-free Read in tomoyo_realpath_from_path
  2019-06-05 18:42 KASAN: use-after-free Read in tomoyo_realpath_from_path syzbot
  2019-06-05 22:09 ` Tetsuo Handa
  2019-06-06  2:08 ` Tetsuo Handa
@ 2019-06-06  5:20 ` Tetsuo Handa
  2019-06-09  6:41   ` [PATCH] tomoyo: Don't check open/getattr permission on sockets Tetsuo Handa
  2 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-06-06  5:20 UTC (permalink / raw)
  To: Al Viro, linux-fsdevel
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, takedakn

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;
}
----------------------------------------

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH] tomoyo: Don't check open/getattr permission on sockets.
  2019-06-06  5:20 ` Tetsuo Handa
@ 2019-06-09  6:41   ` Tetsuo Handa
  2019-06-16  6:49     ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-06-09  6:41 UTC (permalink / raw)
  To: Al Viro, linux-fsdevel
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, takedakn

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] tomoyo: Don't check open/getattr permission on sockets.
  2019-06-09  6:41   ` [PATCH] tomoyo: Don't check open/getattr permission on sockets Tetsuo Handa
@ 2019-06-16  6:49     ` Tetsuo Handa
  2019-06-18 20:49       ` Al Viro
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-06-16  6:49 UTC (permalink / raw)
  To: Al Viro, linux-fsdevel
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, takedakn, David S. Miller

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] tomoyo: Don't check open/getattr permission on sockets.
  2019-06-16  6:49     ` Tetsuo Handa
@ 2019-06-18 20:49       ` Al Viro
  2019-06-22  4:45         ` [PATCH v2] " Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Al Viro @ 2019-06-18 20:49 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-06-18 20:49       ` Al Viro
@ 2019-06-22  4:45         ` " Tetsuo Handa
  2019-07-04 11:58           ` Tetsuo Handa
  2019-08-22  6:30           ` Eric Biggers
  0 siblings, 2 replies; 30+ messages in thread
From: Tetsuo Handa @ 2019-06-22  4:45 UTC (permalink / raw)
  To: Al Viro
  Cc: linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-06-22  4:45         ` [PATCH v2] " Tetsuo Handa
@ 2019-07-04 11:58           ` Tetsuo Handa
  2019-07-07  2:44             ` James Morris
  2019-08-22  6:30           ` Eric Biggers
  1 sibling, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-07-04 11:58 UTC (permalink / raw)
  To: jmorris; +Cc: linux-security-module

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-07-04 11:58           ` Tetsuo Handa
@ 2019-07-07  2:44             ` James Morris
  2019-07-07  2:50               ` James Morris
  0 siblings, 1 reply; 30+ messages in thread
From: James Morris @ 2019-07-07  2:44 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-security-module

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>


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-07-07  2:44             ` James Morris
@ 2019-07-07  2:50               ` James Morris
  2019-08-09 15:51                 ` Tetsuo Handa
  2019-09-03  6:52                 ` Tetsuo Handa
  0 siblings, 2 replies; 30+ messages in thread
From: James Morris @ 2019-07-07  2:50 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-security-module

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>


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-07-07  2:50               ` James Morris
@ 2019-08-09 15:51                 ` Tetsuo Handa
  2019-09-03  6:52                 ` Tetsuo Handa
  1 sibling, 0 replies; 30+ messages in thread
From: Tetsuo Handa @ 2019-08-09 15:51 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-security-module, Tetsuo Handa, syzbot

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-06-22  4:45         ` [PATCH v2] " Tetsuo Handa
  2019-07-04 11:58           ` Tetsuo Handa
@ 2019-08-22  6:30           ` Eric Biggers
  2019-08-22  6:55             ` Tetsuo Handa
  1 sibling, 1 reply; 30+ messages in thread
From: Eric Biggers @ 2019-08-22  6:30 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Al Viro, linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-08-22  6:30           ` Eric Biggers
@ 2019-08-22  6:55             ` Tetsuo Handa
  2019-08-22  7:01               ` Eric Biggers
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-08-22  6:55 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Al Viro, linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-08-22  6:55             ` Tetsuo Handa
@ 2019-08-22  7:01               ` Eric Biggers
  2019-08-22  7:42                 ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Eric Biggers @ 2019-08-22  7:01 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Al Viro, linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-08-22  7:01               ` Eric Biggers
@ 2019-08-22  7:42                 ` Tetsuo Handa
  2019-08-22 15:47                   ` Eric Biggers
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-08-22  7:42 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Al Viro, linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-08-22  7:42                 ` Tetsuo Handa
@ 2019-08-22 15:47                   ` Eric Biggers
  0 siblings, 0 replies; 30+ messages in thread
From: Eric Biggers @ 2019-08-22 15:47 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Al Viro, linux-fsdevel, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, takedakn,
	David S. Miller

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-07-07  2:50               ` James Morris
  2019-08-09 15:51                 ` Tetsuo Handa
@ 2019-09-03  6:52                 ` Tetsuo Handa
  2019-09-13 13:41                   ` Tetsuo Handa
  1 sibling, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-09-03  6:52 UTC (permalink / raw)
  To: James Morris; +Cc: linux-security-module, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-09-03  6:52                 ` Tetsuo Handa
@ 2019-09-13 13:41                   ` Tetsuo Handa
       [not found]                     ` <A9CE5147-4047-4C42-B772-F0ED510FA283@canb.auug.org.au>
  2019-10-02 22:22                     ` Stephen Rothwell
  0 siblings, 2 replies; 30+ messages in thread
From: Tetsuo Handa @ 2019-09-13 13:41 UTC (permalink / raw)
  To: Stephen Rothwell; +Cc: James Morris, linux-security-module

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
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
       [not found]                     ` <A9CE5147-4047-4C42-B772-F0ED510FA283@canb.auug.org.au>
@ 2019-10-02 10:50                       ` Tetsuo Handa
  2019-10-02 22:25                         ` Stephen Rothwell
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-10-02 10:50 UTC (permalink / raw)
  To: Stephen Rothwell; +Cc: James Morris, linux-security-module

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?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-09-13 13:41                   ` Tetsuo Handa
       [not found]                     ` <A9CE5147-4047-4C42-B772-F0ED510FA283@canb.auug.org.au>
@ 2019-10-02 22:22                     ` Stephen Rothwell
  1 sibling, 0 replies; 30+ messages in thread
From: Stephen Rothwell @ 2019-10-02 22:22 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: James Morris, linux-security-module

[-- 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 --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-10-02 10:50                       ` Tetsuo Handa
@ 2019-10-02 22:25                         ` Stephen Rothwell
  2019-10-03  9:59                           ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Stephen Rothwell @ 2019-10-02 22:25 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: James Morris, linux-security-module

[-- 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 --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-10-02 22:25                         ` Stephen Rothwell
@ 2019-10-03  9:59                           ` Tetsuo Handa
  2019-11-13 13:49                             ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-10-03  9:59 UTC (permalink / raw)
  To: Stephen Rothwell; +Cc: James Morris, linux-security-module, Linus Torvalds

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 ?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-10-03  9:59                           ` Tetsuo Handa
@ 2019-11-13 13:49                             ` Tetsuo Handa
  2019-11-21  7:21                               ` James Morris
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-11-13 13:49 UTC (permalink / raw)
  To: James Morris, Andrew Morton
  Cc: Stephen Rothwell, linux-security-module, Linus Torvalds

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 ?
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-11-13 13:49                             ` Tetsuo Handa
@ 2019-11-21  7:21                               ` James Morris
  2019-11-21 10:18                                 ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: James Morris @ 2019-11-21  7:21 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Andrew Morton, Stephen Rothwell, linux-security-module, Linus Torvalds

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>


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-11-21  7:21                               ` James Morris
@ 2019-11-21 10:18                                 ` Tetsuo Handa
  2019-11-21 13:59                                   ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-11-21 10:18 UTC (permalink / raw)
  To: James Morris
  Cc: Andrew Morton, Stephen Rothwell, linux-security-module, Linus Torvalds

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-11-21 10:18                                 ` Tetsuo Handa
@ 2019-11-21 13:59                                   ` Tetsuo Handa
  2019-12-04 12:50                                     ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-11-21 13:59 UTC (permalink / raw)
  To: James Morris
  Cc: Andrew Morton, Stephen Rothwell, linux-security-module, Linus Torvalds

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...

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-11-21 13:59                                   ` Tetsuo Handa
@ 2019-12-04 12:50                                     ` Tetsuo Handa
  2019-12-09 21:37                                       ` James Morris
  0 siblings, 1 reply; 30+ messages in thread
From: Tetsuo Handa @ 2019-12-04 12:50 UTC (permalink / raw)
  To: James Morris
  Cc: Andrew Morton, Stephen Rothwell, linux-security-module, Linus Torvalds

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-12-04 12:50                                     ` Tetsuo Handa
@ 2019-12-09 21:37                                       ` James Morris
  2019-12-10 10:21                                         ` Tetsuo Handa
  0 siblings, 1 reply; 30+ messages in thread
From: James Morris @ 2019-12-09 21:37 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Andrew Morton, Stephen Rothwell, linux-security-module, Linus Torvalds

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>


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
  2019-12-09 21:37                                       ` James Morris
@ 2019-12-10 10:21                                         ` Tetsuo Handa
  0 siblings, 0 replies; 30+ messages in thread
From: Tetsuo Handa @ 2019-12-10 10:21 UTC (permalink / raw)
  To: James Morris
  Cc: Andrew Morton, Stephen Rothwell, linux-security-module, Linus Torvalds

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, back to index

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-05 18:42 KASAN: use-after-free Read in tomoyo_realpath_from_path syzbot
2019-06-05 22:09 ` Tetsuo Handa
2019-06-06  2:08 ` Tetsuo Handa
2019-06-06  5:20 ` Tetsuo Handa
2019-06-09  6:41   ` [PATCH] tomoyo: Don't check open/getattr permission on sockets Tetsuo Handa
2019-06-16  6:49     ` Tetsuo Handa
2019-06-18 20:49       ` Al Viro
2019-06-22  4:45         ` [PATCH v2] " Tetsuo Handa
2019-07-04 11:58           ` Tetsuo Handa
2019-07-07  2:44             ` James Morris
2019-07-07  2:50               ` James Morris
2019-08-09 15:51                 ` Tetsuo Handa
2019-09-03  6:52                 ` Tetsuo Handa
2019-09-13 13:41                   ` Tetsuo Handa
     [not found]                     ` <A9CE5147-4047-4C42-B772-F0ED510FA283@canb.auug.org.au>
2019-10-02 10:50                       ` Tetsuo Handa
2019-10-02 22:25                         ` Stephen Rothwell
2019-10-03  9:59                           ` Tetsuo Handa
2019-11-13 13:49                             ` Tetsuo Handa
2019-11-21  7:21                               ` James Morris
2019-11-21 10:18                                 ` Tetsuo Handa
2019-11-21 13:59                                   ` Tetsuo Handa
2019-12-04 12:50                                     ` Tetsuo Handa
2019-12-09 21:37                                       ` James Morris
2019-12-10 10:21                                         ` Tetsuo Handa
2019-10-02 22:22                     ` Stephen Rothwell
2019-08-22  6:30           ` Eric Biggers
2019-08-22  6:55             ` Tetsuo Handa
2019-08-22  7:01               ` Eric Biggers
2019-08-22  7:42                 ` Tetsuo Handa
2019-08-22 15:47                   ` Eric Biggers

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git