Re: [PATCH 10/11] kexec: Support for loading ELF x86_64 images

[Borislav Petkov <bp@alien8.de>]

On Fri, Feb 28, 2014 at 12:11:43PM -0500, Vivek Goyal wrote:
> I was rather thinking of arch/x86/kernel/kexec. But that's for some other
> day. Not part of this patchset. This is alredy too big and I don't want
> to make any changes which are nice to have and bloat the patch size.

Ok.

Btw, we were talking about the "kernel" part of the path. And, as Ingo
said, it is all kernel so arch/x86/kexec should be perfectly fine.

...

> > > +	for (i = 0; i < ehdr->e_phnum; i++) {
> > > +		if (phdrs[i].p_type != PT_LOAD)
> > > +			continue;
> > 
> > newline
> 
> What's that?

Ah, that's: "please add an empty line here" for better readability.

> > > +/* Fill in fields which are usually present in bzImage */
> > > +static int init_linux_parameters(struct boot_params *params)
> > > +{
> > > +	/*
> > > +	 * FIXME: It is odd that the information which comes from kernel
> > > +	 * has to be faked by loading kernel. I guess it is limitation of
> > > +	 * ELF format. Right now keeping it same as kexec-tools
> > > +	 * implementation. But this most likely needs fixing.
> > > +	 */
> > > +	memcpy(&params->hdr.header, "HdrS", 4);
> > > +	params->hdr.version = 0x0206;
> > > +	params->hdr.initrd_addr_max = 0x37FFFFFF;
> > > +	params->hdr.cmdline_size = 2048;
> > > +	return 0;
> > > +}
> > 
> > Why a separate function? Its body is small enough to be merged into
> > elf_x86_64_load.
> 
> Actually this logic shows the limitation of ELF format kernel image.
> This information should be exported by the image so that loader can
> do some verifications. But instead loader is hardcoding this info and
> faking things.
> 
> For example, it should be kernel which tells what's the maximum command
> line size it can handle and then loader can return error if user specified
> command line size is greater than what new kernel can handle.
> 
> Similarly, what's the max address initrd can be loaded at.
> 
> Actually I have copied this code from kexec-tools. And I am wondering
> if some of this is 
> 
> I am not sure why we set hdr.version and hdr.header. Are there any
> assumptions in booth path kernel is making. May be some other code
> down the line is parsing it or it is completely redundant. I think
> I will play with removing setting of hdr.version and hdr.header and
> see how does it go.

Well, this is mandated by the boot protocol, no?

"If the "HdrS" (0x53726448) magic number is not found at offset 0x202,
the boot protocol version is "old".  Loading an old kernel, the
following parameters should be assumed:

        Image type = zImage
        initrd not supported
        Real-mode kernel must be located at 0x90000."

About version 0x0206:

Field name:     cmdline_size
Type:           read
Offset/size:    0x238/4
Protocol:       2.06+

  The maximum size of the command line without the terminating
  zero. This means that the command line can contain at most
  cmdline_size characters. With protocol version 2.05 and earlier, the
  maximum size was 255.

So according to the protocol, cmdline_size should be set by
kexec_file_load and not hardcoded to 2K, if we're mandating protocol
version 2.06.

Maybe hpa can comment on all this.

> so I put it in a separate function because user space code had it that
> way. Also because I did not like this part of the code and this looks
> like a limitation of ELF format, I wanted to isolate it in a separate
> function so that it is easy to spot it.

Right.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--