Received-SPF: pass (google.com: domain of linux-kernel+bounces-37567-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 24 Jan 2024 11:02:34 -0800
From: Kees Cook <keescook@chromium.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Kentaro Takeda <takedakn@nttdata.co.jp>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	John Johansen <john.johansen@canonical.com>,
	Paul Moore <paul@paul-moore.com>,
	Kevin Locke <kevin@kevinlocke.name>,
	Josh Triplett <josh@joshtriplett.org>,
	Mateusz Guzik <mjguzik@gmail.com>,
	Al Viro <viro@zeniv.linux.org.uk>, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from
 virt-aa-helper
Message-ID: <202401241058.16E3140@keescook>
References: <ZbE4qn9_h14OqADK@kevinlocke.name>
 <202401240832.02940B1A@keescook>
 <CAHk-=wgJmDuYOQ+m_urRzrTTrQoobCJXnSYMovpwKckGgTyMxA@mail.gmail.com>
 <CAHk-=wijSFE6+vjv7vCrhFJw=y36RY6zApCA07uD1jMpmmFBfA@mail.gmail.com>
 <CAHk-=wiZj-C-ZjiJdhyCDGK07WXfeROj1ACaSy7OrxtpqQVe-g@mail.gmail.com>
 <202401240916.044E6A6A7A@keescook>
 <CAHk-=whq+Kn-_LTvu8naGqtN5iK0c48L1mroyoGYuq_DgFEC7g@mail.gmail.com>
 <CAHk-=whDAUMSPhDhMUeHNKGd-ZX8ixNeEz7FLfQasAGvi_knDg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=whDAUMSPhDhMUeHNKGd-ZX8ixNeEz7FLfQasAGvi_knDg@mail.gmail.com>

On Wed, Jan 24, 2024 at 10:27:03AM -0800, Linus Torvalds wrote:
> On Wed, 24 Jan 2024 at 09:27, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > IOW, I think the goal here should be "minimal fix" followed by "remove
> > that horrendous thing".
> 
> Ugh. The tomoyo use is even *more* disgusting, in how it uses it for
> "tomoyo_domain()" entirely independently of even the ->file_open()
> callback.

Yeah, I just sent a similar email.

> So for tomoyo, it's not about the file open, it's about
> tomoyo_cred_prepare() and friends.

Yeah, it looks like it should happily follow cred lifetime, but I
haven't fully convinced myself.

> So the patch I posted probably fixes apparmor, but only breaks tomoyo
> instead, because tomoyo really does seem to use it around the whole
> security_bprm_creds_for_exec() thing.
> 
> Now, tomoyo *also* uses it for the file_open() callback, just to confuse things.
> 
> IOW, I think the right thing to do is to split this in two:
> 
>  - leave the existing ->in_execve for the bprm_creds dance in
> boprm_execve(). Horrendous and disgusing.

Agreed.

>  - the ->file_open() thing is changed to check file->f_flags

Agreed. (And I've tested this for AppArmor now. I can confirm the
failure case -- it's only for profile transitions, which is why I didn't
see it originally in testing.

> IOW, I think the patch I posted earlier - and Kees' version of the
> same thing - is just broken. This attached patch might work.

Yup. Should I post a formal patch, or do you want to commit what you've
got (with the "file" -> "f" fix)?

-Kees

-- 
Kees Cook