Messages in this thread | | | Date | Mon, 11 May 2020 14:55:27 -0700 | From | Kees Cook <> | Subject | Re: [PATCH 3/5] exec: Remove recursion from search_binary_handler |
| |
On Mon, May 11, 2020 at 09:33:21AM -0500, Eric W. Biederman wrote: > Linus Torvalds <torvalds@linux-foundation.org> writes: > > > On Sat, May 9, 2020 at 9:30 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> > >> Wouldn't this change cause > >> > >> if (fd_binary > 0) > >> ksys_close(fd_binary); > >> bprm->interp_flags = 0; > >> bprm->interp_data = 0; > >> > >> not to be called when "Search for the interpreter" failed? > > > > Good catch. We seem to have some subtle magic wrt the fd_binary file > > descriptor, which depends on the recursive behavior. > > Yes. I Tetsuo I really appreciate you noticing this. This is exactly > the kind of behavior I am trying to flush out and keep from being > hidden. > > > I'm not seeing how to fix it cleanly with the "turn it into a loop". > > Basically, that binfmt_misc use-case isn't really a tail-call. > > I have reservations about installing a new file descriptor before > we process the close on exec logic and the related security modules > closing file descriptors that your new credentials no longer give > you access to logic.
Hm, this does feel odd. In looking at this, it seems like this file never gets close-on-exec set, and doesn't have its flags changed from its original open: .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, only the UMH path through exec doesn't explicitly open a file by name from what I can see, so we'll only have these flags.
> I haven't yet figured out how opening a file descriptor during exec > should fit into all of that. > > What I do see is that interp_data is just a parameter that is smuggled > into the call of search binary handler. And the next binary handler > needs to be binfmt_elf for it to make much sense, as only binfmt_elf > (and binfmt_elf_fdpic) deals with BINPRM_FLAGS_EXECFD. > > So I think what needs to happen is to rename bprm->interp_data to > bprm->execfd, remove BINPRM_FLAGS_EXECFD and make closing that file > descriptor free_bprm's responsiblity.
Yeah, I would agree. As far as the close handling, I don't think there is a difference here: it interp_data was closed on the binfmt_misc.c error path, and in the new world it would be the exec error path -- both would be under the original credentials.
> I hope such a change will make it easier to see all of the pieces that > are intereacting during exec.
Right -- I'm not sure which piece should "consume" bprm->execfd though, which I think is what you're asking next...
> I am still asking: is the installation of that file descriptor useful if > it is not exported passed to userspace as an AT_EXECFD note? > > I will dig in and see what I can come up with.
Should binfmt_misc do the install, or can the consuming binfmt do it? i.e. when binfmt_elf sees bprm->execfd, does it perform the install instead?
-- Kees Cook
| |