[PATCH] uprobes/x86: Refuse to attach uprobe to "word-sized" branch insns

[PATCH] uprobes/x86: Refuse to attach uprobe to "word-sized" branch insns

[Denys Vlasenko]

All branch insns on x86 can be prefixed with the operand-size
override prefix, 0x66. It was only ever useful for performing
jumps to 32-bit offsets in 16-bit code segments.

In 32-bit code, such instructions are useless since
they cause IP truncation to 16 bits, and in case of call insns,
they save only 16 bits of return address and misalign
the stack pointer as a "bonus".

In 64-bit code, such instructions are treated differently by Intel
and AMD CPUs: Intel ignores the prefix altogether,
AMD treats them the same as in 32-bit mode.

Before this patch, the emulation code would execute
the instructions as if they have no 0x66 prefix.

With this patch, we refuse to attach uprobes to such insns.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/uprobes.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index ace2291..29b152d 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -582,6 +582,7 @@ static struct uprobe_xol_ops branch_xol_ops = {
 /* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
 static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
 {
+	int i;
 	u8 opc1 = OPCODE1(insn);
 
 	/* has the side-effect of processing the entire instruction */
@@ -612,6 +613,17 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
 			return -ENOSYS;
 	}
 
+	/*
+	 * 16-bit overrides such as CALLW (66 e8 nn nn) are not supported.
+	 * Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix.
+	 * No one uses these insns.
+	 * Reject any branch insns with such prefix.
+	 */
+	for (i = 0; i < insn->prefixes.nbytes; i++) {
+		if (insn->prefixes.bytes[i] == 0x66)
+			return -ENOTSUPP;
+	}
+
 	auprobe->branch.opc1 = opc1;
 	auprobe->branch.ilen = insn->length;
 	auprobe->branch.offs = insn->immediate.value;
-- 
1.8.1.4

Re: [PATCH] uprobes/x86: Refuse to attach uprobe to "word-sized" branch insns

[Oleg Nesterov]

Thanks!

Masami, Jim, any acks?

On 04/24, Denys Vlasenko wrote:
>
> All branch insns on x86 can be prefixed with the operand-size
> override prefix, 0x66. It was only ever useful for performing
> jumps to 32-bit offsets in 16-bit code segments.
>
> In 32-bit code, such instructions are useless since
> they cause IP truncation to 16 bits, and in case of call insns,
> they save only 16 bits of return address and misalign
> the stack pointer as a "bonus".
>
> In 64-bit code, such instructions are treated differently by Intel
> and AMD CPUs: Intel ignores the prefix altogether,
> AMD treats them the same as in 32-bit mode.
>
> Before this patch, the emulation code would execute
> the instructions as if they have no 0x66 prefix.
>
> With this patch, we refuse to attach uprobes to such insns.
>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/uprobes.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index ace2291..29b152d 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -582,6 +582,7 @@ static struct uprobe_xol_ops branch_xol_ops = {
>  /* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
>  static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>  {
> +	int i;
>  	u8 opc1 = OPCODE1(insn);
>
>  	/* has the side-effect of processing the entire instruction */
> @@ -612,6 +613,17 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>  			return -ENOSYS;
>  	}
>
> +	/*
> +	 * 16-bit overrides such as CALLW (66 e8 nn nn) are not supported.
> +	 * Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix.
> +	 * No one uses these insns.
> +	 * Reject any branch insns with such prefix.
> +	 */
> +	for (i = 0; i < insn->prefixes.nbytes; i++) {
> +		if (insn->prefixes.bytes[i] == 0x66)
> +			return -ENOTSUPP;
> +	}
> +
>  	auprobe->branch.opc1 = opc1;
>  	auprobe->branch.ilen = insn->length;
>  	auprobe->branch.offs = insn->immediate.value;
> --
> 1.8.1.4
>

Re: [PATCH] uprobes/x86: Refuse to attach uprobe to "word-sized" branch insns

[Jim Keniston]

On Thu, 2014-04-24 at 19:33 +0200, Oleg Nesterov wrote:
> Thanks!
> 
> Masami, Jim, any acks?

Acked-by: Jim Keniston <jkenisto@us.ibm.com>

> 
> On 04/24, Denys Vlasenko wrote:
> >
> > All branch insns on x86 can be prefixed with the operand-size
> > override prefix, 0x66. It was only ever useful for performing
> > jumps to 32-bit offsets in 16-bit code segments.
> >
> > In 32-bit code, such instructions are useless since
> > they cause IP truncation to 16 bits, and in case of call insns,
> > they save only 16 bits of return address and misalign
> > the stack pointer as a "bonus".
> >
> > In 64-bit code, such instructions are treated differently by Intel
> > and AMD CPUs: Intel ignores the prefix altogether,
> > AMD treats them the same as in 32-bit mode.
> >
> > Before this patch, the emulation code would execute
> > the instructions as if they have no 0x66 prefix.
> >
> > With this patch, we refuse to attach uprobes to such insns.
> >
> > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  arch/x86/kernel/uprobes.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index ace2291..29b152d 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -582,6 +582,7 @@ static struct uprobe_xol_ops branch_xol_ops = {
> >  /* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
> >  static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> >  {
> > +	int i;
> >  	u8 opc1 = OPCODE1(insn);
> >
> >  	/* has the side-effect of processing the entire instruction */
> > @@ -612,6 +613,17 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> >  			return -ENOSYS;
> >  	}
> >
> > +	/*
> > +	 * 16-bit overrides such as CALLW (66 e8 nn nn) are not supported.
> > +	 * Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix.
> > +	 * No one uses these insns.
> > +	 * Reject any branch insns with such prefix.
> > +	 */
> > +	for (i = 0; i < insn->prefixes.nbytes; i++) {
> > +		if (insn->prefixes.bytes[i] == 0x66)
> > +			return -ENOTSUPP;
> > +	}
> > +
> >  	auprobe->branch.opc1 = opc1;
> >  	auprobe->branch.ilen = insn->length;
> >  	auprobe->branch.offs = insn->immediate.value;
> > --
> > 1.8.1.4
> >
> 

Re: Re: [PATCH] uprobes/x86: Refuse to attach uprobe to "word-sized" branch insns

[Masami Hiramatsu]

(2014/04/25 2:33), Oleg Nesterov wrote:
> Thanks!
> 
> Masami, Jim, any acks?
> 
> On 04/24, Denys Vlasenko wrote:
>>
>> All branch insns on x86 can be prefixed with the operand-size
>> override prefix, 0x66. It was only ever useful for performing
>> jumps to 32-bit offsets in 16-bit code segments.
>>
>> In 32-bit code, such instructions are useless since
>> they cause IP truncation to 16 bits, and in case of call insns,
>> they save only 16 bits of return address and misalign
>> the stack pointer as a "bonus".
>>
>> In 64-bit code, such instructions are treated differently by Intel
>> and AMD CPUs: Intel ignores the prefix altogether,
>> AMD treats them the same as in 32-bit mode.
>>
>> Before this patch, the emulation code would execute
>> the instructions as if they have no 0x66 prefix.
>>
>> With this patch, we refuse to attach uprobes to such insns.
>>
>> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
>> Cc: Oleg Nesterov <oleg@redhat.com>

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thank you :)

>> ---
>>  arch/x86/kernel/uprobes.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
>> index ace2291..29b152d 100644
>> --- a/arch/x86/kernel/uprobes.c
>> +++ b/arch/x86/kernel/uprobes.c
>> @@ -582,6 +582,7 @@ static struct uprobe_xol_ops branch_xol_ops = {
>>  /* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
>>  static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>>  {
>> +	int i;
>>  	u8 opc1 = OPCODE1(insn);
>>
>>  	/* has the side-effect of processing the entire instruction */
>> @@ -612,6 +613,17 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>>  			return -ENOSYS;
>>  	}
>>
>> +	/*
>> +	 * 16-bit overrides such as CALLW (66 e8 nn nn) are not supported.
>> +	 * Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix.
>> +	 * No one uses these insns.
>> +	 * Reject any branch insns with such prefix.
>> +	 */
>> +	for (i = 0; i < insn->prefixes.nbytes; i++) {
>> +		if (insn->prefixes.bytes[i] == 0x66)
>> +			return -ENOTSUPP;
>> +	}
>> +
>>  	auprobe->branch.opc1 = opc1;
>>  	auprobe->branch.ilen = insn->length;
>>  	auprobe->branch.offs = insn->immediate.value;
>> --
>> 1.8.1.4
>>
> 
> --
> 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/
> 


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