On Wed, 2017-04-12 at 18:28 +0200, Borislav Petkov wrote:
On Tue, Mar 07, 2017 at 04:32:38PM -0800, Ricardo Neri wrote:
The function insn_get_reg_offset takes as argument an enumeration that
Please end function names with parentheses.
Will do!
And do you mean get_reg_offset(), per chance?
Yes, I meant that. This was a copy/paste error.
indicates the type of offset that is returned: the R/M part of the ModRM byte, the index of the SIB byte or the base of the SIB byte.
Err, you mean, it returns the offset to the register the argument specifies.
Yes. I will reword.
Callers of this function would need the definition of such enumeration. This is not needed. Instead, helper functions can be defined for this purpose can be added.
"Instead, add helpers... "
I will reword.
These functions are useful in cases when, for instance, the caller needs to decide whether the operand is a register or a memory location by looking at the mod part of the ModRM byte.
Cc: Dave Hansen dave.hansen@linux.intel.com Cc: Adam Buchbinder adam.buchbinder@gmail.com Cc: Colin Ian King colin.king@canonical.com Cc: Lorenzo Stoakes lstoakes@gmail.com Cc: Qiaowei Ren qiaowei.ren@intel.com Cc: Arnaldo Carvalho de Melo acme@redhat.com Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Adrian Hunter adrian.hunter@intel.com Cc: Kees Cook keescook@chromium.org Cc: Thomas Garnier thgarnie@google.com Cc: Peter Zijlstra peterz@infradead.org Cc: Borislav Petkov bp@suse.de Cc: Dmitry Vyukov dvyukov@google.com Cc: Ravi V. Shankar ravi.v.shankar@intel.com Cc: x86@kernel.org Signed-off-by: Ricardo Neri ricardo.neri-calderon@linux.intel.com
arch/x86/include/asm/insn-eval.h | 3 +++ arch/x86/lib/insn-eval.c | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)
diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h index 5cab1b1..754211b 100644 --- a/arch/x86/include/asm/insn-eval.h +++ b/arch/x86/include/asm/insn-eval.h @@ -12,5 +12,8 @@ #include <asm/ptrace.h>
void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); +int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs); +int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); +int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
Forgotten to edit the copy-paste?
Which means, nothing really needs insn_get_reg_offset_sib_index() and you can get rid of it?
Yes, I can get rid of it.
#endif /* _ASM_X86_INSN_EVAL_H */ diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c index 23cf010..78df1c9 100644 --- a/arch/x86/lib/insn-eval.c +++ b/arch/x86/lib/insn-eval.c @@ -98,6 +98,57 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, return regoff[regno]; }
+/**
- insn_get_reg_offset_modrm_rm - Obtain register in r/m part of ModRM byte
- @insn: Instruction structure containing the ModRM byte
- @regs: Set of registers indicated by the ModRM byte
That's simply struct pt_regs - not a set of registers indicated by ModRM?!?
I will reword it to say "A struct pt_regs containing register values indicated by the ModRM byte".
- Obtain the register indicated by the r/m part of the ModRM byte. The
- register is obtained as an offset from the base of pt_regs. In specific
- cases, the returned value can be -EDOM to indicate that the particular value
- of ModRM does not refer to a register.
Put that sentence under the "Return: " paragraph below so that it is immediately obvious what the retvals are.
Will do.
- Return: Register indicated by r/m, as an offset within struct pt_regs
- */
+int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs)
That name is too long: insn_get_modrm_rm_off() should be enough.
+{
- return get_reg_offset(insn, regs, REG_TYPE_RM);
+}
+/**
- insn_get_reg_offset_sib_base - Obtain register in base part of SiB byte
- @insn: Instruction structure containing the SiB byte
- @regs: Set of registers indicated by the SiB byte
- Obtain the register indicated by the base part of the SiB byte. The
- register is obtained as an offset from the base of pt_regs. In specific
- cases, the returned value can be -EDOM to indicate that the particular value
- of SiB does not refer to a register.
- Return: Register indicated by SiB's base, as an offset within struct pt_regs
Will make the spelling consistent.
Let's stick to a single spelling: SIB, all caps.
- */
+int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs)
insn_get_sib_base_off()
Ditto for the rest of the comments on insn_get_reg_offset_modrm_rm() above.
+{
- return get_reg_offset(insn, regs, REG_TYPE_BASE);
+}
+/**
- insn_get_reg_offset_sib_index - Obtain register in index part of SiB byte
- @insn: Instruction structure containing the SiB byte
- @regs: Set of registers indicated by the SiB byte
- Obtain the register indicated by the index part of the SiB byte. The
- register is obtained as an offset from the index of pt_regs. In specific
- cases, the returned value can be -EDOM to indicate that the particular value
- of SiB does not refer to a register.
- Return: Register indicated by SiB's base, as an offset within struct pt_regs
- */
+int insn_get_reg_offset_sib_index(struct insn *insn, struct pt_regs *regs)
insn_get_sib_idx_off()
And again, if this function is unused, don't add it.
Masami Hiramatsu had originally requested to add the two functions. I suppose the unneeded functions could be added if/when needed.
Thanks and BR, Ricardo