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

Bernd Edlinger
2020-04-03T05:09:42Z

On 4/3/20 1:01 AM, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Thu, Apr 2, 2020 at 2:00 PM Bernd Edlinger <bernd.edlinger@hotmail.de> 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.
>

Sorry, I did it as slowly as I could possibly do.
I wanted to wait for you, but....

> 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.
>
> 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.
>

That worries me a bit.
Could you please share details of the failed attempts with us,
Leaning from failures could help us better understand the issue.

> 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.
>

Please elaborate on the details.

> 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.
>

Also here, it might help to make us aware of the problems you face.

I also considered moving all the credentials to the inner block,
but had the impression that is probably a really tough problem instead.

I wondered what happens if a ptraced execve process executes a
suid program that is. Don't you need different credentials
when you are pthraced, I mean, doesn't that override the suid bit,
while when not ptraced, you be root user, and have all the root
powers to load the image in the new vm?

Isn't there a race when execve starts, and ptrace attach happens later?

Thanks
Bernd.

> 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
>