linux-kernel - Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP

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: <20110713183322.GA12535@redhat.com>
Date:	Wed, 13 Jul 2011 20:33:22 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	vda.linux@...glemail.com, jan.kratochvil@...hat.com,
	pedro@...esourcery.com, indan@....nu, bdonlan@...il.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED
	interaction

Hi Tejun,

On 07/13, Tejun Heo wrote:
>
> Sorry about the long delay.

No, no, you shouldn't use this mantra.  I hold the copyright!

> On Thu, Jul 07, 2011 at 09:03:24PM +0200, Oleg Nesterov wrote:
> > Without the patch it hangs. After the patch SIGSTOP "injected" by the
> > tracer is not ignored and stops the tracee.
>
> I always felt the ability to 'inject' different signal there is rather
> useless and prone to induce weird issues.  It would be better if
> ptrace_signal() is part of signal delivery action after all the checks
> so that the ptracer says whether to proceed with the action or not but
> no more.  Well...

Oh, probably. If the tracer wants a different signr, it can simply do
tkill() + PTRACE_CONT(0). I agree. Although perhaps this is needed for
gdb, I dunno. But we can't change this.

And, I'd like to clarify... It is not that I think it is that important
to ensure PTRACE_CONT(SIGSTOP) will actually stop the tracee if the tracer
changes the original signal. I simply do not know if it is used this way.

The only important thing, imho, the behaviour shouldn't depend on
/dev/random, with this patch it is at least clearly defined.

> > So lets add STOP_DEQUEUED _before_ we report the signal. It has no effect
> > unless sig_kernel_stop() == T after the tracer resumes us, and in the
> > latter case the pending STOP_DEQUEUED means no SIGCONT in between, we
> > should stop.
>
> Anyways, yes, this seems to be a nice improvement but it looks very
> weird (and difficult to comprehend) to be setting STOP_DEQUEUED
> unconditionally in ptrace_signal().

Yeeees, agreed. I even added the comment to explain this weirdness.

> Wouldn't it be better to flip the
> flag so that we have CONT_RECEIVED before doing this?

May be. You know, I thought about this when I did ee77f075
"signal: Turn SIGNAL_STOP_DEQUEUED into GROUP_STOP_DEQUEUED".

Or may be we can simply rename it into STOP_ALLOWED. In this case
we can even set it unconditionally before dequeue_signal().

Anyway, whatever we do, this patch doesn't complicate the
CONT_RECEIVED/STOP_ALLOWED change. Can't we do this later?

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/