linux-next boot error: general protection fault in tomoyo_get_local_path

linux-next boot error: general protection fault in tomoyo_get_local_path

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    bdecf38f Add linux-next specific files for 20200515
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=155a43b2100000
kernel config:  https://syzkaller.appspot.com/x/.config?x=27a5e30c87a59937
dashboard link: https://syzkaller.appspot.com/bug?extid=c1af344512918c61362c
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)

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

general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
CPU: 0 PID: 6698 Comm: sshd Not tainted 5.7.0-rc5-next-20200515-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:tomoyo_get_local_path+0x450/0x800 security/tomoyo/realpath.c:165
Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 b4 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 7f 60 49 8d 7f 28 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 87 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b
RSP: 0018:ffffc900063d7450 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: ffff88809975c000 RCX: ffffffff8363deda
RDX: 0000000000000005 RSI: ffffffff8363dee8 RDI: 0000000000000028
RBP: 1ffff92000c7ae8b R08: ffff8880a47644c0 R09: fffffbfff155a0a2
R10: ffffffff8aad050f R11: fffffbfff155a0a1 R12: ffff88809df3cfea
R13: ffff88809df3c000 R14: 0000000000001a2a R15: 0000000000000000
FS:  00007efe13ce28c0(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055e78cf578f5 CR3: 00000000987ed000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 tomoyo_realpath_from_path+0x393/0x620 security/tomoyo/realpath.c:282
 tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
 tomoyo_path_number_perm+0x1c2/0x4d0 security/tomoyo/file.c:723
 tomoyo_path_mknod+0x10d/0x190 security/tomoyo/tomoyo.c:246
 security_path_mknod+0x116/0x180 security/security.c:1072
 may_o_create fs/namei.c:2905 [inline]
 lookup_open+0x5ae/0x1320 fs/namei.c:3046
 open_last_lookups fs/namei.c:3155 [inline]
 path_openat+0x93c/0x27f0 fs/namei.c:3343
 do_filp_open+0x192/0x260 fs/namei.c:3373
 do_sys_openat2+0x585/0x7d0 fs/open.c:1179
 do_sys_open+0xc3/0x140 fs/open.c:1195
 do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
 entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x7efe11e4b6f0
Code: 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 83 3d 19 30 2c 00 00 75 10 b8 02 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 fe 9d 01 00 48 89 04 24
RSP: 002b:00007ffc3d0894d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 000055e78f0bc110 RCX: 00007efe11e4b6f0
RDX: 00000000000001b6 RSI: 0000000000000241 RDI: 000055e78cf578f5
RBP: 0000000000000004 R08: 0000000000000004 R09: 0000000000000001
R10: 0000000000000240 R11: 0000000000000246 R12: 000055e78cf2851e
R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace 0a58064de06d50f4 ]---
RIP: 0010:tomoyo_get_local_path+0x450/0x800 security/tomoyo/realpath.c:165
Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 b4 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 7f 60 49 8d 7f 28 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 87 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b
RSP: 0018:ffffc900063d7450 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: ffff88809975c000 RCX: ffffffff8363deda
RDX: 0000000000000005 RSI: ffffffff8363dee8 RDI: 0000000000000028
RBP: 1ffff92000c7ae8b R08: ffff8880a47644c0 R09: fffffbfff155a0a2
R10: ffffffff8aad050f R11: fffffbfff155a0a1 R12: ffff88809df3cfea
R13: ffff88809df3c000 R14: 0000000000001a2a R15: 0000000000000000
FS:  00007efe13ce28c0(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055dfe16c15f8 CR3: 00000000987ed000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


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

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Tetsuo Handa]

This is

        if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
                char *ep;
                const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
                struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry)); // <= here

                if (*ep == '/' && pid && pid ==
                    task_tgid_nr_ns(current, proc_pidns)) {

which was added by commit c59f415a7cb6e1e1 ("Use proc_pid_ns() to get pid_namespace from the proc superblock").

@@ -161,9 +162,10 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
        if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
                char *ep;
                const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
+               struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));

                if (*ep == '/' && pid && pid ==
-                   task_tgid_nr_ns(current, sb->s_fs_info)) {
+                   task_tgid_nr_ns(current, proc_pidns)) {
                        pos = ep - 5;
                        if (pos < buffer)
                                goto out;

Alexey and Eric, any clue?

On 2020/05/15 23:46, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    bdecf38f Add linux-next specific files for 20200515
> git tree:       linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=155a43b2100000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=27a5e30c87a59937
> dashboard link: https://syzkaller.appspot.com/bug?extid=c1af344512918c61362c
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
> 
> general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
> CPU: 0 PID: 6698 Comm: sshd Not tainted 5.7.0-rc5-next-20200515-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> RIP: 0010:tomoyo_get_local_path+0x450/0x800 security/tomoyo/realpath.c:165
> Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 b4 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 7f 60 49 8d 7f 28 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 87 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b
> RSP: 0018:ffffc900063d7450 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff88809975c000 RCX: ffffffff8363deda
> RDX: 0000000000000005 RSI: ffffffff8363dee8 RDI: 0000000000000028
> RBP: 1ffff92000c7ae8b R08: ffff8880a47644c0 R09: fffffbfff155a0a2
> R10: ffffffff8aad050f R11: fffffbfff155a0a1 R12: ffff88809df3cfea
> R13: ffff88809df3c000 R14: 0000000000001a2a R15: 0000000000000000
> FS:  00007efe13ce28c0(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000055e78cf578f5 CR3: 00000000987ed000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  tomoyo_realpath_from_path+0x393/0x620 security/tomoyo/realpath.c:282
>  tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
>  tomoyo_path_number_perm+0x1c2/0x4d0 security/tomoyo/file.c:723
>  tomoyo_path_mknod+0x10d/0x190 security/tomoyo/tomoyo.c:246
>  security_path_mknod+0x116/0x180 security/security.c:1072
>  may_o_create fs/namei.c:2905 [inline]
>  lookup_open+0x5ae/0x1320 fs/namei.c:3046
>  open_last_lookups fs/namei.c:3155 [inline]
>  path_openat+0x93c/0x27f0 fs/namei.c:3343
>  do_filp_open+0x192/0x260 fs/namei.c:3373
>  do_sys_openat2+0x585/0x7d0 fs/open.c:1179
>  do_sys_open+0xc3/0x140 fs/open.c:1195
>  do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
>  entry_SYSCALL_64_after_hwframe+0x49/0xb3
> RIP: 0033:0x7efe11e4b6f0
> Code: 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 83 3d 19 30 2c 00 00 75 10 b8 02 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 fe 9d 01 00 48 89 04 24
> RSP: 002b:00007ffc3d0894d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
> RAX: ffffffffffffffda RBX: 000055e78f0bc110 RCX: 00007efe11e4b6f0
> RDX: 00000000000001b6 RSI: 0000000000000241 RDI: 000055e78cf578f5
> RBP: 0000000000000004 R08: 0000000000000004 R09: 0000000000000001
> R10: 0000000000000240 R11: 0000000000000246 R12: 000055e78cf2851e
> R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000000
> Modules linked in:
> ---[ end trace 0a58064de06d50f4 ]---
> RIP: 0010:tomoyo_get_local_path+0x450/0x800 security/tomoyo/realpath.c:165
> Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 b4 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 7f 60 49 8d 7f 28 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 87 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b
> RSP: 0018:ffffc900063d7450 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff88809975c000 RCX: ffffffff8363deda
> RDX: 0000000000000005 RSI: ffffffff8363dee8 RDI: 0000000000000028
> RBP: 1ffff92000c7ae8b R08: ffff8880a47644c0 R09: fffffbfff155a0a2
> R10: ffffffff8aad050f R11: fffffbfff155a0a1 R12: ffff88809df3cfea
> R13: ffff88809df3c000 R14: 0000000000001a2a R15: 0000000000000000
> FS:  00007efe13ce28c0(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000055dfe16c15f8 CR3: 00000000987ed000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> 
> 
> ---
> 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.
> 

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Tetsuo Handa]

On 2020/05/16 0:18, Tetsuo Handa wrote:
> This is
> 
>         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>                 char *ep;
>                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
>                 struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry)); // <= here
> 
>                 if (*ep == '/' && pid && pid ==
>                     task_tgid_nr_ns(current, proc_pidns)) {
> 
> which was added by commit c59f415a7cb6e1e1 ("Use proc_pid_ns() to get pid_namespace from the proc superblock").
> 
> @@ -161,9 +162,10 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
>         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>                 char *ep;
>                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> +               struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
> 
>                 if (*ep == '/' && pid && pid ==
> -                   task_tgid_nr_ns(current, sb->s_fs_info)) {
> +                   task_tgid_nr_ns(current, proc_pidns)) {
>                         pos = ep - 5;
>                         if (pos < buffer)
>                                 goto out;
> 
> Alexey and Eric, any clue?
> 

A similar bug (racing inode destruction with open() on proc filesystem) was fixed as
commit 6f7c41374b62fd80 ("tomoyo: Don't use nifty names on sockets."). Then, it might
not be safe to replace dentry->d_sb->s_fs_info with dentry->d_inode->i_sb->s_fs_info .

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:

> This is
>
>         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>                 char *ep;
>                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
>                 struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry)); // <= here
>
>                 if (*ep == '/' && pid && pid ==
>                     task_tgid_nr_ns(current, proc_pidns)) {
>
> which was added by commit c59f415a7cb6e1e1 ("Use proc_pid_ns() to get pid_namespace from the proc superblock").
>
> @@ -161,9 +162,10 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
>         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>                 char *ep;
>                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> +               struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
>
>                 if (*ep == '/' && pid && pid ==
> -                   task_tgid_nr_ns(current, sb->s_fs_info)) {
> +                   task_tgid_nr_ns(current, proc_pidns)) {
>                         pos = ep - 5;
>                         if (pos < buffer)
>                                 goto out;
>
> Alexey and Eric, any clue?

Looking at the stack backtrace this is happening as part of creating a
file or a device node.  The dentry that is passed in most likely
comes from d_alloc_parallel.  So we have d_inode == NULL.

I want to suggest doing the very simple fix:

-	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
+	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/' && denty->d_inode) {

But I don't know if there are any other security hooks early in lookup,
that could be called for an already existing dentry.

So it looks like we need a version proc_pid_ns that works for a dentry,
or a superblock.

Alex do you think you can code up an patch against my proc-next branch
to fix this?

Eric

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Alexey Gladkov]

On Fri, May 15, 2020 at 01:16:59PM -0500, Eric W. Biederman wrote:
> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> 
> > This is
> >
> >         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
> >                 char *ep;
> >                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> >                 struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry)); // <= here
> >
> >                 if (*ep == '/' && pid && pid ==
> >                     task_tgid_nr_ns(current, proc_pidns)) {
> >
> > which was added by commit c59f415a7cb6e1e1 ("Use proc_pid_ns() to get pid_namespace from the proc superblock").
> >
> > @@ -161,9 +162,10 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
> >         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
> >                 char *ep;
> >                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> > +               struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
> >
> >                 if (*ep == '/' && pid && pid ==
> > -                   task_tgid_nr_ns(current, sb->s_fs_info)) {
> > +                   task_tgid_nr_ns(current, proc_pidns)) {
> >                         pos = ep - 5;
> >                         if (pos < buffer)
> >                                 goto out;
> >
> > Alexey and Eric, any clue?
> 
> Looking at the stack backtrace this is happening as part of creating a
> file or a device node.  The dentry that is passed in most likely
> comes from d_alloc_parallel.  So we have d_inode == NULL.
> 
> I want to suggest doing the very simple fix:
> 
> -	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
> +	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/' && denty->d_inode) {
> 
> But I don't know if there are any other security hooks early in lookup,
> that could be called for an already existing dentry.
> 
> So it looks like we need a version proc_pid_ns that works for a dentry,
> or a superblock.
> 
> Alex do you think you can code up an patch against my proc-next branch
> to fix this?

Sure.

-- 
Rgrds, legion

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Al Viro]

On Sat, May 16, 2020 at 12:36:28AM +0900, Tetsuo Handa wrote:
> On 2020/05/16 0:18, Tetsuo Handa wrote:
> > This is
> > 
> >         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
> >                 char *ep;
> >                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> >                 struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry)); // <= here
> > 
> >                 if (*ep == '/' && pid && pid ==
> >                     task_tgid_nr_ns(current, proc_pidns)) {
> > 
> > which was added by commit c59f415a7cb6e1e1 ("Use proc_pid_ns() to get pid_namespace from the proc superblock").
> > 
> > @@ -161,9 +162,10 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
> >         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
> >                 char *ep;
> >                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> > +               struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
> > 
> >                 if (*ep == '/' && pid && pid ==
> > -                   task_tgid_nr_ns(current, sb->s_fs_info)) {
> > +                   task_tgid_nr_ns(current, proc_pidns)) {
> >                         pos = ep - 5;
> >                         if (pos < buffer)
> >                                 goto out;
> > 
> > Alexey and Eric, any clue?
> > 
> 
> A similar bug (racing inode destruction with open() on proc filesystem) was fixed as
> commit 6f7c41374b62fd80 ("tomoyo: Don't use nifty names on sockets."). Then, it might
> not be safe to replace dentry->d_sb->s_fs_info with dentry->d_inode->i_sb->s_fs_info .

Could you explain why do you want to bother with d_inode() anyway?  Anything that
does dentry->d_inode->i_sb can bloody well use dentry->d_sb.  And that's never
changed over the struct dentry lifetime - ->d_sb is set on allocation and never
modified afterwards.

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Eric W. Biederman]

Al Viro <viro@zeniv.linux.org.uk> writes:

> On Sat, May 16, 2020 at 12:36:28AM +0900, Tetsuo Handa wrote:
>> On 2020/05/16 0:18, Tetsuo Handa wrote:
>> > This is
>> > 
>> >         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>> >                 char *ep;
>> >                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
>> >                 struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry)); // <= here
>> > 
>> >                 if (*ep == '/' && pid && pid ==
>> >                     task_tgid_nr_ns(current, proc_pidns)) {
>> > 
>> > which was added by commit c59f415a7cb6e1e1 ("Use proc_pid_ns() to get pid_namespace from the proc superblock").
>> > 
>> > @@ -161,9 +162,10 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
>> >         if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>> >                 char *ep;
>> >                 const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
>> > +               struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
>> > 
>> >                 if (*ep == '/' && pid && pid ==
>> > -                   task_tgid_nr_ns(current, sb->s_fs_info)) {
>> > +                   task_tgid_nr_ns(current, proc_pidns)) {
>> >                         pos = ep - 5;
>> >                         if (pos < buffer)
>> >                                 goto out;
>> > 
>> > Alexey and Eric, any clue?
>> > 
>> 
>> A similar bug (racing inode destruction with open() on proc filesystem) was fixed as
>> commit 6f7c41374b62fd80 ("tomoyo: Don't use nifty names on sockets."). Then, it might
>> not be safe to replace dentry->d_sb->s_fs_info with dentry->d_inode->i_sb->s_fs_info .
>
> Could you explain why do you want to bother with d_inode() anyway?  Anything that
> does dentry->d_inode->i_sb can bloody well use dentry->d_sb.  And that's never
> changed over the struct dentry lifetime - ->d_sb is set on allocation and never
> modified afterwards.

Wanting to bother with d_inode is a bit strong.

It was just a matter of the helper function proc_pid_ns was built to
work against inodes.  And working with inodes looks reasonable as
in all of the places in proc where it was originally called it had
an inode, and did not have a dentry.

I don't think there was any strong design to it.

Before changing the s_fs_info in proc we found Tomoyo accessing it
without any helpers, and used the helper we had.  Which looked
reasonable.  Now we have discovered it wasn't.

It probably makes most sense just to have the helper go from the
super_block, and not worry about inodes or dentries.

Alexey Gladkov is already looking at fixing this.

Eric

Re: linux-next boot error: general protection fault in tomoyo_get_local_path

[Al Viro]

On Fri, May 15, 2020 at 09:13:57PM +0100, Al Viro wrote:
> On Sat, May 16, 2020 at 12:36:28AM +0900, Tetsuo Handa wrote:
> > On 2020/05/16 0:18, Tetsuo Handa wrote:
[snip]
> > A similar bug (racing inode destruction with open() on proc filesystem) was fixed as
> > commit 6f7c41374b62fd80 ("tomoyo: Don't use nifty names on sockets."). Then, it might
> > not be safe to replace dentry->d_sb->s_fs_info with dentry->d_inode->i_sb->s_fs_info .
> 
> Could you explain why do you want to bother with d_inode() anyway?  Anything that
> does dentry->d_inode->i_sb can bloody well use dentry->d_sb.  And that's never
> changed over the struct dentry lifetime - ->d_sb is set on allocation and never
> modified afterwards.

Incidentally, this
        r_ino = nfs_fhget(ss_mnt->mnt_root->d_inode->i_sb, src_fh, &fattr,
                        NULL);
(in nfs42_ssc_open()) is just plain weird.

	1) d->d_inode->i_sb is equal to d->d_sb
	2) m->mnt_root->d_sb is equal to m->mnt_sb
IOW, the whole thing should be 
        r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, &fattr, NULL);

Moreover,
	server = NFS_SERVER(ss_mnt->mnt_root->d_inode);
in the same function is again too convoluted for no good reason, seeing that
NFS_SERVER(inode) is NFS_SB(inode->i_sb).

Something along the lines of

nfs: don't obfuscate ->mnt_sb as ->mnt_root->d_inode->i_sb

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 8e5d6223ddd3..1e8ca45bc806 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -317,15 +317,14 @@ nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
 {
 	struct nfs_fattr fattr;
 	struct file *filep, *res;
-	struct nfs_server *server;
+	struct super_block *sb = ss_mnt->mnt_sb;
+	struct nfs_server *server = NFS_SB(sb);
 	struct inode *r_ino = NULL;
 	struct nfs_open_context *ctx;
 	struct nfs4_state_owner *sp;
 	char *read_name = NULL;
 	int len, status = 0;
 
-	server = NFS_SERVER(ss_mnt->mnt_root->d_inode);
-
 	nfs_fattr_init(&fattr);
 
 	status = nfs4_proc_getattr(server, src_fh, &fattr, NULL, NULL);
@@ -341,8 +340,7 @@ nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
 		goto out;
 	snprintf(read_name, len, SSC_READ_NAME_BODY, read_name_gen++);
 
-	r_ino = nfs_fhget(ss_mnt->mnt_root->d_inode->i_sb, src_fh, &fattr,
-			NULL);
+	r_ino = nfs_fhget(sb, src_fh, &fattr, NULL);
 	if (IS_ERR(r_ino)) {
 		res = ERR_CAST(r_ino);
 		goto out_free_name;

[PATCH] proc: proc_pid_ns takes super_block as an argument

[Alexey Gladkov]

The proc_pid_ns() can be used for both inode and dentry. To avoid making
two identical functions, change the argument type of the proc_pid_ns().

Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 fs/locks.c                 |  4 ++--
 fs/proc/array.c            |  2 +-
 fs/proc/base.c             | 10 +++++-----
 fs/proc/self.c             |  2 +-
 fs/proc/thread_self.c      |  2 +-
 include/linux/proc_fs.h    |  4 ++--
 kernel/fork.c              |  2 +-
 net/ipv6/ip6_flowlabel.c   |  2 +-
 security/tomoyo/realpath.c |  2 +-
 9 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 399c5dbb72c4..ab702d6efb55 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2823,7 +2823,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 {
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl_pid = locks_translate_pid(fl, proc_pidns);
 	/*
@@ -2901,7 +2901,7 @@ static int locks_show(struct seq_file *f, void *v)
 {
 	struct locks_iterator *iter = f->private;
 	struct file_lock *fl, *bfl;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl = hlist_entry(v, struct file_lock, fl_link);
 
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 8e16f14bb05a..a4d4763731e0 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -728,7 +728,7 @@ static int children_seq_show(struct seq_file *seq, void *v)
 {
 	struct inode *inode = file_inode(seq->file);
 
-	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)));
+	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)->i_sb));
 	return 0;
 }
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5a307b3bb2d1..30c9fceca0b7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -754,7 +754,7 @@ static const struct inode_operations proc_def_inode_operations = {
 static int proc_single_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct pid *pid = proc_pid(inode);
 	struct task_struct *task;
 	int ret;
@@ -1423,7 +1423,7 @@ static const struct file_operations proc_fail_nth_operations = {
 static int sched_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct task_struct *p;
 
 	p = get_proc_task(inode);
@@ -2466,7 +2466,7 @@ static int proc_timers_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 
 	tp->pid = proc_pid(inode);
-	tp->ns = proc_pid_ns(inode);
+	tp->ns = proc_pid_ns(inode->i_sb);
 	return 0;
 }
 
@@ -3377,7 +3377,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct tgid_iter iter;
 	struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
-	struct pid_namespace *ns = proc_pid_ns(file_inode(file));
+	struct pid_namespace *ns = proc_pid_ns(file_inode(file)->i_sb);
 	loff_t pos = ctx->pos;
 
 	if (pos >= PID_MAX_LIMIT + TGID_OFFSET)
@@ -3730,7 +3730,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
 	/* f_version caches the tgid value that the last readdir call couldn't
 	 * return. lseek aka telldir automagically resets f_version to 0.
 	 */
-	ns = proc_pid_ns(inode);
+	ns = proc_pid_ns(inode->i_sb);
 	tid = (int)file->f_version;
 	file->f_version = 0;
 	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
diff --git a/fs/proc/self.c b/fs/proc/self.c
index 309301ac0136..ca5158fa561c 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -12,7 +12,7 @@ static const char *proc_self_get_link(struct dentry *dentry,
 				      struct inode *inode,
 				      struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char *name;
 
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index 2493cbbdfa6f..ac284f409568 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -12,7 +12,7 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
 					     struct inode *inode,
 					     struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	pid_t pid = task_pid_nr_ns(current, ns);
 	char *name;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 2cb424e6f36a..6ec524d8842c 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -202,9 +202,9 @@ int open_related_ns(struct ns_common *ns,
 		   struct ns_common *(*get_ns)(struct ns_common *ns));
 
 /* get the associated pid namespace for a file in procfs */
-static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
+static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
 {
-	return proc_sb_info(inode->i_sb)->pid_ns;
+	return proc_sb_info(sb)->pid_ns;
 }
 
 #endif /* _LINUX_PROC_FS_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4385f3d639f2..e7bdaccad942 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1745,7 +1745,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 	pid_t nr = -1;
 
 	if (likely(pid_has_task(pid, PIDTYPE_PID))) {
-		ns = proc_pid_ns(file_inode(m->file));
+		ns = proc_pid_ns(file_inode(m->file)->i_sb);
 		nr = pid_nr_ns(pid, ns);
 	}
 
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index d64b83e85642..ce4fbba4acce 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -779,7 +779,7 @@ static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
 
-	state->pid_ns = proc_pid_ns(file_inode(seq->file));
+	state->pid_ns = proc_pid_ns(file_inode(seq->file)->i_sb);
 
 	rcu_read_lock_bh();
 	return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 08b096e2f7e3..df4798980416 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -162,7 +162,7 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
 	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
 		char *ep;
 		const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
-		struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
+		struct pid_namespace *proc_pidns = proc_pid_ns(sb);
 
 		if (*ep == '/' && pid && pid ==
 		    task_tgid_nr_ns(current, proc_pidns)) {
-- 
2.25.4

[PATCH v2] proc: proc_pid_ns takes super_block as an argument

[Alexey Gladkov]

The proc_pid_ns() can be used for both inode and dentry. To avoid making
two identical functions, change the argument type of the proc_pid_ns().

Link: https://lore.kernel.org/lkml/c3461e26-1407-2262-c709-dac0df3da2d0@i-love.sakura.ne.jp/
Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 fs/locks.c                 |  4 ++--
 fs/proc/array.c            |  2 +-
 fs/proc/base.c             | 10 +++++-----
 fs/proc/self.c             |  2 +-
 fs/proc/thread_self.c      |  2 +-
 include/linux/proc_fs.h    |  4 ++--
 kernel/fork.c              |  2 +-
 net/ipv6/ip6_flowlabel.c   |  2 +-
 security/tomoyo/realpath.c |  2 +-
 9 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 399c5dbb72c4..ab702d6efb55 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2823,7 +2823,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 {
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl_pid = locks_translate_pid(fl, proc_pidns);
 	/*
@@ -2901,7 +2901,7 @@ static int locks_show(struct seq_file *f, void *v)
 {
 	struct locks_iterator *iter = f->private;
 	struct file_lock *fl, *bfl;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl = hlist_entry(v, struct file_lock, fl_link);
 
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 8e16f14bb05a..a4d4763731e0 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -728,7 +728,7 @@ static int children_seq_show(struct seq_file *seq, void *v)
 {
 	struct inode *inode = file_inode(seq->file);
 
-	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)));
+	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)->i_sb));
 	return 0;
 }
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5a307b3bb2d1..30c9fceca0b7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -754,7 +754,7 @@ static const struct inode_operations proc_def_inode_operations = {
 static int proc_single_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct pid *pid = proc_pid(inode);
 	struct task_struct *task;
 	int ret;
@@ -1423,7 +1423,7 @@ static const struct file_operations proc_fail_nth_operations = {
 static int sched_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct task_struct *p;
 
 	p = get_proc_task(inode);
@@ -2466,7 +2466,7 @@ static int proc_timers_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 
 	tp->pid = proc_pid(inode);
-	tp->ns = proc_pid_ns(inode);
+	tp->ns = proc_pid_ns(inode->i_sb);
 	return 0;
 }
 
@@ -3377,7 +3377,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct tgid_iter iter;
 	struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
-	struct pid_namespace *ns = proc_pid_ns(file_inode(file));
+	struct pid_namespace *ns = proc_pid_ns(file_inode(file)->i_sb);
 	loff_t pos = ctx->pos;
 
 	if (pos >= PID_MAX_LIMIT + TGID_OFFSET)
@@ -3730,7 +3730,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
 	/* f_version caches the tgid value that the last readdir call couldn't
 	 * return. lseek aka telldir automagically resets f_version to 0.
 	 */
-	ns = proc_pid_ns(inode);
+	ns = proc_pid_ns(inode->i_sb);
 	tid = (int)file->f_version;
 	file->f_version = 0;
 	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
diff --git a/fs/proc/self.c b/fs/proc/self.c
index 309301ac0136..ca5158fa561c 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -12,7 +12,7 @@ static const char *proc_self_get_link(struct dentry *dentry,
 				      struct inode *inode,
 				      struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char *name;
 
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index 2493cbbdfa6f..ac284f409568 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -12,7 +12,7 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
 					     struct inode *inode,
 					     struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	pid_t pid = task_pid_nr_ns(current, ns);
 	char *name;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 2cb424e6f36a..6ec524d8842c 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -202,9 +202,9 @@ int open_related_ns(struct ns_common *ns,
 		   struct ns_common *(*get_ns)(struct ns_common *ns));
 
 /* get the associated pid namespace for a file in procfs */
-static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
+static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
 {
-	return proc_sb_info(inode->i_sb)->pid_ns;
+	return proc_sb_info(sb)->pid_ns;
 }
 
 #endif /* _LINUX_PROC_FS_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4385f3d639f2..e7bdaccad942 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1745,7 +1745,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 	pid_t nr = -1;
 
 	if (likely(pid_has_task(pid, PIDTYPE_PID))) {
-		ns = proc_pid_ns(file_inode(m->file));
+		ns = proc_pid_ns(file_inode(m->file)->i_sb);
 		nr = pid_nr_ns(pid, ns);
 	}
 
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index d64b83e85642..ce4fbba4acce 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -779,7 +779,7 @@ static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
 
-	state->pid_ns = proc_pid_ns(file_inode(seq->file));
+	state->pid_ns = proc_pid_ns(file_inode(seq->file)->i_sb);
 
 	rcu_read_lock_bh();
 	return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 08b096e2f7e3..df4798980416 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -162,7 +162,7 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
 	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
 		char *ep;
 		const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
-		struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
+		struct pid_namespace *proc_pidns = proc_pid_ns(sb);
 
 		if (*ep == '/' && pid && pid ==
 		    task_tgid_nr_ns(current, proc_pidns)) {
-- 
2.25.4

Re: [PATCH v2] proc: proc_pid_ns takes super_block as an argument

[Eric W. Biederman]

Alexey Gladkov <gladkov.alexey@gmail.com> writes:

> The proc_pid_ns() can be used for both inode and dentry. To avoid making
> two identical functions, change the argument type of the proc_pid_ns().
>
> Link: https://lore.kernel.org/lkml/c3461e26-1407-2262-c709-dac0df3da2d0@i-love.sakura.ne.jp/
> Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>

So overall this looks good.

However, the description leaves a little bit to be desired as it does
not describe why it is bad to use dentry->d_sb.  A fixes tag would be
nice if for no other reason than to help anyone who decides to backport
this.

And can you please compile test this?

There is a very silly typo in proc that keeps this from compiling.

Thank you,
Eric

> ---
>  fs/locks.c                 |  4 ++--
>  fs/proc/array.c            |  2 +-
>  fs/proc/base.c             | 10 +++++-----
>  fs/proc/self.c             |  2 +-
>  fs/proc/thread_self.c      |  2 +-
>  include/linux/proc_fs.h    |  4 ++--
>  kernel/fork.c              |  2 +-
>  net/ipv6/ip6_flowlabel.c   |  2 +-
>  security/tomoyo/realpath.c |  2 +-
>  9 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 399c5dbb72c4..ab702d6efb55 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2823,7 +2823,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  {
>  	struct inode *inode = NULL;
>  	unsigned int fl_pid;
> -	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
> +	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
>  
>  	fl_pid = locks_translate_pid(fl, proc_pidns);
>  	/*
> @@ -2901,7 +2901,7 @@ static int locks_show(struct seq_file *f, void *v)
>  {
>  	struct locks_iterator *iter = f->private;
>  	struct file_lock *fl, *bfl;
> -	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
> +	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
>  
>  	fl = hlist_entry(v, struct file_lock, fl_link);
>  
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 8e16f14bb05a..a4d4763731e0 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -728,7 +728,7 @@ static int children_seq_show(struct seq_file *seq, void *v)
>  {
>  	struct inode *inode = file_inode(seq->file);
>  
> -	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)));
> +	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)->i_sb));
>  	return 0;
>  }
>  
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 5a307b3bb2d1..30c9fceca0b7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -754,7 +754,7 @@ static const struct inode_operations proc_def_inode_operations = {
>  static int proc_single_show(struct seq_file *m, void *v)
>  {
>  	struct inode *inode = m->private;
> -	struct pid_namespace *ns = proc_pid_ns(inode);
> +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
>  	struct pid *pid = proc_pid(inode);
>  	struct task_struct *task;
>  	int ret;
> @@ -1423,7 +1423,7 @@ static const struct file_operations proc_fail_nth_operations = {
>  static int sched_show(struct seq_file *m, void *v)
>  {
>  	struct inode *inode = m->private;
> -	struct pid_namespace *ns = proc_pid_ns(inode);
> +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
>  	struct task_struct *p;
>  
>  	p = get_proc_task(inode);
> @@ -2466,7 +2466,7 @@ static int proc_timers_open(struct inode *inode, struct file *file)
>  		return -ENOMEM;
>  
>  	tp->pid = proc_pid(inode);
> -	tp->ns = proc_pid_ns(inode);
> +	tp->ns = proc_pid_ns(inode->i_sb);
>  	return 0;
>  }
>  
> @@ -3377,7 +3377,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	struct tgid_iter iter;
>  	struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
> -	struct pid_namespace *ns = proc_pid_ns(file_inode(file));
> +	struct pid_namespace *ns = proc_pid_ns(file_inode(file)->i_sb);
>  	loff_t pos = ctx->pos;
>  
>  	if (pos >= PID_MAX_LIMIT + TGID_OFFSET)
> @@ -3730,7 +3730,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
>  	/* f_version caches the tgid value that the last readdir call couldn't
>  	 * return. lseek aka telldir automagically resets f_version to 0.
>  	 */
> -	ns = proc_pid_ns(inode);
> +	ns = proc_pid_ns(inode->i_sb);
>  	tid = (int)file->f_version;
>  	file->f_version = 0;
>  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> diff --git a/fs/proc/self.c b/fs/proc/self.c
> index 309301ac0136..ca5158fa561c 100644
> --- a/fs/proc/self.c
> +++ b/fs/proc/self.c
> @@ -12,7 +12,7 @@ static const char *proc_self_get_link(struct dentry *dentry,
>  				      struct inode *inode,
>  				      struct delayed_call *done)
>  {
> -	struct pid_namespace *ns = proc_pid_ns(inode);
> +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
>  	pid_t tgid = task_tgid_nr_ns(current, ns);
>  	char *name;
>  
> diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
> index 2493cbbdfa6f..ac284f409568 100644
> --- a/fs/proc/thread_self.c
> +++ b/fs/proc/thread_self.c
> @@ -12,7 +12,7 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
>  					     struct inode *inode,
>  					     struct delayed_call *done)
>  {
> -	struct pid_namespace *ns = proc_pid_ns(inode);
> +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
>  	pid_t tgid = task_tgid_nr_ns(current, ns);
>  	pid_t pid = task_pid_nr_ns(current, ns);
>  	char *name;
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 2cb424e6f36a..6ec524d8842c 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -202,9 +202,9 @@ int open_related_ns(struct ns_common *ns,
>  		   struct ns_common *(*get_ns)(struct ns_common *ns));
>  
>  /* get the associated pid namespace for a file in procfs */
> -static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
> +static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
>  {
> -	return proc_sb_info(inode->i_sb)->pid_ns;
> +	return proc_sb_info(sb)->pid_ns;
>  }
>  
>  #endif /* _LINUX_PROC_FS_H */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4385f3d639f2..e7bdaccad942 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1745,7 +1745,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>  	pid_t nr = -1;
>  
>  	if (likely(pid_has_task(pid, PIDTYPE_PID))) {
> -		ns = proc_pid_ns(file_inode(m->file));
> +		ns = proc_pid_ns(file_inode(m->file)->i_sb);
>  		nr = pid_nr_ns(pid, ns);
>  	}
>  
> diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> index d64b83e85642..ce4fbba4acce 100644
> --- a/net/ipv6/ip6_flowlabel.c
> +++ b/net/ipv6/ip6_flowlabel.c
> @@ -779,7 +779,7 @@ static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
>  {
>  	struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
>  
> -	state->pid_ns = proc_pid_ns(file_inode(seq->file));
> +	state->pid_ns = proc_pid_ns(file_inode(seq->file)->i_sb);
>  
>  	rcu_read_lock_bh();
>  	return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
> index 08b096e2f7e3..df4798980416 100644
> --- a/security/tomoyo/realpath.c
> +++ b/security/tomoyo/realpath.c
> @@ -162,7 +162,7 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
>  	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
>  		char *ep;
>  		const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> -		struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
> +		struct pid_namespace *proc_pidns = proc_pid_ns(sb);
>  
>  		if (*ep == '/' && pid && pid ==
>  		    task_tgid_nr_ns(current, proc_pidns)) {

Re: [PATCH v2] proc: proc_pid_ns takes super_block as an argument

[Alexey Gladkov]

On Mon, May 18, 2020 at 07:08:57AM -0500, Eric W. Biederman wrote:
> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
> 
> > The proc_pid_ns() can be used for both inode and dentry. To avoid making
> > two identical functions, change the argument type of the proc_pid_ns().
> >
> > Link: https://lore.kernel.org/lkml/c3461e26-1407-2262-c709-dac0df3da2d0@i-love.sakura.ne.jp/
> > Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> 
> So overall this looks good.
> 
> However, the description leaves a little bit to be desired as it does
> not describe why it is bad to use dentry->d_sb.  A fixes tag would be
> nice if for no other reason than to help anyone who decides to backport
> this.

OK. I will add it.

> And can you please compile test this?
> 
> There is a very silly typo in proc that keeps this from compiling.

I compiled the kernel with this patch and ran the kernel, but accidentally
did not check children_seq_show(). Sorry.

> Thank you,
> Eric
> 
> > ---
> >  fs/locks.c                 |  4 ++--
> >  fs/proc/array.c            |  2 +-
> >  fs/proc/base.c             | 10 +++++-----
> >  fs/proc/self.c             |  2 +-
> >  fs/proc/thread_self.c      |  2 +-
> >  include/linux/proc_fs.h    |  4 ++--
> >  kernel/fork.c              |  2 +-
> >  net/ipv6/ip6_flowlabel.c   |  2 +-
> >  security/tomoyo/realpath.c |  2 +-
> >  9 files changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 399c5dbb72c4..ab702d6efb55 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2823,7 +2823,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> >  {
> >  	struct inode *inode = NULL;
> >  	unsigned int fl_pid;
> > -	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
> > +	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
> >  
> >  	fl_pid = locks_translate_pid(fl, proc_pidns);
> >  	/*
> > @@ -2901,7 +2901,7 @@ static int locks_show(struct seq_file *f, void *v)
> >  {
> >  	struct locks_iterator *iter = f->private;
> >  	struct file_lock *fl, *bfl;
> > -	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
> > +	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
> >  
> >  	fl = hlist_entry(v, struct file_lock, fl_link);
> >  
> > diff --git a/fs/proc/array.c b/fs/proc/array.c
> > index 8e16f14bb05a..a4d4763731e0 100644
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -728,7 +728,7 @@ static int children_seq_show(struct seq_file *seq, void *v)
> >  {
> >  	struct inode *inode = file_inode(seq->file);
> >  
> > -	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)));
> > +	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)->i_sb));
> >  	return 0;
> >  }
> >  
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 5a307b3bb2d1..30c9fceca0b7 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -754,7 +754,7 @@ static const struct inode_operations proc_def_inode_operations = {
> >  static int proc_single_show(struct seq_file *m, void *v)
> >  {
> >  	struct inode *inode = m->private;
> > -	struct pid_namespace *ns = proc_pid_ns(inode);
> > +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
> >  	struct pid *pid = proc_pid(inode);
> >  	struct task_struct *task;
> >  	int ret;
> > @@ -1423,7 +1423,7 @@ static const struct file_operations proc_fail_nth_operations = {
> >  static int sched_show(struct seq_file *m, void *v)
> >  {
> >  	struct inode *inode = m->private;
> > -	struct pid_namespace *ns = proc_pid_ns(inode);
> > +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
> >  	struct task_struct *p;
> >  
> >  	p = get_proc_task(inode);
> > @@ -2466,7 +2466,7 @@ static int proc_timers_open(struct inode *inode, struct file *file)
> >  		return -ENOMEM;
> >  
> >  	tp->pid = proc_pid(inode);
> > -	tp->ns = proc_pid_ns(inode);
> > +	tp->ns = proc_pid_ns(inode->i_sb);
> >  	return 0;
> >  }
> >  
> > @@ -3377,7 +3377,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
> >  {
> >  	struct tgid_iter iter;
> >  	struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
> > -	struct pid_namespace *ns = proc_pid_ns(file_inode(file));
> > +	struct pid_namespace *ns = proc_pid_ns(file_inode(file)->i_sb);
> >  	loff_t pos = ctx->pos;
> >  
> >  	if (pos >= PID_MAX_LIMIT + TGID_OFFSET)
> > @@ -3730,7 +3730,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> >  	/* f_version caches the tgid value that the last readdir call couldn't
> >  	 * return. lseek aka telldir automagically resets f_version to 0.
> >  	 */
> > -	ns = proc_pid_ns(inode);
> > +	ns = proc_pid_ns(inode->i_sb);
> >  	tid = (int)file->f_version;
> >  	file->f_version = 0;
> >  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> > diff --git a/fs/proc/self.c b/fs/proc/self.c
> > index 309301ac0136..ca5158fa561c 100644
> > --- a/fs/proc/self.c
> > +++ b/fs/proc/self.c
> > @@ -12,7 +12,7 @@ static const char *proc_self_get_link(struct dentry *dentry,
> >  				      struct inode *inode,
> >  				      struct delayed_call *done)
> >  {
> > -	struct pid_namespace *ns = proc_pid_ns(inode);
> > +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
> >  	pid_t tgid = task_tgid_nr_ns(current, ns);
> >  	char *name;
> >  
> > diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
> > index 2493cbbdfa6f..ac284f409568 100644
> > --- a/fs/proc/thread_self.c
> > +++ b/fs/proc/thread_self.c
> > @@ -12,7 +12,7 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
> >  					     struct inode *inode,
> >  					     struct delayed_call *done)
> >  {
> > -	struct pid_namespace *ns = proc_pid_ns(inode);
> > +	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
> >  	pid_t tgid = task_tgid_nr_ns(current, ns);
> >  	pid_t pid = task_pid_nr_ns(current, ns);
> >  	char *name;
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index 2cb424e6f36a..6ec524d8842c 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -202,9 +202,9 @@ int open_related_ns(struct ns_common *ns,
> >  		   struct ns_common *(*get_ns)(struct ns_common *ns));
> >  
> >  /* get the associated pid namespace for a file in procfs */
> > -static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
> > +static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
> >  {
> > -	return proc_sb_info(inode->i_sb)->pid_ns;
> > +	return proc_sb_info(sb)->pid_ns;
> >  }
> >  
> >  #endif /* _LINUX_PROC_FS_H */
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 4385f3d639f2..e7bdaccad942 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1745,7 +1745,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
> >  	pid_t nr = -1;
> >  
> >  	if (likely(pid_has_task(pid, PIDTYPE_PID))) {
> > -		ns = proc_pid_ns(file_inode(m->file));
> > +		ns = proc_pid_ns(file_inode(m->file)->i_sb);
> >  		nr = pid_nr_ns(pid, ns);
> >  	}
> >  
> > diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> > index d64b83e85642..ce4fbba4acce 100644
> > --- a/net/ipv6/ip6_flowlabel.c
> > +++ b/net/ipv6/ip6_flowlabel.c
> > @@ -779,7 +779,7 @@ static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
> >  {
> >  	struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
> >  
> > -	state->pid_ns = proc_pid_ns(file_inode(seq->file));
> > +	state->pid_ns = proc_pid_ns(file_inode(seq->file)->i_sb);
> >  
> >  	rcu_read_lock_bh();
> >  	return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
> > diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
> > index 08b096e2f7e3..df4798980416 100644
> > --- a/security/tomoyo/realpath.c
> > +++ b/security/tomoyo/realpath.c
> > @@ -162,7 +162,7 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
> >  	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
> >  		char *ep;
> >  		const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
> > -		struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
> > +		struct pid_namespace *proc_pidns = proc_pid_ns(sb);
> >  
> >  		if (*ep == '/' && pid && pid ==
> >  		    task_tgid_nr_ns(current, proc_pidns)) {
> 

-- 
Rgrds, legion

Re: [PATCH v2] proc: proc_pid_ns takes super_block as an argument

[Eric W. Biederman]

Alexey Gladkov <gladkov.alexey@gmail.com> writes:

> On Mon, May 18, 2020 at 07:08:57AM -0500, Eric W. Biederman wrote:
>> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
>> 
>> > The proc_pid_ns() can be used for both inode and dentry. To avoid making
>> > two identical functions, change the argument type of the proc_pid_ns().
>> >
>> > Link: https://lore.kernel.org/lkml/c3461e26-1407-2262-c709-dac0df3da2d0@i-love.sakura.ne.jp/
>> > Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
>> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
>> 
>> So overall this looks good.
>> 
>> However, the description leaves a little bit to be desired as it does
>> not describe why it is bad to use dentry->d_sb.  A fixes tag would be
>> nice if for no other reason than to help anyone who decides to backport
>> this.
>
> OK. I will add it.

Thank you.  It really helps to have the full description of why in
the commit comments.

>> And can you please compile test this?
>> 
>> There is a very silly typo in proc that keeps this from compiling.
>
> I compiled the kernel with this patch and ran the kernel, but accidentally
> did not check children_seq_show(). Sorry.

Yes, children_seq_show is behind a sneaky CONFIG option.

Eric

[PATCH v3] proc: proc_pid_ns takes super_block as an argument

[Alexey Gladkov]

syzbot writes:
> general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
> CPU: 0 PID: 6698 Comm: sshd Not tainted 5.7.0-rc5-next-20200515-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> RIP: 0010:tomoyo_get_local_path+0x450/0x800 security/tomoyo/realpath.c:165
> Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 b4 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 7f 60 49 8d 7f 28 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 87 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b
> RSP: 0018:ffffc900063d7450 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff88809975c000 RCX: ffffffff8363deda
> RDX: 0000000000000005 RSI: ffffffff8363dee8 RDI: 0000000000000028
> RBP: 1ffff92000c7ae8b R08: ffff8880a47644c0 R09: fffffbfff155a0a2
> R10: ffffffff8aad050f R11: fffffbfff155a0a1 R12: ffff88809df3cfea
> R13: ffff88809df3c000 R14: 0000000000001a2a R15: 0000000000000000
> FS:  00007efe13ce28c0(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000055e78cf578f5 CR3: 00000000987ed000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  tomoyo_realpath_from_path+0x393/0x620 security/tomoyo/realpath.c:282
>  tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
>  tomoyo_path_number_perm+0x1c2/0x4d0 security/tomoyo/file.c:723
>  tomoyo_path_mknod+0x10d/0x190 security/tomoyo/tomoyo.c:246
>  security_path_mknod+0x116/0x180 security/security.c:1072
>  may_o_create fs/namei.c:2905 [inline]
>  lookup_open+0x5ae/0x1320 fs/namei.c:3046
>  open_last_lookups fs/namei.c:3155 [inline]
>  path_openat+0x93c/0x27f0 fs/namei.c:3343
>  do_filp_open+0x192/0x260 fs/namei.c:3373
>  do_sys_openat2+0x585/0x7d0 fs/open.c:1179
>  do_sys_open+0xc3/0x140 fs/open.c:1195
>  do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
>  entry_SYSCALL_64_after_hwframe+0x49/0xb3
> RIP: 0033:0x7efe11e4b6f0
> Code: 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 83 3d 19 30 2c 00 00 75 10 b8 02 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 fe 9d 01 00 48 89 04 24
> RSP: 002b:00007ffc3d0894d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
> RAX: ffffffffffffffda RBX: 000055e78f0bc110 RCX: 00007efe11e4b6f0
> RDX: 00000000000001b6 RSI: 0000000000000241 RDI: 000055e78cf578f5
> RBP: 0000000000000004 R08: 0000000000000004 R09: 0000000000000001
> R10: 0000000000000240 R11: 0000000000000246 R12: 000055e78cf2851e
> R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000000
> Modules linked in:
> ---[ end trace 0a58064de06d50f4 ]---
> RIP: 0010:tomoyo_get_local_path+0x450/0x800 security/tomoyo/realpath.c:165
> Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 b4 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 7f 60 49 8d 7f 28 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 87 03 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b
> RSP: 0018:ffffc900063d7450 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff88809975c000 RCX: ffffffff8363deda
> RDX: 0000000000000005 RSI: ffffffff8363dee8 RDI: 0000000000000028
> RBP: 1ffff92000c7ae8b R08: ffff8880a47644c0 R09: fffffbfff155a0a2
> R10: ffffffff8aad050f R11: fffffbfff155a0a1 R12: ffff88809df3cfea
> R13: ffff88809df3c000 R14: 0000000000001a2a R15: 0000000000000000
> FS:  00007efe13ce28c0(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000055dfe16c15f8 CR3: 00000000987ed000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

This is happening as part of creating a file or a device node. The
dentry that is passed in most likely comes from d_alloc_parallel and
does not have a d_inode.

Before c59f415a7cb6, Tomoyo received pid_ns from proc's s_fs_info
directly. Since proc_pid_ns() can only work with inode, using it in
the tomoyo_get_local_path() was wrong.

To avoid creating more functions for getting proc_ns would be more
correct to change the argument type of the proc_pid_ns() function.

In this case, Tomoyo can use the existing super_block to get pid_ns.

Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
Fixes: c59f415a7cb6 ("Use proc_pid_ns() to get pid_namespace from the proc superblock")
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 fs/locks.c                 |  4 ++--
 fs/proc/array.c            |  2 +-
 fs/proc/base.c             | 10 +++++-----
 fs/proc/self.c             |  2 +-
 fs/proc/thread_self.c      |  2 +-
 include/linux/proc_fs.h    |  4 ++--
 kernel/fork.c              |  2 +-
 net/ipv6/ip6_flowlabel.c   |  2 +-
 security/tomoyo/realpath.c |  2 +-
 9 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 399c5dbb72c4..ab702d6efb55 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2823,7 +2823,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 {
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl_pid = locks_translate_pid(fl, proc_pidns);
 	/*
@@ -2901,7 +2901,7 @@ static int locks_show(struct seq_file *f, void *v)
 {
 	struct locks_iterator *iter = f->private;
 	struct file_lock *fl, *bfl;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl = hlist_entry(v, struct file_lock, fl_link);
 
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 8e16f14bb05a..043311014db2 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -728,7 +728,7 @@ static int children_seq_show(struct seq_file *seq, void *v)
 {
 	struct inode *inode = file_inode(seq->file);
 
-	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)));
+	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode->i_sb)));
 	return 0;
 }
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5a307b3bb2d1..30c9fceca0b7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -754,7 +754,7 @@ static const struct inode_operations proc_def_inode_operations = {
 static int proc_single_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct pid *pid = proc_pid(inode);
 	struct task_struct *task;
 	int ret;
@@ -1423,7 +1423,7 @@ static const struct file_operations proc_fail_nth_operations = {
 static int sched_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct task_struct *p;
 
 	p = get_proc_task(inode);
@@ -2466,7 +2466,7 @@ static int proc_timers_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 
 	tp->pid = proc_pid(inode);
-	tp->ns = proc_pid_ns(inode);
+	tp->ns = proc_pid_ns(inode->i_sb);
 	return 0;
 }
 
@@ -3377,7 +3377,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct tgid_iter iter;
 	struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
-	struct pid_namespace *ns = proc_pid_ns(file_inode(file));
+	struct pid_namespace *ns = proc_pid_ns(file_inode(file)->i_sb);
 	loff_t pos = ctx->pos;
 
 	if (pos >= PID_MAX_LIMIT + TGID_OFFSET)
@@ -3730,7 +3730,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
 	/* f_version caches the tgid value that the last readdir call couldn't
 	 * return. lseek aka telldir automagically resets f_version to 0.
 	 */
-	ns = proc_pid_ns(inode);
+	ns = proc_pid_ns(inode->i_sb);
 	tid = (int)file->f_version;
 	file->f_version = 0;
 	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
diff --git a/fs/proc/self.c b/fs/proc/self.c
index 309301ac0136..ca5158fa561c 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -12,7 +12,7 @@ static const char *proc_self_get_link(struct dentry *dentry,
 				      struct inode *inode,
 				      struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char *name;
 
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index 2493cbbdfa6f..ac284f409568 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -12,7 +12,7 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
 					     struct inode *inode,
 					     struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	pid_t pid = task_pid_nr_ns(current, ns);
 	char *name;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 2cb424e6f36a..6ec524d8842c 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -202,9 +202,9 @@ int open_related_ns(struct ns_common *ns,
 		   struct ns_common *(*get_ns)(struct ns_common *ns));
 
 /* get the associated pid namespace for a file in procfs */
-static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
+static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
 {
-	return proc_sb_info(inode->i_sb)->pid_ns;
+	return proc_sb_info(sb)->pid_ns;
 }
 
 #endif /* _LINUX_PROC_FS_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4385f3d639f2..e7bdaccad942 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1745,7 +1745,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 	pid_t nr = -1;
 
 	if (likely(pid_has_task(pid, PIDTYPE_PID))) {
-		ns = proc_pid_ns(file_inode(m->file));
+		ns = proc_pid_ns(file_inode(m->file)->i_sb);
 		nr = pid_nr_ns(pid, ns);
 	}
 
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index d64b83e85642..ce4fbba4acce 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -779,7 +779,7 @@ static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
 
-	state->pid_ns = proc_pid_ns(file_inode(seq->file));
+	state->pid_ns = proc_pid_ns(file_inode(seq->file)->i_sb);
 
 	rcu_read_lock_bh();
 	return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 08b096e2f7e3..df4798980416 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -162,7 +162,7 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
 	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
 		char *ep;
 		const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
-		struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
+		struct pid_namespace *proc_pidns = proc_pid_ns(sb);
 
 		if (*ep == '/' && pid && pid ==
 		    task_tgid_nr_ns(current, proc_pidns)) {
-- 
2.25.4

Re: [PATCH v3] proc: proc_pid_ns takes super_block as an argument

[Tetsuo Handa]

I would shorten like below:

syzbot found that

  touch /proc/testfile

causes NULL pointer dereference at tomoyo_get_local_path()
because inode of the dentry is NULL.

Before c59f415a7cb6, Tomoyo received pid_ns from proc's s_fs_info
directly. Since proc_pid_ns() can only work with inode, using it in
the tomoyo_get_local_path() was wrong.

To avoid creating more functions for getting proc_ns, change the
argument type of the proc_pid_ns() function. Then, Tomoyo can use
the existing super_block to get pid_ns.

[PATCH v4] proc: proc_pid_ns takes super_block as an argument

[Alexey Gladkov]

syzbot found that

  touch /proc/testfile

causes NULL pointer dereference at tomoyo_get_local_path()
because inode of the dentry is NULL.

Before c59f415a7cb6, Tomoyo received pid_ns from proc's s_fs_info
directly. Since proc_pid_ns() can only work with inode, using it in
the tomoyo_get_local_path() was wrong.

To avoid creating more functions for getting proc_ns, change the
argument type of the proc_pid_ns() function. Then, Tomoyo can use
the existing super_block to get pid_ns.

Reported-by: syzbot+c1af344512918c61362c@syzkaller.appspotmail.com
Fixes: c59f415a7cb6 ("Use proc_pid_ns() to get pid_namespace from the proc superblock")
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 fs/locks.c                 |  4 ++--
 fs/proc/array.c            |  2 +-
 fs/proc/base.c             | 10 +++++-----
 fs/proc/self.c             |  2 +-
 fs/proc/thread_self.c      |  2 +-
 include/linux/proc_fs.h    |  4 ++--
 kernel/fork.c              |  2 +-
 net/ipv6/ip6_flowlabel.c   |  2 +-
 security/tomoyo/realpath.c |  2 +-
 9 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 399c5dbb72c4..ab702d6efb55 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2823,7 +2823,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 {
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl_pid = locks_translate_pid(fl, proc_pidns);
 	/*
@@ -2901,7 +2901,7 @@ static int locks_show(struct seq_file *f, void *v)
 {
 	struct locks_iterator *iter = f->private;
 	struct file_lock *fl, *bfl;
-	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file));
+	struct pid_namespace *proc_pidns = proc_pid_ns(file_inode(f->file)->i_sb);
 
 	fl = hlist_entry(v, struct file_lock, fl_link);
 
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 8e16f14bb05a..043311014db2 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -728,7 +728,7 @@ static int children_seq_show(struct seq_file *seq, void *v)
 {
 	struct inode *inode = file_inode(seq->file);
 
-	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode)));
+	seq_printf(seq, "%d ", pid_nr_ns(v, proc_pid_ns(inode->i_sb)));
 	return 0;
 }
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5a307b3bb2d1..30c9fceca0b7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -754,7 +754,7 @@ static const struct inode_operations proc_def_inode_operations = {
 static int proc_single_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct pid *pid = proc_pid(inode);
 	struct task_struct *task;
 	int ret;
@@ -1423,7 +1423,7 @@ static const struct file_operations proc_fail_nth_operations = {
 static int sched_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct task_struct *p;
 
 	p = get_proc_task(inode);
@@ -2466,7 +2466,7 @@ static int proc_timers_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 
 	tp->pid = proc_pid(inode);
-	tp->ns = proc_pid_ns(inode);
+	tp->ns = proc_pid_ns(inode->i_sb);
 	return 0;
 }
 
@@ -3377,7 +3377,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct tgid_iter iter;
 	struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
-	struct pid_namespace *ns = proc_pid_ns(file_inode(file));
+	struct pid_namespace *ns = proc_pid_ns(file_inode(file)->i_sb);
 	loff_t pos = ctx->pos;
 
 	if (pos >= PID_MAX_LIMIT + TGID_OFFSET)
@@ -3730,7 +3730,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
 	/* f_version caches the tgid value that the last readdir call couldn't
 	 * return. lseek aka telldir automagically resets f_version to 0.
 	 */
-	ns = proc_pid_ns(inode);
+	ns = proc_pid_ns(inode->i_sb);
 	tid = (int)file->f_version;
 	file->f_version = 0;
 	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
diff --git a/fs/proc/self.c b/fs/proc/self.c
index 309301ac0136..ca5158fa561c 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -12,7 +12,7 @@ static const char *proc_self_get_link(struct dentry *dentry,
 				      struct inode *inode,
 				      struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char *name;
 
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index 2493cbbdfa6f..ac284f409568 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -12,7 +12,7 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
 					     struct inode *inode,
 					     struct delayed_call *done)
 {
-	struct pid_namespace *ns = proc_pid_ns(inode);
+	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	pid_t pid = task_pid_nr_ns(current, ns);
 	char *name;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 2cb424e6f36a..6ec524d8842c 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -202,9 +202,9 @@ int open_related_ns(struct ns_common *ns,
 		   struct ns_common *(*get_ns)(struct ns_common *ns));
 
 /* get the associated pid namespace for a file in procfs */
-static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
+static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
 {
-	return proc_sb_info(inode->i_sb)->pid_ns;
+	return proc_sb_info(sb)->pid_ns;
 }
 
 #endif /* _LINUX_PROC_FS_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4385f3d639f2..e7bdaccad942 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1745,7 +1745,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 	pid_t nr = -1;
 
 	if (likely(pid_has_task(pid, PIDTYPE_PID))) {
-		ns = proc_pid_ns(file_inode(m->file));
+		ns = proc_pid_ns(file_inode(m->file)->i_sb);
 		nr = pid_nr_ns(pid, ns);
 	}
 
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index d64b83e85642..ce4fbba4acce 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -779,7 +779,7 @@ static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
 
-	state->pid_ns = proc_pid_ns(file_inode(seq->file));
+	state->pid_ns = proc_pid_ns(file_inode(seq->file)->i_sb);
 
 	rcu_read_lock_bh();
 	return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 08b096e2f7e3..df4798980416 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -162,7 +162,7 @@ static char *tomoyo_get_local_path(struct dentry *dentry, char * const buffer,
 	if (sb->s_magic == PROC_SUPER_MAGIC && *pos == '/') {
 		char *ep;
 		const pid_t pid = (pid_t) simple_strtoul(pos + 1, &ep, 10);
-		struct pid_namespace *proc_pidns = proc_pid_ns(d_inode(dentry));
+		struct pid_namespace *proc_pidns = proc_pid_ns(sb);
 
 		if (*ep == '/' && pid && pid ==
 		    task_tgid_nr_ns(current, proc_pidns)) {
-- 
2.25.4