| 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
| ||
|
Date: Fri, 3 Sep 2010 23:18:32 +0530
From: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To: Andi Kleen <andi@...stfloor.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
Steven Rostedt <rostedt@...dmis.org>,
Randy Dunlap <rdunlap@...otime.net>,
Arnaldo Carvalho de Melo <acme@...radead.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Christoph Hellwig <hch@...radead.org>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Oleg Nesterov <oleg@...hat.com>,
Mark Wielaard <mjw@...hat.com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
LKML <linux-kernel@...r.kernel.org>,
Naren A Devaiah <naren.devaiah@...ibm.com>,
Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
Frederic Weisbecker <fweisbec@...il.com>,
"Frank Ch. Eigler" <fche@...hat.com>,
Ananth N Mavinakayanahalli <ananth@...ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCHv11 2.6.36-rc2-tip 4/15] 4: uprobes: x86 specific
functions for user space breakpointing.
>
> 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.
>
> > +typedef u8 user_bkpt_opcode_t;
>
> Maybe it's me, but I would prefer breakpoint instead of bkpt
Even Peter wasnt comfortable with user_bkpt. How about user_bp?
i.e the above field would be user_bp_opcode_t. I felt
user_breakpoint_opcode_t might look long. Also we would have to
rename other structures accordingly like user_bkpt_task_arch_info
would become user_breakpoint_task_arch_info. Do let me know your
choice.
> > +#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?
>
> > +
> > +#define UPROBES_FIX_RIP_AX 0x8000
> > +#define UPROBES_FIX_RIP_CX 0x4000
> > +
> > +/* Adaptations for mhiramat x86 decoder v14. */
> > +#define OPCODE1(insn) ((insn)->opcode.bytes[0])
> > +#define OPCODE2(insn) ((insn)->opcode.bytes[1])
> > +#define OPCODE3(insn) ((insn)->opcode.bytes[2])
> > +#define MODRM_REG(insn) X86_MODRM_REG(insn->modrm.value)
> > +
> > +/*
> > + * @reg: reflects the saved state of the task
> > + * @vaddr: the virtual address to jump to.
> > + * Return 0 on success or a -ve number on error.
> > + */
> > +void set_ip(struct pt_regs *regs, unsigned long vaddr)
> > +{
> > + regs->ip = vaddr;
> > +}
> > +
> > +#ifdef CONFIG_X86_64
> > +static bool is_riprel_insn(struct user_bkpt *user_bkpt)
> > +{
> > + return ((user_bkpt->fixups &
> > + (UPROBES_FIX_RIP_AX | UPROBES_FIX_RIP_CX)) != 0);
> > +}
> > +
>
> Shouldn't all this stuff be in the instruction decoder?
>
> It seems weird to have the knowledge spread over multiple files.
Agree, Shall move it to instruction decoder.
>
> > +
> > +static void report_bad_prefix(void)
> > +{
> > + printk(KERN_ERR "uprobes does not currently support probing "
> > + "instructions with any of the following prefixes: "
> > + "cs:, ds:, es:, ss:, lock:\n");
> > +}
> > +
> > +static void report_bad_1byte_opcode(int mode, user_bkpt_opcode_t op)
> > +{
> > + printk(KERN_ERR "In %d-bit apps, "
> > + "uprobes does not currently support probing "
> > + "instructions whose first byte is 0x%2.2x\n", mode, op);
> > +}
> > +
> > +static void report_bad_2byte_opcode(user_bkpt_opcode_t op)
> > +{
> > + printk(KERN_ERR "uprobes does not currently support probing "
> > + "instructions with the 2-byte opcode 0x0f 0x%2.2x\n", op);
> > +}
>
> These functions that just do a single printk seem weird. I would
> do that in the caller. Also the message could be shortened I guess
> and should just dump the bytes.
>
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?
> > +
> > +/**
> > + * analyze_insn - instruction analysis including validity and fixups.
> > + * @tsk: the probed task.
> > + * @user_bkpt: the probepoint information.
> > + * Return 0 on success or a -ve number on error.
> > + */
> > +int analyze_insn(struct task_struct *tsk, struct user_bkpt *user_bkpt)
> > +{
> > + int ret;
> > + struct insn insn;
> > +
> > + user_bkpt->fixups = 0;
> > +#ifdef CONFIG_X86_64
> > + user_bkpt->arch_info.rip_target_address = 0x0;
> > +#endif
> > +
> > + if (is_32bit_app(tsk))
>
> 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.
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.
>
> 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.
> > + */
> > +static int adjust_ret_addr(struct task_struct *tsk, unsigned long sp,
> > + long correction)
> > +{
> > + int rasize, ncopied;
> > + long ra = 0;
> > +
> > + if (is_32bit_app(tsk))
> > + rasize = 4;
> > + else
> > + rasize = 8;
> > + ncopied = uprobes_read_vm(tsk, (void __user *) sp, &ra, rasize);
> > + if (unlikely(ncopied != rasize))
> > + goto fail;
>
> goto is automatically unlikely and unlikely is deprecated anyways.
Okay, shall remove unlikely from the above.
--
Thanks and Regards
Srikar
--
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/
Powered by blists - more mailing lists