linux-kernel - Re: [PATCH 1/1] ptrace: make sure do_wait() won't hang after PTRACE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110217191952.GA13611@redhat.com>
Date:	Thu, 17 Feb 2011 20:19:52 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Denys Vlasenko <vda.linux@...glemail.com>
Cc:	Jan Kratochvil <jan.kratochvil@...hat.com>,
	Tejun Heo <tj@...nel.org>, Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org
Subject: Re: [PATCH 1/1] ptrace: make sure do_wait() won't hang after
	PTRACE_ATTACH

On 02/17, Denys Vlasenko wrote:
>
> Hi Jan,
>
> Thanks for joining!

Yep ;)

> On Wednesday 16 February 2011 22:51, Jan Kratochvil wrote:
>
> > The current upstream GDB trick of
> >   PTRACE_ATTACH
> >   if /proc/PID/status->State: == `T (stopped)'
> >     tgkill(SIGSTOP)
> >     PTRACE_CONT(0)
> >   waitpid->SIGSTOP (or preceded by some other signal but 1x SIGSTOP will come)
>
> I don't fully understand the steps of the trick.

Please see my reply to Jan. In short: ->exit_code can be zero after
attach. This is unlikely case after 90bc8d8b, but still possible.

> I believe there is a proposal to add PTRACE_ATTACH_NOSTOP+PTRACE_INTERRUPT,

(This is more or less orthogonal to the discussed problem, I think
 we should discuss this separately).

> > That is after PTRACE_DETACH(0) the process should remain `T (stopped)'
> > iff the process was `T (stopped)' before PTRACE_ATTACH.
> >  - PTRACE_DETACH(0)       should preserve `T (stopped)'.
>
> I assume you are thinking about PTRACE_ATTACH + wait():SIGSTOP
> + PTRACE_DETACH(0) sequence.

plus it should be stopped before attach, I assume. Otherwise this
not true with the current code.

> It looks logical to use 0 in *this* sequence, but consider
> the following sequence:
>
> ....
> ptrace(PTRACE_CONT, 0)
> waitpid(): got SIGFOO
> ptrace(PTRACE_CONT/SYSCALL/DETACH, 0)

Agreed, and this matches (I hope) my reply.

> Regarding PTRACE_ATTACH + wait():SIGSTOP + PTRACE_DETACH(???)
> sequence. I think this SIGSTOP should be considered by kernel
> not to be a signal delivery ptrace stop - because this SIGSTOP
> is "artificial".

Well, this is another case when we have what we have. Unfortunately
it is not artificial. We can add the new PTRACE_ATTACH_* requests
(and we are going to do this), but I don't think we can change this.

> Actually, if you do waitpid():SIGTRAP + ptrace(PTRACE_DETACH, <anything>),
> <anything> will be ignored, because tracee is not in signal delivery ptrace stop,

Well, to clarify, this depends on SIGTRAP above. But in general yes,
<anything> can be ignored. In particular, it _is_ ignored when the
tracee is stopped (T (stopped)). I do not know was it supposed or not,
but this is how the code currently works.

Perhaps ptrace interface should give more respect to SIGXXX argument,
I have no idea. But let me repeat, imho this is another issue.

> So, the only special trick you seem to want is to make ptrace(PTRACE_DETACH, SIGCONT)
> to forcibly unpause stopped task, even if done from non-signal ptrace stop. Right?
>
> I guess this can be special-cased, but can't the same be trivially achieved by
> kill(SIGCONT) + ptrace(PTRACE_DETACH, SIGCONT)?

Hmm. Agreed. Contrary to what I said in the previous email.

> This will avoid the need to special case in the kernel...

And note that this currently doesn't work anyway.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/