Messages in this thread | | | Date | Mon, 6 Sep 2010 09:53:19 +0200 | From | Andi Kleen <> | Subject | Re: [PATCHv11 2.6.36-rc2-tip 4/15] 4: uprobes: x86 specific functions for user space breakpointing. |
| |
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.
| |