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

From: ebiederm@xmission.com (Eric W. Biederman)
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Alexey Gladkov <gladkov.alexey@gmail.com>,
	syzbot <syzbot+c1af344512918c61362c@syzkaller.appspotmail.com>,
	jmorris@namei.org, linux-kernel@vger.kernel.org,
	linux-next@vger.kernel.org,
	linux-security-module@vger.kernel.org, serge@hallyn.com,
	sfr@canb.auug.org.au, syzkaller-bugs@googlegroups.com,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: linux-next boot error: general protection fault in tomoyo_get_local_path
Date: Fri, 15 May 2020 15:36:13 -0500	[thread overview]
Message-ID: <87o8qpaqbm.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20200515201357.GG23230@ZenIV.linux.org.uk> (Al Viro's message of "Fri, 15 May 2020 21:13:57 +0100")

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