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: <20201125192553.GD9996@zn.tnic>
Date:   Wed, 25 Nov 2020 20:25:53 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Masami Hiramatsu <mhiramat@...nel.org>
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 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. :-)

> > @@ -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.

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.

Thoughts?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette