linux-kernel - Re: [PATCH v7 05/11] arm64: kexec

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] [day] [month] [year] [list]

Date:   Fri, 9 Feb 2018 21:40:56 +0900
From:   AKASHI Takahiro <takahiro.akashi@...aro.org>
To:     James Morse <james.morse@....com>
Cc:     catalin.marinas@....com, will.deacon@....com,
        bauerman@...ux.vnet.ibm.com, dhowells@...hat.com,
        vgoyal@...hat.com, herbert@...dor.apana.org.au,
        davem@...emloft.net, akpm@...ux-foundation.org, mpe@...erman.id.au,
        dyoung@...hat.com, bhe@...hat.com, arnd@...db.de,
        ard.biesheuvel@...aro.org, julien.thierry@....com,
        kexec@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 05/11] arm64: kexec_file: create purgatory

On Wed, Feb 07, 2018 at 06:37:16PM +0000, James Morse wrote:
> Hi Akashi,
> 
> On 04/12/17 02:57, AKASHI Takahiro wrote:
> > This is a basic purgatory, or a kind of glue code between the two kernels,
> > for arm64.
> > 
> > Since purgatory is assumed to be relocatable (not executable) object by
> > kexec generic code, arch_kexec_apply_relocations_add() is required in
> > general. Arm64's purgatory, however, is a simple asm and all the references
> > can be resolved as local, no re-linking is needed here.
> > 
> > Please note that even if we don't support digest check at purgatory we
> 
> (You knew what I was going to ask!)

Yes, definitely.

> 
> > need purgatory_sha_regions and purgatory_sha256_digest as they are
> > referenced by generic kexec code.
> 
> As somewhere to store the values? If we aren't doing the validation could we add
> something about why not to the commit message? I think its because we only worry
> about memory corruption for kdump, and for kdump we unmap the crash-kernel
> region during normal-operation to prevent it getting corrupted.
> 
> As we aren't doing the hash validation, could we hide its core-code behind some
> ARCH_HAS_KEXEC_PURGATORY_HASH, instead of defining dummy symbols and doing
> unnecessary work to fill them in?

Yes, this is one idea.
But as you mentioned below, adding a purgatory for arm64's kexec_file
does make little sense as I've already removed digest check code after
MarkR's comment.

> 
> > diff --git a/arch/arm64/purgatory/entry.S b/arch/arm64/purgatory/entry.S
> > new file mode 100644
> > index 000000000000..fe6e968076db
> > --- /dev/null
> > +++ b/arch/arm64/purgatory/entry.S
> > @@ -0,0 +1,55 @@
> > +/*
> > + * kexec core purgatory
> > + */
> > +#include <linux/linkage.h>
> > +#include <uapi/linux/kexec.h>
> > +
> > +#define SHA256_DIGEST_SIZE	32 /* defined in crypto/sha.h */
> > +
> > +.text
> > +
> > +ENTRY(purgatory_start)
> > +	/* Start new image. */
> > +	ldr	x17, __kernel_entry
> > +	ldr	x0, __dtb_addr
> > +	mov	x1, xzr
> > +	mov	x2, xzr
> > +	mov	x3, xzr
> > +	br	x17
> > +END(purgatory_start)
> 
> Is this what arm64_relocate_new_kernel() drops into? I thought that had the
> kernel boot register values already so we wouldn't need another trampoline for
> kexec_file_load()...

Indeed

> .. but now that I look, it doesn't have the DTB, presumably because for regular
> kexec we don't know where user-space put it.
> 
> Could we add some x0_for_kexec that is 0 by default (if that's the ABI), or the

First, I didn't get what you meaned here.
After managing to modify my code, I found that we could re-use
cpu_soft_restart(), especially, the fifth argument, which is currently
contant 0, but we will be able to pass dtb address here.
In turn, we can also use this argument to determine, in relocate_new_kernel(),
whether we should call puragatory (kexec_load) or directly jump into the kernel
(kexec_file_load).


> DTB address for kexec_file_load()? This would avoid this extra trampoline, and
> patching in the values from load_other_segments().
> 
> I'd love to avoid an in-kernel purgatory! (its code with funny
> compile/link/relocation requirements that is impossible to debug)

Lovely!

I really appreicated your valuable comments.
and more on other patches comming?

-Takahiro AKASHI

> 
> Thanks,
> 
> James