Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

[Bernd Edlinger]

On 4/29/20 9:26 PM, Jann Horn wrote:
> On Wed, Apr 29, 2020 at 9:23 PM Bernd Edlinger
> <bernd.edlinger@xxxxxxxxxx> wrote:
>> On 4/29/20 7:58 PM, Linus Torvalds wrote:
>>> On Tue, Apr 28, 2020 at 4:36 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
>>>>
>>>> On Wed, Apr 29, 2020 at 12:14 AM Linus Torvalds
>>>> <torvalds@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/>?
> 

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.