On Wed, Jan 24, 2024 at 10:27:03AM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 09:27, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
