linux-kernel - Re: [PATCH 5/5] arm64: Add uprobe support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160921170403.GE12180@e104818-lin.cambridge.arm.com>
Date:   Wed, 21 Sep 2016 18:04:04 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Pratyush Anand <panand@...hat.com>
Cc:     Shi Yang <yang.shi@...aro.org>,
        Mark Rutland <mark.rutland@....com>, srikar@...ux.vnet.ibm.com,
        will.deacon@....com, linux-kernel@...r.kernel.org,
        Vladimir Murzin <vladimir.murzin@....com>,
        linux@....linux.org.uk, vijaya.kumar@...iumnetworks.com,
        dave.long@...aro.org, Jungseok Lee <jungseoklee85@...il.com>,
        steve.capper@...aro.org,
        "Suzuki K. Poulose" <suzuki.poulose@....com>,
        Andre Przywara <andre.przywara@....com>,
        Shaokun Zhang <zhangshaokun@...ilicon.com>,
        Ashok Kumar <ashoks@...adcom.com>,
        Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>,
        wcohen@...hat.com, linux-arm-kernel@...ts.infradead.org,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>, oleg@...hat.com,
        James Morse <james.morse@....com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH 5/5] arm64: Add uprobe support

On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > On Tue, Aug 02, 2016 at 11:00:09AM +0530, Pratyush Anand wrote:
> > > --- a/arch/arm64/include/asm/thread_info.h
> > > +++ b/arch/arm64/include/asm/thread_info.h
> > > @@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
> > >  #define TIF_NEED_RESCHED	1
> > >  #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
> > >  #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
> > > +#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */
> > 
> > Nitpick: you can just use 4 until we cover this gap.
> 
> Hummm..as stated in commit log, Shi Yang suggested to define TIF_UPROBE as 5 in
> stead of 4, since 4 has already been used in -rt kernel. May be, I can put a
> comment in code as well.
> Or, keep it 4 and -rt kernel will change their definitions. I am OK with both.
> let me know.

I forgot about the -rt kernel. I guess the -rt guys need to rebase the
patches anyway on top of mainline, so it's just a matter of sorting out
a minor conflict (as I already said, these bits are internal to the
kernel, so no ABI affected). For now, just use 4 so that we avoid
additional asm changes.

> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -688,7 +688,8 @@ ret_fast_syscall:
> > >  	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> > >  	and	x2, x1, #_TIF_SYSCALL_WORK
> > >  	cbnz	x2, ret_fast_syscall_trace
> > > -	and	x2, x1, #_TIF_WORK_MASK
> > > +	mov     x2, #_TIF_WORK_MASK
> > > +	and     x2, x1, x2
> > 
> > Is this needed because _TIF_WORK_MASK cannot work as an immediate value
> > to 'and'? We could reorder the TIF bits, they are not exposed to user to
> > have ABI implications.
> 
> _TIF_WORK_MASK is defined as follows:
> 
> #define _TIF_WORK_MASK          (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
>                                   _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
>                                   _TIF_UPROBE)
> Re-ordering will not help, because 0-3 have already been used by previous
> definitions. Only way to use immediate value could be if, TIF_UPROBE is defined
> as 4. 

Yes, see above.

> > > +unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
> > > +{
> > > +	return instruction_pointer(regs);
> > > +}
> > > +
> > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > +		unsigned long addr)
> > > +{
> > > +	probe_opcode_t insn;
> > > +
> > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > 
> > Is there a way to check (not necessarily in this file) that we don't
> > probe 32-bit tasks?
> 
> - Well, I do not have complete idea about it that, how it can be done. I think
>   we can not check that just by looking a single bit in an instruction.
>   My understanding is that, we can only know about it when we are executing the
>   instruction, by reading pstate, but that would not be useful for uprobe
>   instruction analysis.
>   
>   I hope, instruction encoding for aarch32 and aarch64 are different, and by
>   analyzing for all types of aarch32 instructions, we will be able to decide
>   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
>   either BRK or BKPT instruction for breakpoint generation.

We may have some unrelated instruction encoding overlapping but I
haven't checked. I was more thinking about whether we know which task is
being probed and check is_compat_task() or maybe using
compat_user_mode(regs).

-- 
Catalin