Messages in this thread | | | Subject | Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1 | From | Bernd Edlinger <> | Date | Fri, 3 Apr 2020 07:09:35 +0200 |
| |
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 >
| |