Re: [v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

[Dave Hansen]

On 12/27/2016 02:33 PM, Ricardo Neri wrote:
>>> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
>>> index 6a75a75..71681d0 100644
>>> --- a/arch/x86/mm/mpx.c
>>> +++ b/arch/x86/mm/mpx.c
>>> @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct 
>>> pt_regs *regs,
>>>
>>> case REG_TYPE_BASE:
>>> regno = X86_SIB_BASE(insn->sib.value);
>>> +   if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) {
>>> +   WARN_ONCE(1, "An explicit displacement is required 
>>> when %sBP used as SIB base.",
>>> + (IS_ENABLED(CONFIG_X86_64) && 
>>> insn->x86_64) ?
>>> + "R13 or R" : "E");
>>> +   return -EINVAL;
>>> +   }
>>> +
>> Now that I've read the cover letter, I see what's going on.  This
>> should not warn -- user code can easily trigger this deliberately.
> OK, I'll remove it. Are you concerned about the warning printing the
> calltrace, even only once?

Yes.  We don't let userspace spam the kernel, even once.  If we have a
couple thousand "only once" places, then userspace can overwhelm the
kernel log.

Also, this needs a much better description of what's going on in the
code.  Could you add a comment explaining what's going on, and why
regno==5, etc...?
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

[Andy Lutomirski]

On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri
 wrote:
> Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software
> Developer's Manual volume 2A states that when memory addressing with no
> explicit displacement (i.e, mod part of ModR/M is 0), a SIB byte is used
> and the base of the SIB byte points to (R/EBP) (i.e., base = 5), an
> explicit displacement of 0 must be used.
>
> Make the address decoder to return -EINVAL in such a case.
>
> Cc: Dave Hansen 
> Cc: Adam Buchbinder 
> Cc: Colin Ian King 
> Cc: Lorenzo Stoakes 
> Cc: Qiaowei Ren 
> Cc: Ravi V. Shankar 
> Cc: x...@kernel.org
> Signed-off-by: Ricardo Neri 
> ---
>  arch/x86/mm/mpx.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index 6a75a75..71681d0 100644
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct 
> pt_regs *regs,
>
> case REG_TYPE_BASE:
> regno = X86_SIB_BASE(insn->sib.value);
> +   if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) {
> +   WARN_ONCE(1, "An explicit displacement is required 
> when %sBP used as SIB base.",
> + (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) 
> ?
> + "R13 or R" : "E");
> +   return -EINVAL;
> +   }
> +

Now that I've read the cover letter, I see what's going on.  This
should not warn -- user code can easily trigger this deliberately.
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

[Ricardo Neri]

Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software
Developer's Manual volume 2A states that when memory addressing with no
explicit displacement (i.e, mod part of ModR/M is 0), a SIB byte is used
and the base of the SIB byte points to (R/EBP) (i.e., base = 5), an
explicit displacement of 0 must be used.

Make the address decoder to return -EINVAL in such a case.

Cc: Dave Hansen 
Cc: Adam Buchbinder 
Cc: Colin Ian King 
Cc: Lorenzo Stoakes 
Cc: Qiaowei Ren 
Cc: Ravi V. Shankar 
Cc: x...@kernel.org
Signed-off-by: Ricardo Neri 
---
 arch/x86/mm/mpx.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index 6a75a75..71681d0 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct 
pt_regs *regs,
 
case REG_TYPE_BASE:
regno = X86_SIB_BASE(insn->sib.value);
+   if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) {
+   WARN_ONCE(1, "An explicit displacement is required when 
%sBP used as SIB base.",
+ (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) ?
+ "R13 or R" : "E");
+   return -EINVAL;
+   }
+
if (X86_REX_B(insn->rex_prefix.value))
regno += 8;
break;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html