linux-kernel - Re: [PATCH 05/70] x86/insn: Make inat-tables.c suitable for pre-decompression code

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: <20200417215000.47141001f80005f41153d22e@kernel.org>
Date:   Fri, 17 Apr 2020 21:50:00 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Joerg Roedel <joro@...tes.org>
Cc:     Borislav Petkov <bp@...en8.de>, Joerg Roedel <jroedel@...e.de>,
        x86@...nel.org, hpa@...or.com, Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Jiri Slaby <jslaby@...e.cz>,
        Dan Williams <dan.j.williams@...el.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Juergen Gross <jgross@...e.com>,
        Kees Cook <keescook@...omium.org>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH 05/70] x86/insn: Make inat-tables.c suitable for
 pre-decompression code

On Thu, 16 Apr 2020 17:24:06 +0200
Joerg Roedel <joro@...tes.org> wrote:

> Hi Masami,
> 
> On Fri, Mar 27, 2020 at 12:02:32PM +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Mar 2020 16:39:45 +0100
> > Borislav Petkov <bp@...en8.de> wrote:
> > 
> > > + Masami.
> > > 
> > > On Thu, Mar 19, 2020 at 10:13:02AM +0100, Joerg Roedel wrote:
> > > > From: Joerg Roedel <jroedel@...e.de>
> > > > 
> > > > The inat-tables.c file has some arrays in it that contain pointers to
> > > > other arrays. These pointers need to be relocated when the kernel
> > > > image is moved to a different location.
> > > > 
> > > > The pre-decompression boot-code has no support for applying ELF
> > > > relocations, so initialize these arrays at runtime in the
> > > > pre-decompression code to make sure all pointers are correctly
> > > > initialized.
> > 
> > I need to check the whole series, but as far as I can understand from
> > this patch, this seems not allowing to store the address value in
> > static pointers. It may break more things, for example _kprobe_blacklist
> > records the NOKPROBE_SYMBOL() symbol addresses at the build time.
> 
> The runtime-initialization function is only used in the
> pre-decompression boot code (arch/x86/boot/compressed/) which is not
> part of the running kernel image. At that stage of booting there is no
> support for kprobe or tracing or any other neat features that might
> break things here.

Ah, I got it. So you intended to port the instruction decoder to
pre-decompression boot code, correct?

> > > > +	print "#ifndef __BOOT_COMPRESSED\n"
> > > > +
> > > >  	# print escape opcode map's array
> > > >  	print "/* Escape opcode map array */"
> > > >  	print "const insn_attr_t * const inat_escape_tables[INAT_ESC_MAX + 1]" \
> > > > @@ -388,6 +391,51 @@ END {
> > > >  		for (j = 0; j < max_lprefix; j++)
> > > >  			if (atable[i,j])
> > > >  				print "	["i"]["j"] = "atable[i,j]","
> > > > -	print "};"
> > > > +	print "};\n"
> > > > +
> > > > +	print "#else /* !__BOOT_COMPRESSED */\n"
> > 
> > I think the definitions of inat_*_tables can be shared in both case.
> > If __BOOT_COMPRESSED is set, we can define inat_init_tables() as a
> > initialize function, and if not, it will be just a dummy "do {} while (0)".
> 
> The inat_*_tables are all declared const, so this way it is not possible
> to change them at runtime.

Indeed.

> For the running kernel image this is fine, as
> there are ELF relocations which fix things up, but at the
> pre-decompression boot stage there are no ELF relocations which can fix
> the tables, so the pointers in there need to be initialized at runtime.

OK.

> 
> > BTW, where is the __BOOT_COMPRESSED defined?
> 
> It is defined in arch/x86/boot/compressed/sev-es.c by patch
> 
> 	x86/boot/compressed/64: Setup GHCB Based VC Exception handler
> 
> which also includes parts of the instruction decoder into the
> pre-decompression boot code and adds the only call-site for
> inat_init_tables().

Thanks, I understand it.

> 
> > > > +	print "static void inat_init_tables(void)"
> > 
> > This functions should be "inline".
> > And I can not see the call-site of inat_init_tables() in this patch.
> 
> The call-site is added with the patch that includes the
> instruction decoder into the pre-decompression code. If possible I'd
> like to keep those things separate, as both patches are already pretty
> big by themselfes (and they do different things, in different parts of
> the code).

OK, if you will send v2, please CC both to me.

> 
> > If possible, please include call-site with definition (especially
> > new init function) so that I can check the init call timing too.
> 
> The function is called at the first #VC exception after a GHCB has been
> set up. Call-path is: boot_vc_handler -> sev_es_setup_ghcb ->
> inat_init_tables.

sound good to me. 

Thank you,

> 
> See
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/joro/linux.git/tree/arch/x86/boot/compressed/sev-es.c?h=sev-es-client-v5.6-rc6
> 
> for the full code there.
> 
> Thanks,
> 
> 	Joerg


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