On 4/29/20 9:26 PM, Jann Horn wrote: > On Wed, Apr 29, 2020 at 9:23 PM Bernd Edlinger > <bernd.edlin...@hotmail.de> wrote: >> On 4/29/20 7:58 PM, Linus Torvalds wrote: >>> On Tue, Apr 28, 2020 at 4:36 PM Jann Horn <ja...@google.com> wrote: >>>> >>>> On Wed, Apr 29, 2020 at 12:14 AM Linus Torvalds >>>> <torva...@linux-foundation.org> wrote: >>>>> >>>>> - we move check_unsafe_exec() down. As far as I can tell, there's no >>>>> reason it's that early - the flags it sets aren't actually used until >>>>> when we actually do that final set_creds.. >>>> >>>> Right, we should be able to do that stuff quite a bit later than it >>>> happens now. >>> >>> Actually, looking at it, this looks painful for multiple reasons. >>> >>> The LSM_UNSAFE_xyz flags are used by security_bprm_set_creds(), which >>> when I traced it through, happened much earlier than I thought. Making >>> things worse, it's done by prepare_binprm(), which also potentially >>> gets called from random points by the low-level binfmt handlers too. >>> >>> And we also have that odd "fs->in_exec" flag, which is used by thread >>> cloning and io_uring, and I'm not sure what the exact semantics are. >>> >>> I'm _almost_ inclined to say that we should just abort the execve() >>> entirely if somebody tries to attach in the middle. >>> >>> IOW, get rid of the locking, and replace it all just with a sequence >>> count. Make execve() abort if the sequence count has changed between >>> loading the original creds, and having installed the new creds. >>> >>> You can ptrace _over_ an execve, and you can ptrace _after_ an >>> execve(), but trying to attach just as we execve() would just cause >>> the execve() to fail. >>> >>> We could maybe make it conditional on the credentials actually having >>> changed at all (set another flag in bprm_fill_uid()). So it would only >>> fail for the suid exec case. >>> >>> Because honestly, trying to ptrace in the middle of a suid execve() >>> sounds like an attack, not a useful thing. >>> >> >> I think the use case where a program attaches and detaches many >> processes at a high rate, is either an attack or a very aggressive >> virus checker, fixing a bug that prevents an attack is not a good >> idea, but fixing a bug that would otherwise break a virus checker >> would be a good thing. >> >> By the way, all other attempts to fix it look much more dangerous >> than my initially proposed patch, you know the one you hated, but >> it does work and does not look overly complicated either. >> >> What was the reason why that cannot be done this way? > > I'm not sure which patch you're talking about - I assume you don't > mean > <https://lore.kernel.org/lkml/am6pr03mb5170b06f3a2b75efb98d071ae4...@am6pr03mb5170.eurprd03.prod.outlook.com/>? >
No, I meant: [PATCH v7 15/16] exec: Fix dead-lock in de_thread with ptrace_attach https://marc.info/?l=linux-kernel&m=158559277631548&w=2 and [PATCH v6 16/16] doc: Update documentation of ->exec_*_mutex https://marc.info/?l=linux-kernel&m=158559277631548&w=2 I think that was the latest version, but this had several iterations already. Thanks Bernd.
