linux-kernel - Re: [PATCH v2 6/9] uprobes/x86: Introduce uprobe_xol_ops and arch

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]

Date:	Wed, 09 Apr 2014 21:58:54 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <aarapov@...hat.com>,
	David Long <dave.long@...aro.org>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Jim Keniston <jkenisto@...ibm.com>,
	Jonathan Lebon <jlebon@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/9] uprobes/x86: Introduce uprobe_xol_ops and arch_uprobe->ops

(2014/04/09 1:10), Oleg Nesterov wrote:
> On 04/08, Masami Hiramatsu wrote:
>>
>> (2014/04/05 3:51), Oleg Nesterov wrote:
>>>
>>> TODO: An error from adjust_ret_addr() shouldn't be silently ignored,
>>> we should teach arch_uprobe_post_xol() or handle_singlestep() paths
>>> to restart the probed insn in this case. And probably "adjust" can
>>> be simplified and turned into set_ret_addr(). It seems that we do
>>> not really need copy_from_user(), we can always calculate the value
>>> we need to write into *regs->sp.
>>
>> It seems that you fixed this in 8/9, we don't need the TODO list in
>> the description.
> 
> Well, OK, I'll update the changelog and remove the "error ... ignored"
> part. Although to be honest, I do not understand why do you think it
> is bad to document the other problems you found while you were writing
> the patch.

Because you know how to fix that and you just can do that in following
patches :). In that case, you don't need to state it here.


>>> +	if (auprobe->ops->emulate)
>>> +		return auprobe->ops->emulate(auprobe, regs);
>>> +
>>> +	/* TODO: move this code into ->emulate() hook */
>>
>> If you think this can move into the emulate(),
> 
> Yes sure,
> 
>> you should do in this
>> patch.
> 
> No, sorry, I strongly disagree, this should come as a separate change,
> and only after "Emulate jmp's".

Ah, I see, with your RFC series. :)

>> Since the following code runs by default, there should be
>> no problem to do that.
> 
> Hmm. Not sure I understand "by default".

I meant that the auprobe->ops->emulate() is always skipped and the
old code is always run, since the default_emulate_op() is NULL at
this point.

> If you meant that this should
> go into default_emulate_op() (which we do not have), then I strongly
> disagree again.
> 
> It should not, if nothing else we need to record insn->length somewhere,
> this should go into ttt_emulate_op() we add later. And it simply looks
> much more natural to handle jmp's and nop's together.

I see, OK with ttt_emulate_op() series.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>

Thank you :)

> 
> Masami, this time I simply can't understand your objections, please
> clarify.
> 
> Thanks,
> 
> Oleg.
> 
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


--
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/