Date: Mon, 18 Jun 2012 17:36:43 +0530
From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>,
        lkml <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@elte.hu>,
        peterz@infradead.org, Jim Keniston <jkenisto@us.ibm.com>,
        "H. Peter Anvin" <hpa@zytor.com>, torvalds@linux-foundation.org
Subject: Re: [PATCH v2 1/2] uprobes: Pass probed vaddr to
 arch_uprobe_analyze_insn()
Message-ID: <20120618120642.GA4629@linux.vnet.ibm.com>
Reply-To: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
References: <20120608093257.GG13409@in.ibm.com>
 <20120611161215.GA12116@redhat.com>
 <20120612165404.GB30555@linux.vnet.ibm.com>
 <20120612174305.GA16349@redhat.com>
 <20120613191519.GA14246@redhat.com>
 <20120614114514.GA12051@linux.vnet.ibm.com>
 <20120614181934.GA9424@redhat.com>
 <20120615123300.GA5748@linux.vnet.ibm.com>
 <20120616180555.GA13634@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <20120616180555.GA13634@redhat.com>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4268
Lines: 110

* Oleg Nesterov <oleg@redhat.com> [2012-06-16 20:05:55]:

> Srikar,
> 
> To clarify: I am not arguing, I am asking because I know nothing about
> asm/opcodes/etc.

Certainly not, I think you are talking about a valid concern.

> 
> On 06/15, Srikar Dronamraju wrote:
> >
> > > But this can't protect from the malicious user who does
> > > mmap(64-bit-code, PROT_EXEC) from a 32-bit app, and this can confuse
> > > uprobes even if that 32-bit app never tries to actually execute that
> > > 64-bit-code.
> > >
> >
> > So if we read just after we allocate uprobe struct but before
> > probe insertion, then we dont need to check this for each process.
> >
> > So if the library was 64 bit mapped in 32 bit process and has a valid
> > instruction for 64 bit, then we just check for valid 64 bit instructions
> > and allow insertion of the breakpoint even in the 32 bit process.
> 
> And what happens if this insn is not valid from validate_insn_32bits()
> pov and that 32-bit application tries to execute it? Or vice versa,
> see below.
> 
> > Here is a very crude implementation of the same.
> > Also this depends on read_mapping_page taking NULL as an valid argument
> > for file. As a side-effect we can do away with UPROBE_COPY_INSN which
> > was set and read at just one place.
> >
> > 1. Move the copy_insn to just after alloc_uprobe.
> > 2. Assume that copy_insn can work without struct file
> > ...
> > 5. Move the analyze instruction to before the actual probe insertion.
> 
> OK, this is what I think we should do anyway (at least try to do).
> 
> > 3. Read the elfhdr for the file.
> > 4. Pass the elfhdr to the arch specific analyze insn
> 
> This assumes that everything is elf. Why? An application is free to
> create a file in any format and do mmap(PROT_EXEC).
> 
> But OK, probably we can restrict uprobe_register() to work only with
> elf files which do not mix 32/64 bits.
> 
 
We should probably be able to fix file type part, As I said it was just
a thought.

> 
> My concern is, are you sure an evil user can't confuse uprobes and
> do something bad?
> 
> Just to explain what I mean. For example, we certainly do not want
> to allow to probe the "syscall" insn, at least with the current
> implementation. So I assume that validate_insn_64bits("syscall")
> must fail.
> 
> Are you sure that validate_insn_32bits("syscall") will fail too?
> 
> Of course, I am not asking about "syscall" in particular. In general,
> suppose that, say, validate_insn_64bits() returns true. Are you sure
> this insn can't do something different and harmful if it is executed
> by __USER32_CS task?
> 

validate_insn_64bits can return fail for two cases.
1. Few opcodes that uprobes refuses to place probes.
2. opcodes that are invalid from a 64 perspective.

validate_insn_32bits() can return fail for similar reasons.

The first set is a common set between validate_insn_64bits /
validate_insn_32bits. This includes the syscall, lock prefix, etc.

Coming to the second set, there can be an instruction that is valid for
64 bit and not valid for 32 bit. 

If the instruction is valid for 32 bit set but invalid instruction for
64 bit, and is part of a 32 bit executable file but was mapped by a 64
bit process. We would allow it to be probed since we only check for 32
bit set. [Assuming it runs till a breakpoint hit;] I assume singlestep
should generate a SIGILL signal since its not a valid 64 bit
instruction.  However this behaviour is on par with the behaviour if the
probe was not hit too. i.e Once this invalid instruction was executed,
It would have generated SIGILL.  The same should hold true for a 32 bit
invalid instruction in a 64 bit executable mapped into 32 bit process.

Please do let me know if my understanding is incorrect or if there are
loopholes 

Again, this is all dependent on the ability to detect the executable
type from the inode. If there is any case we cant detect the executable
type, I think we should just skip this discussion and go with your
suggestion.

-- 
Thanks and Regards
Srikar

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/