linux-kernel - Re: [RFC] Have insn decoder functions return success/failure

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: <20201024161315.267499b854b1d08213f7c445@kernel.org>
Date:   Sat, 24 Oct 2020 16:13:15 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Andy Lutomirski <luto@...nel.org>, x86-ml <x86@...nel.org>,
        Joerg Roedel <jroedel@...e.de>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] Have insn decoder functions return success/failure

On Sat, 24 Oct 2020 01:27:41 +0200
Borislav Petkov <bp@...en8.de> wrote:

> On Fri, Oct 23, 2020 at 07:47:04PM +0900, Masami Hiramatsu wrote:
> > Thanks! I look forward to it.
> 
> Ok, here's a first stab, it is a single big diff and totally untested
> but it should show what I mean. I've made some notes while converting,
> as I went along.

Thanks, so will you split this into several patches, since I saw some
cleanups in this patch?

> 
> Have a look at insn_decode() and its call sites: they are almost trivial
> now because caller needs simply to do:
> 
> 	if (insn_decode(insn, buffer, ...))
> 
> and not care about any helper functions.

Yeah, that's good to me because in the most cases, user needs prefix,
length or total decoded info.

BTW, it seems you returns 1 for errors, I rather like -EINVAL or -EILSEQ
for errors so that user can also write

 if (insn_decode() < 0)
   ...

I think "positive" and "zero" pair can easily mislead user to "true" and
"false" trap.


> For some of the call sites it still makes sense to do a piecemeal insn
> decoding and I've left them this way but they can be converted too, if
> one wants.

Yeah, for the kprobes, if you see the insn_init() and insn_get_length()
those can be replaced with one insn_decode(). 

> In any case, just have a look please and lemme know if that looks OKish.
> I'll do the actual splitting and testing afterwards.

Except for the return value, it looks good to me.

Thank you,


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