Re: [PATCH v11 10/15] arm64: kexec_file: allow for loading Image-format kernel

[AKASHI Takahiro]

On Wed, Jul 18, 2018 at 05:47:50PM +0100, James Morse wrote:
> Hi Akashi,
> 
> On 11/07/18 08:41, AKASHI Takahiro wrote:
> > This patch provides kexec_file_ops for "Image"-format kernel. In this
> > implementation, a binary is always loaded with a fixed offset identified
> > in text_offset field of its header.
> > 
> > Regarding signature verification for trusted boot, this patch doesn't
> > contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
> > in this series, but file-attribute-based verification is still a viable
> > option by enabling IMA security subsystem.
> > 
> > You can sign(label) a to-be-kexec'ed kernel image on target file system
> > with:
> >     $ evmctl ima_sign --key /path/to/private_key.pem Image
> > 
> > On live system, you must have IMA enforced with, at least, the following
> > security policy:
> >     "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"
> > 
> > See more details about IMA here:
> >     https://sourceforge.net/p/linux-ima/wiki/Home/
> 
> This looks useful to set a keys/signature/policy for a kernel that wasn't built
> to enforce signatures at compile time, so its a good thing to have from a
> single-image perspective.
> 
> I haven't managed to get IMA working to test this, but its all done by the kexec
> core code, so I don't think we're missing anything.
> 
> 
> > diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
> > new file mode 100644
> > index 000000000000..a47cf9bc699e
> > --- /dev/null
> > +++ b/arch/arm64/kernel/kexec_image.c
> 
> > +static int image_probe(const char *kernel_buf, unsigned long kernel_len)
> > +{
> > +	const struct arm64_image_header *h;
> > +
> > +	h = (const struct arm64_image_header *)(kernel_buf);
> > +
> > +	if (!h || (kernel_len < sizeof(*h)) ||
> 
> > +			!memcmp(&h->magic, ARM64_MAGIC, sizeof(ARM64_MAGIC)))
> 
> Doesn't memcmp() return 0 if the memory regions are the same?
> This would always match the correct magic, rejecting the image.
> 
> That's not whats happening, as kexec-file works, so this never matches anything.
> 
> sizeof(ARM64_MAGIC) includes the null terminator, but this sequence is output in
> head.S using '.ascii' which doesn't include the terminator, (otherwise it
> wouldn't fit in the 4byte magic field). The memcmp() here is also consuming the
> least significant bytes of the next field.
> 
> I think this line should be:
> | 			memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))

Absolutely you're right!

> 
> > +static void *image_load(struct kimage *image,
> > +				char *kernel, unsigned long kernel_len,
> > +				char *initrd, unsigned long initrd_len,
> > +				char *cmdline, unsigned long cmdline_len)
> 
> > +	kbuf.buffer = kernel;
> > +	kbuf.bufsz = kernel_len;
> > +	kbuf.memsz = le64_to_cpu(h->image_size);
> > +	text_offset = le64_to_cpu(h->text_offset);
> > +	kbuf.buf_align = SZ_2M;
> 
> Nit: MIN_KIMG_ALIGN ?

OK.

> 
> > +	/* Adjust kernel segment with TEXT_OFFSET */
> > +	kbuf.memsz += text_offset;
> > +
> > +	ret = kexec_add_buffer(&kbuf);
> > +	if (ret)
> > +		goto out;
> 
> You just return in the error cases above but here you goto ... the return
> statement at the end. Seems a bit odd.

Will fix it.

> 
> With the memcmp() thing fixed:
> Reviewed-by: James Morse <james.morse@xxxxxxx>

Always appreciate you reviewing.

-Takahiro AKASHI


> 
> Thanks,
> 
> James