Re: [PATCHv11 2.6.36-rc2-tip 4/15] 4: uprobes: x86 specific functions for user space breakpointing.

[Andi Kleen <andi@firstfloor.org>]

On Fri, 3 Sep 2010 23:18:32 +0530
Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

[cutting down cc list]

> > 
> > One general comment here: since with uprobes the instruction
> > decoder becomes security critical did you do any fuzz tests
> > on it (e.g. like using it on crashme or on code that has 
> > been corrupted with a few bitflips) ?
> 
> I havent tried any fuzz tests with the instruction decoder. But I am
> not sure if Masami has tried that out some of these. 
> One question: Do you want to test uprobes with crashme or test
> instruction decoder with crashme.

Ideally both, but as a minimum the part that is exposed
to user space, that is uprobes.

BTW if you test it I would test it both with real crashme
and varying legal code that just has a few bits flipped.

> > > +#ifdef CONFIG_X86_32
> > > +#define is_32bit_app(tsk) 1
> > > +#else
> > > +#define is_32bit_app(tsk) (test_tsk_thread_flag(tsk, TIF_IA32))
> > > +#endif
> > 
> > This probably should be elsewhere.
> 
> Would this fit in x86 Instruction decoder?

compat.h probably. 


> Okay, I can move the printk to the caller, I will try to shorten the
> message, Would something like "uprobes: no support for 2-byte
> opcode 0x0f 0x%2" look fine?

Yes that's fine. Optionally you could supply a short
script like scripts/decodecode that feeds it through objdump -d
This might need dumping a few more bytes.


> > This check is not fully correct because it's valid to have
> > 32bit code in 64bit programs and vice versa.  The only good
> > way to check that is to look at the code segment at runtime
> > though (and it gets complicated if you want to handle LDTs,
> > but that could be optional). May be difficult to do though.
> 
> validate_insn_32bit is able to identify all valid instructions in a 32
> bit app and validate_insn_64bits is a superset of
> validate_insn_32bits; i.e it considers valid 32 bit codes as valid
> too.

How can this be? e.g. 32bit has 1 byte INC/DEC but on 64bit
these are REX prefixes and can be in front of nearly anything.
So a super set cannot be correct. It has to be either / or.

> 
> Did you get a chance to look at
> validate_insn_32bit/validate_insn_64bits? If you feel that
> validate_insn_32bit/validate_insn_64bits? are unable to detect
> valid codes, then I will certainly rework.

I don't think you can do a 100% solution because for 100%
you would need to know the code segment the CPU is going
to use later, and that's not possible in advance.

A heuristic is reasonable (and leave out applications
that generate 64bit code from 32bit executables or vice versa)
but you need to test the right personality bits for that.

 
> > Also the compat bit is not necessarily set if no system call is
> > executing. You would rather need to check the exec_domain.
> 
> Okay, I shall check and revert on this.

Hmm actually I double checked and this is a separate bit.
So scratch that, TIF_32BIT is ok to test.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.