Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAHk-=whJttTNFQn1fMYp91LZ90iHE7B2THZ8NjQ7fBwmWX9k6w@mail.gmail.com>
 <20200411182043.GA3136@redhat.com> <CAHk-=wgwXpKepChGi4ZhQVxZxD0ic8s2CDXvUmqBTMaKGz-fjg@mail.gmail.com>
 <20200412195049.GA23824@redhat.com> <CAHk-=wiDwR+6ugYaKEGHfYteLF+NH5xu=T7uuUTkK9y-hr6zow@mail.gmail.com>
 <AM6PR03MB51708CF53D8A02086427DAC2E4AC0@AM6PR03MB5170.eurprd03.prod.outlook.com>
 <CAHk-=wi_zr9dwX3UBYvgkmm6eVQfRP50orryJ6ZVAxuFqdSG5A@mail.gmail.com>
 <20200428190836.GC29960@redhat.com> <CAHk-=wi03QRcUR1DfbEr+Pw-DAMENzY-FuRcGawtj9p597=p2w@mail.gmail.com>
 <CAG48ez03ABTa-KbCtFHqB1hOT7dgAM96c3kiw-e80B+utSEwYw@mail.gmail.com>
 <CAHk-=wjTLnMuZmBO2foeHhsLAoUTpUi7oBVJ67F4XKB+tdEDbQ@mail.gmail.com>
 <CAG48ez3EQOvdbzu9aO-cEAJwF_=fJzn1Cg0LMs3ruc=5r1ie5w@mail.gmail.com>
 <CAHk-=whTgFbjGTP=CqMWs_LOkY7bWvLQGYKwKx86amdbMovAkw@mail.gmail.com>
 <CAG48ez2-Nu2ALN6VEUZL-prtR_Kk8QYBHcnvuh0aU2e4zf37RA@mail.gmail.com>
 <CAHk-=wh=G47oD2F1CgOrvGFbEPh2ddMKLV4_wV_bs6S=98aZ5A@mail.gmail.com> <AM6PR03MB5170A6AA240D2E8F5E88B911E4AD0@AM6PR03MB5170.eurprd03.prod.outlook.com>
In-Reply-To: <AM6PR03MB5170A6AA240D2E8F5E88B911E4AD0@AM6PR03MB5170.eurprd03.prod.outlook.com>
From:   Jann Horn <jannh@google.com>
Date:   Wed, 29 Apr 2020 21:26:49 +0200
Message-ID: <CAG48ez07GSXAvqv03-1w=CqedgwmUis5=8oaQsfnFXkPpuN4_g@mail.gmail.com>
Subject: Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1
To:     Bernd Edlinger <bernd.edlinger@hotmail.de>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Waiman Long <longman@redhat.com>,
        Ingo Molnar <mingo@kernel.org>, Will Deacon <will@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alexey Gladkov <gladkov.alexey@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Apr 29, 2020 at 9:23 PM Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 4/29/20 7:58 PM, Linus Torvalds wrote:
> > On Tue, Apr 28, 2020 at 4:36 PM Jann Horn <jannh@google.com> wrote:
> >>
> >> On Wed, Apr 29, 2020 at 12:14 AM Linus Torvalds
> >> <torvalds@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/AM6PR03MB5170B06F3A2B75EFB98D071AE4E60@AM6PR03MB5170.eurprd03.prod.outlook.com/>?