linux-kernel - Re: [RFC PATCH v0 03/19] x86/insn: Add an insn

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: <20201126103709.23d581fe2320f14a272a7dc5@kernel.org>
Date:   Thu, 26 Nov 2020 10:37:09 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Andy Lutomirski <luto@...capital.net>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v0 03/19] x86/insn: Add an insn_decode() API

On Wed, 25 Nov 2020 20:25:53 +0100
Borislav Petkov <bp@...en8.de> wrote:

> On Thu, Nov 26, 2020 at 01:53:33AM +0900, Masami Hiramatsu wrote:
> > (only from the viewpoint of VEX coding, a bit stricter, but not perfect.)
> 
> Yeah, I wanted to document the fact that it has changed behavior in the
> commit message - we'll make it perfect later. :-)

It is just a note :) and no need to update the document.

BTW, the instruction validation depends on who needs it, because to
check the all invalid ops, we need more information in the x86-opcode-map.txt
and it will bloat up the table size and consumes more time to analysis.
(Moreover, it depends on the processor generation -- older processor will
not support VEX prefix, those are invalid)
If we can expect that the target instruction is kernel text which compiler
outputs, we may not need such instruction validation.
> 
> > > @@ -98,8 +101,12 @@ static void insn_get_emulate_prefix(struct insn *insn)
> > >   * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
> > >   * to point to the (first) opcode.  No effect if @insn->prefixes.got
> > >   * is already set.
> > > + *
> > > + * * Returns:
> > > + * 0:  on success
> > > + * !0: on error
> > >   */
> > 
> > So this is different from...
> > 
> > [..]
> > > +
> > > +/**
> > > + * insn_decode() - Decode an x86 instruction
> > > + * @insn:	&struct insn to be initialized
> > > + * @kaddr:	address (in kernel memory) of instruction (or copy thereof)
> > > + * @buf_len:	length of the insn buffer at @kaddr
> > > + * @m:		insn mode, see enum insn_mode
> > > + *
> > > + * Returns:
> > > + * 0: if decoding succeeded
> > > + * < 0: otherwise.
> > 
> > this return value.
> > 
> > Even for the insn_get_*(), I would like to see them returning -EINVAL
> > as same as insn_decode(). Same API group has different return value is
> > confusing.
> 
> Right, my goal in the end here is to make *all* users of the decoder
> call insn_decode() and nothing else. And there you can have different
> return values so checking negative/positive is the proper way to go.
> 
> Those other helpers, though, should then become internal and for those I
> think it is easier to use them when they return a boolean yes/no value,
> meaning, they do one thing and one thing only:
> 
> For example, it is more readable to do:
> 
> 	if (insn_...)
> 
> vs
> 
> 	int ret;
> 
> 	...
> 
> 	ret = insn_,...()
> 	if (ret)
> 		...
> 
> which is 4 more lines of error handling and return variable, leading to
> more code.

OK, then could you use -1 instead of 1? It may allow us to expand it
to return error code in the future.

> 
> But if you want to be able to use those other helpers outside of the
> decoder - for whatever reason - then sure, the function signatures
> should be the same.

I think insn_get_prefixes() can be used independently, because x86
perfix bytes is very complex. (I found kprobes is using its insn_attribute
directly, but that is not good. I'll fix it)

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>