Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 1 Dec 2020 02:21:45 +0900
From:   Masami Hiramatsu <mhiramat@kernel.org>
To:     Borislav Petkov <bp@alien8.de>
Cc:     Andy Lutomirski <luto@amacapital.net>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v0 00/19] x86/insn: Add an insn_decode() API
Message-Id: <20201201022145.48201fe165a28cb0e1f042ae@kernel.org>
In-Reply-To: <20201130134442.GB6019@zn.tnic>
References: <20201124101952.7909-1-bp@alien8.de>
        <20201124174647.GI4009@zn.tnic>
        <CALCETrXXuUmM6LPj36h2KLw5zuKUPnmrACcOq2-8XfXzWXQt7Q@mail.gmail.com>
        <20201129175005.7e07a9f799e888ffd5f4ed67@kernel.org>
        <20201130134442.GB6019@zn.tnic>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 30 Nov 2020 14:44:42 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Sun, Nov 29, 2020 at 05:50:05PM +0900, Masami Hiramatsu wrote:
> > Good point. I think we can return, e.g. -EFAULT if we failed in
> > get_next(). Then, we can read out next page, for example.
> 
> Why -EFAULT?

Because it overruns the buffer. Maybe -E2BIG/ENODATA or any other
error (except for -EINVAL) is OK :)
 
> Running this
> 
>         size = 1;
>         ret = insn_decode(&insn, b, size, INSN_MODE_64)
> 
> i.e., buffer size is 1:
> 
> ./arch/x86/tools/insn_sanity: Success: decoded and checked 10000 random instructions with 0 errors (seed:0x9994a137)
> insn buffer:
> 0x48 0xcf 0x48 0x83 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 
> supplied buf size: 15, ret 0
> supplied buf size: 2, ret 0
> supplied buf size: 3, ret 0
> supplied buf size: 4, ret 0
> insn_decode: entry
> insn_decode: will get_length
> insn_get_immediate: getting immediate
> insn_get_displacement: getting displacement
> insn_get_sib: getting sib
> insn_get_modrm: entry
> insn_get_opcode: entry
> insn_get_prefixes: entry, prefixes->got: 0
> insn_get_prefixes: 1
> insn_get_prefixes: REX
> insn_get_prefixes: VEX
> insn_get_prefixes: validate_next: 0
> insn_get_prefixes: insn->next_byte: 0x7ffec297c3e1, insn->end_kaddr: 0x7ffec297c3e1
> insn_get_prefixes: errored out
> supplied buf size: 1, ret -22
> 
> is caught in validate_next() where ->next_byte == ->end_kaddr.
> 
> I'm thinking we should return EOF here, to denote that we're reached the
> end and then propagate that error up the callchain.

EOF means the end of file and it is not an error. Here, since the
decoder fails to decode the data, so it should return an error code.

> 
> We don't have "define EOF" in the kernel but we can designate one for
> the insn decoder, perhaps
> 
> #define EOF      -1
> 
> as stdio.h does:
> 
> /* The value returned by fgetc and similar functions to indicate the
>    end of the file.  */
> #define EOF (-1)

It is because libc fgetc() returns -1 in any error case...

> 
> Hmm, but then the callers would need to know EOF too so maybe EIO or
> something.
> 
> In any case, it should be a value which callers should be able to use to
> get told that input buffer is truncated...

Yes!

Thank you,

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette


-- 
Masami Hiramatsu <mhiramat@kernel.org>