linux-kernel - Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

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: <20120730141638.GA5306@redhat.com>
Date:	Mon, 30 Jul 2012 16:16:38 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Ingo Molnar <mingo@...hat.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Roland McGrath <roland@...k.frob.com>
Subject: Re: [PATCH] uprobes: don't enable/disable signle step if the user
	did it

On 07/30, Ananth N Mavinakayanahalli wrote:
>
> On Thu, Jul 26, 2012 at 05:20:43PM +0200, Sebastian Andrzej Siewior wrote:
> > If someone is using single stepping over uprobe brackpoint then after
> > we pass the uprobe single step, single stepping is disabled and the user
> > who enebaled them in the first place does not know anything about this.
> >
> > This patch avoids enabling / disabling the single step mode if it is
> > already enabled.
>
> This could happen any time 2 different entities call the
> user_(en/dis)able_single_step() helpers on the same thread.

Yes. But nobody except ptrace should do use these helpers, I think.

> Wouldn't the right way to fix it be to teach these helpers
> to honor what the TIF_SINGLESTEP

Well, I think uprobes should not use TIF_SINGLESTEP at all. This
bit is (mostly) needed to handle the stepping over syscall. But
I guess you didn't actually mean TIF_SINGLESTEP...

> flag setting was in the first place?

Perhaps, but I don't think so. If nothing else, we do not want
to add the new counter/whatever in task_struct, while uprobes
already has uprobe_task which can "remember" the state of _TF
bit and more.

And this can't solve other problems. Suppose that gdb does
PTRACE_SINGLESTEP but the original "popf" insn was already replaced
by "int3", this will obviously confuse is_setting_trap_flag().

And we need the additional SIGTRAP from handle_singlestep().
And we have more problems with DEBUGCTLMSR_BTF. And we do
not want access_process_vm() from uprobes code.

So I think we need arch_uprobe_*able_step(struct uprobe_task *utask).
Ignoring all problems except the one this patch tries to fix, x86
can simply do:

	arch_uprobe_enble_step(utask, struct arch_uprobe *auprobe)
	{
		utask->clear_tf =
			!(regs->flags & X86_EFLAGS_TF) &&
			(auprobe->insn != "popf");
		regs->flags |= X86_EFLAGS_TF;
	}

	arch_uprobe_disable_step(utask)
	{
		if (utask->clear_tf)
			regs->flags &= ~X86_EFLAGS_TF;
	}

Fortunately, we can never race with gdb/enable_step(), and we do
not care why X86_EFLAGS_TF was set, and we do not care about
TIF_SINGLESTEP/TIF_FORCED_TF.

However. This all needs more discussion (and help from Roland I guess).

Sebastian, I think your patch is simple and certainly makes the things
better, just it is not correct (you already realized you can't use
uprobe->flags) and it is not arch-friendly.

I'd suggest you to make 2 patches:

	- 1/2 creates arch_uprobe_*_step(...) __weak helpers in
	      kernel/events/uprobes.c which simply call
	      user_*_single_step() and updates the callers

	      Not strictly necessary, but imho makes sense...

	- 2/2 adds the x86 implementation in arch/x86/kernel/uprobes.c
	      which still uses user_*_single_step() but checks
	      TIF_SINGLESTEP. As your patch does, but you should use
	      utask, not uprobe.

IOW, I simply suggest to make your patch x86-specific. Then we
will try to do more fixes/improvements.

Sebastian, Ananth, what do you think?

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/