Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
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


  reply	other threads:[~2020-05-15 20:40 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <0000000000002f0c7505a5b0e04c@google.com>
2020-05-15 15:18 ` Tetsuo Handa
2020-05-15 15:36   ` Tetsuo Handa
2020-05-15 20:13     ` Al Viro
2020-05-15 20:36       ` Eric W. Biederman [this message]
2020-05-15 20:56       ` Al Viro
2020-05-15 18:16   ` Eric W. Biederman
2020-05-15 19:25     ` Alexey Gladkov
2020-05-17 17:11     ` [PATCH] proc: proc_pid_ns takes super_block as an argument Alexey Gladkov
2020-05-18 11:17     ` [PATCH v2] " Alexey Gladkov
2020-05-18 12:08       ` Eric W. Biederman
2020-05-18 12:56         ` Alexey Gladkov
2020-05-18 12:57           ` Eric W. Biederman
2020-05-18 15:08     ` [PATCH v3] " Alexey Gladkov
2020-05-18 15:36       ` Tetsuo Handa
2020-05-18 18:07     ` [PATCH v4] " Alexey Gladkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o8qpaqbm.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=gladkov.alexey@gmail.com \
    --cc=jmorris@namei.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-next@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=serge@hallyn.com \
    --cc=sfr@canb.auug.org.au \
    --cc=syzbot+c1af344512918c61362c@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: linux-next boot error: general protection fault in tomoyo_get_local_path' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).