From: tixy@yxit.co.uk (Tixy)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] Reject kprobes when Rn==15 and writeback is set
Date: Wed, 30 Mar 2011 20:39:09 +0100 [thread overview]
Message-ID: <1301513949.2488.229.camel@computer2.home> (raw)
In-Reply-To: <alpine.LFD.2.00.1103301328280.28032@xanadu.home>
On Wed, 2011-03-30 at 13:59 -0400, Nicolas Pitre wrote:
> On Wed, 30 Mar 2011, Tixy wrote:
>
> > On Wed, 2011-03-30 at 16:42 +0300, Viktor Rosendahl wrote:
> > > On 03/29/2011 09:44 PM, ext Nicolas Pitre wrote:
> > > > On Tue, 29 Mar 2011, Russell King - ARM Linux wrote:
> > > >
> > > >> On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
> > > >>> Sorry, I meant r15-indexed with a write back.
> > > >>
> > > >> I believe that's unpredictable. So actually you can make kprobes do
> > > >> anything you like with it - no one should ever generate an instruction
> > > >> which read-modify-writes the PC.
> > > >
> > > > Indeed. Hence my suggestion to simply refuse and abort the placement of
> > > > a probe on such instructions and keep the actual emulation code free of
> > > > tests for those odd cases. In practice this shouldn't affect anyone.
> > > >
> > >
> > > Here is a patch for implementing the rejection of probes on those instructions,
> > > with Rn == 15 and writeback enabled. Those previous patches are still
> > > required, since they fix the emulation of the fully legal instructions where
> > > Rn == 15 and writeback isn't enabled.
> >
> > I think this could be a slippery slope, what about the other dubious
> > combinations, like writeback when Rn==Rt, or when Rm==pc? By the same
> > rationale we should check for those to.
> >
> > If we start littering the code with all these extra checks we risk
> > introducing bugs and making the code more difficult to maintain.
> >
> > In my opinion we should not add any extra code to handle instructions
> > combinations that the ARM ARM says are UNPREDICTABLE, or have fields
> > which are SBZ/SBO. The toolchain shouldn't ever generate these bad
> > instructions in which case the extra kprobes code is redundant.
>
> There are two categories to consider for such issues:
>
> 1) What to do with any writeback to pc. This is really important
> because this directly affects the running thread and determines where
> execution will resume once the probe is done. Any dubious case
> should be carefully intercepted, otherwise this may become a
> potential security risk. So, given that this has to be handled
> somehow, I think it is best to simply refuse the placement of a probe
> on a dubious instruction rather than working around the dubiousness in
> the actual emulation code.
>
> 2) What to do with any other undefined combinations. Well, anything
> else that is defined as unpredictable by the ARM ARM which doesn't
> involve writing the pc should simply pass through the emulation code
> without incuring any additional overhead or special care. By definition,
> if the result is unpredictable, we can emulate it the way we wish and
> any discrepancy with native hardware result is unimportant.
>
> In the first category can be found things like:
>
> ldmdb pc!, {r0, r1, r2}
>
> In the second category would be:
>
> ldmdb pc, {r0, r1, r2}
>
> We can safely ignore the second case, but the first case clearly has the
> potential for trouble if mis-emulated. And trying to correctly emulate
> any of those cases is worthless.
I don't think it's quite as black and white. In both cases the kernel
was executing an illegal instruction before we inserted the probe, so we
are probably already in the territorial of "affects the running thread"
and security issues.
However, if the simple rule we need to follow is "avoid ambiguous writes
to PC" then I won't put up any more fight :-) (Expecially as I just
checked normal execution of one writeback to PC instruction an found
that PC was unaffected ;-)
I'll get on with writing test code so that we can find bugs when probing
legal instructions...
--
Tixy
next prev parent reply other threads:[~2011-03-30 19:39 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-25 17:01 [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl
2011-03-25 21:19 ` Tixy
2011-03-28 15:56 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl
2011-03-28 22:39 ` Nicolas Pitre
2011-03-29 11:26 ` Viktor Rosendahl
2011-03-29 16:55 ` Nicolas Pitre
2011-03-29 18:31 ` Russell King - ARM Linux
2011-03-29 18:44 ` Nicolas Pitre
2011-03-30 13:42 ` [PATCH] Reject kprobes when Rn==15 and writeback is set Viktor Rosendahl
2011-03-30 15:52 ` Tixy
2011-03-30 16:46 ` Viktor Rosendahl
2011-03-30 17:20 ` Tixy
2011-03-30 17:59 ` Nicolas Pitre
2011-03-30 19:39 ` Tixy [this message]
2011-03-30 20:48 ` Nicolas Pitre
2011-03-30 14:09 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl
2011-03-29 12:55 ` Tixy
2011-03-29 13:46 ` Viktor Rosendahl
2011-03-29 14:03 ` Tixy
2011-03-29 17:07 ` Nicolas Pitre
2011-03-28 16:27 ` [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl
2011-03-29 9:12 ` Tixy
2011-03-26 2:03 ` Nicolas Pitre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1301513949.2488.229.camel@computer2.home \
--to=tixy@yxit.co.uk \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).