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

[Bernd Edlinger]

On 4/3/20 1:01 AM, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> 
>> On Thu, Apr 2, 2020 at 2:00 PM Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> wrote:
>>>
>>> There are two more patches, which might be of interest for you, just to
>>> make the picture complete.
>>> It is not clear if we go that way, or if Eric has a yet better idea.
>>>
>>> [PATCH v7 15/16] exec: Fix dead-lock in de_thread with ptrace_attach
>>> https://www.spinics.net/lists/kernel/msg3459067.html
>>
>> There is no way I would ever take that patch.
>>
>> The amount of confusion in that patch is not acceptable. Randomly
>> unlocking the new lock?
>>
>> That code makes everything worse, it's completely incomprehensible,
>> the locking rules make no sense ahwt-so-ever.
>>
>> I'm seriously starting to feel like I should not have pulled this
>> code, because the future looks _worse_ than what we used to have.
>>
>> No. No no no. Eric, this is not an acceptable direction.
> 
> That is not the direction I intend to take either.
> 
> I was hoping I could put off replying to this thread for a bit because
> I only managed to get 4 hours of sleep last night and I am not as alert
> to technical details as I would like to be.
> 
> Long story short:
> 
> The exec_update_mutex is to be a strict subset of today's
> cred_guard_mutex.  I tried to copy cred_guard_mutex's unlock style so
> that was obvious and that turns out was messier than I intended.
> 
> I thought the changes to the individual locking changes were
> sufficiently unsubtle that they did not need my personal attention.
> Especially as they are just a substitution of one lock for another
> with a slightly smaller scope.
> 
> I started working on the the series of changes that reorganizes
> the changes in exec.
> 
> It was reported that something had gone wrong with my introduction
> of exec_update_mutex and I pulled it from linux-next.
> 
> By the time I was ready to start putting humpty dumpty back together
> again Bernd had collected everything up and had it working.  I had seen
> that he had been given the feedback about better change descriptions.
> 
> I had looked at the code of his patches earlier and the basic changes
> were trivial.
> 
> Since I thought I already knew what was in the patches and the worst
> problem was the missing unlock of cred_guard_mutex, and I know Bernd's
> patches had been tested I applied them.  I missed that Bernd had added
> the exec_mmap_called flag into my patch.  I thought he had only added
> the missing unlock.
> 

Hi Eric,

oh, sorry for that, that was requested in the peer review, I could not
get a patch approved that does not have such a boolean, that simplified
the error handling.

Actually I had sent you an e-mail with that patch 24H before I posted
the update, then Greg asked me to re-post the whole series, that
took at least another two days, so at that time I was seriously
concerned how you are doing, since I head nothing from you about the
updated patch with the exec_mmap_called.

Linus that is not the boolean I was talking in the other mail.
That boolean is called unsafe_execve_in_progres.

So, and now I also try to get some sleep....


Thanks
Bernd.

> I spotted the weirdness in unlocking exec_update_mutex, and because it
> does fix a real world deadlock with ptrace I did not back it out from my
> tree.
> 
> I have been much laxer on the details than I like to be my apologies.
> 
> The plan is:
> 	exec_udpate_mutex will cover just the subset of cred_guard_mutex
>         after the point of no return, and after we do any actions that
> 	might block waiting for userspace to do anything.
> 
> 	So exec_update_mutex will just cover those things that exec
>         is updating, so if you want an atomic snapshot of them
>         plus the appropriate struct cred you can grab exec_update_mutex.
>         
> 	I added a new mutex instead of just fixing cred_guard_mutex so
>         that we can update or revert the individual code paths if it
>         is found that something is wrong.
> 
> 	The cred_guard_mutex also prevents other tasks from starting
>         to ptrace the task that is exec'ing, and other tasks from
>         setting no_new_privs on the task that is exec'ing.
> 
>         My plan is to carefully refactor the code so it can perform
>         both the ptrace and no_new_privs checks after the point of
>         no return.
> 
> I have uncovered all kinds of surprises while working in that direction
> and I realize it is going to take a very delicate and careful touch to
> achieve my goal.
> 
> There are silly things like normal linux exec when you are ptraced and
> exec changes the credentials the ordinary code will continue with the
> old credentials, but the an LSM enabled your process is likely to be
> killed instead.
> 
> There is the personal mind blowing scenario where selinux will increase
> your credentials upon exec but if a magic directive is supplied in it's
> rules will avoid setting AT_SECURE, so that userspace won't protect
> itself from hostile takeover from the pre credential change environment.
> Much to my surprise "noatsecure" is a known and documented feature of
> selinux.  I am not certain but I think I even spotted it in use on
> production.
> 
> I will catch up on my sleep before I allow any more changes, and I will
> see replacing the called_exec_mmap flag with something saner.
> 
> Eric
>