linux-kernel - Re: [PATCH 2/4] MIPS Kprobes: Deny probes on ll/sc instructions

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]

Date:	Tue, 8 Nov 2011 15:26:42 -0800 (PST)
From:	Victor Kamensky <kamensky@...co.com>
To:	David Daney <david.daney@...ium.com>
cc:	"manesoni@...co.com" <manesoni@...co.com>,
	Ralf Baechle <ralf@...ux-mips.org>,
	"ananth@...ibm.com" <ananth@...ibm.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>
Subject: Re: [PATCH 2/4] MIPS Kprobes: Deny probes on ll/sc instructions

Hi David,

Thank you for your feedback! Please see response inline.

On Tue, 8 Nov 2011, David Daney wrote:

> On 11/08/2011 09:05 AM, Maneesh Soni wrote:
> >
> > From: Maneesh Soni<manesoni@...co.com>
> >
> > Deny probes on ll/sc instructions for MIPS kprobes
> >
> > As ll/sc instruction are for atomic read-modify-write operations, allowing
> > probes on top of these insturctions is a bad idea.
> >
>
> s/insturctions/instructions/
>
> Not only is it a bad idea, it will probably make them fail 100% of the time.
>
> It is also an equally bad idea to place a probe between any LL and SC
> instructions.  How do you prevent that?

As per below code comment we don't prevent that. There is no way to do
that.

> If you cannot prevent probes between LL and SC, why bother with this at all?

We just trying to be a bit practical here. It is better than nothing,
right? Breakpoint on top of ll/sc simply won't work and that is the fact.
Breakpoint between related pair of ll/sc won't work too, but nothing we
can do about that.

We run into this situation with SystemTap function wildcard based tracing,
as per attached unit test note. Basically SystemTap wildcard probe picked
inline assembler function which had first 'll' instruction and as result
it was spinning there till SystemTap module reached threshold and shut
itself off so code proceeded after that. Note attached unit test presents
simplified version of real issue we run into, so it may look a bit
artificial. Note it is highly unlikely that SystemTap wildcard tracing
would pick up anything between related pair of ll/sc. In order to have
breakpoint between ll/sc user had use 'statement' SystemTap directive and
it would be specifically targeting given address and therefore could be
removed easily. In case of wildcard tracing there is no easy workaround
for user to drop functions that start with 'll'.

Ideally we would want to push this check into SystemTap compile time,
along with check for branch delay slot instruction, but currently in
SystemTap there is no infrastructure that would check instruction opcode
at compile time. I believe that disallowed instruction check in SystemTap
compiler is missing for any CPU. Adding it would be small feature that we
did not have time to pursue.

Thanks,
Victor

> David Daney
>
> > Signed-off-by: Victor Kamensky<kamensky@...co.com>
> > Signed-off-by: Maneesh Soni<manesoni@...co.com>
> > ---
> >   arch/mips/kernel/kprobes.c |   31 +++++++++++++++++++++++++++++++
> >   1 files changed, 31 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
> > index 9fb1876..0ab1a5f 100644
> > --- a/arch/mips/kernel/kprobes.c
> > +++ b/arch/mips/kernel/kprobes.c
> > @@ -113,6 +113,30 @@ insn_ok:
> >   	return 0;
> >   }
> >
> > +/*
> > + * insn_has_ll_or_sc function checks whether instruction is ll or sc
> > + * one; putting breakpoint on top of atomic ll/sc pair is bad idea;
> > + * so we need to prevent it and refuse kprobes insertion for such
> > + * instructions; cannot do much about breakpoint in the middle of
> > + * ll/sc pair; it is upto user to avoid those places
> > + */
> > +static int __kprobes insn_has_ll_or_sc(union mips_instruction insn)
> > +{
> > +	int ret = 0;
> > +
> > +	switch (insn.i_format.opcode) {
> > +	case ll_op:
> > +	case lld_op:
> > +	case sc_op:
> > +	case scd_op:
> > +		ret = 1;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	return ret;
> > +}
> > +
> >   int __kprobes arch_prepare_kprobe(struct kprobe *p)
> >   {
> >   	union mips_instruction insn;
> > @@ -121,6 +145,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> >
> >   	insn = p->addr[0];
> >
> > +	if (insn_has_ll_or_sc(insn)) {
> > +		pr_notice("Kprobes for ll and sc instructions are not"
> > +			  "supported\n");
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >   	if (insn_has_delayslot(insn)) {
> >   		pr_notice("Kprobes for branch and jump instructions are not"
> >   			  "supported\n");
>
>
View attachment "kprobes_ll_sc_testing.txt" of type "TEXT/PLAIN" (5985 bytes)