On Tue, Oct 09, 2018 at 11:02:29AM -0700, Frank Rowand wrote:
> On 10/04/18 22:06, AKASHI, Takahiro wrote:
[snip]
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> + u64 addr, u64 size)
> >>>> +{
> >>>> + int addr_cells, size_cells;
> >>
> >> unsigned
> >
> > fdt_[address|size]_cell() returns an int.
>
> I stand corrected. They take advantage of the fact that valid values
> are in the range 0..4 and return a negative value for error.

I'm not sure if the spec formally defines that these values will fit
in a positive 'int'. But if addresses are 8GiB each, you're going to
have much bigger problems putting it in a device tree than this
function.

--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachments:

(No filename) (952.00 B)
signature.asc (849.00 B)
Download all attachments

2018-10-10 01:43:43

by AKASHI Takahiro

[permalink] [raw]

Subject: Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

Frank, Rob,

On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:
> On 10/08/18 17:37, AKASHI, Takahiro wrote:
> > On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
> >> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
> >> <[email protected]> wrote:
> >>>
> >>> Rob,
> >>>
> >>> # I haven't replied to this comment yet.
> >>>
> >>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> >>>> +David Gibson
> >>>>
> >>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> >>>> <[email protected]> wrote:
> >>>>>
> >>>>> These functions will be used later to handle kexec-specific properties
> >>>>> in arm64's kexec_file implementation.
> >>>>>
> >>>>> Signed-off-by: AKASHI Takahiro <[email protected]>
> >>>>> Cc: Rob Herring <[email protected]>
> >>>>> Cc: Frank Rowand <[email protected]>
> >>>>> Cc: [email protected]
> >>>>> ---
> >>>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>>>> include/linux/of_fdt.h | 4 +++
> >>>>> 2 files changed, 60 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>>>> index 800ad252cf9c..c65c31562ccb 100644
> >>>>> --- a/drivers/of/fdt.c
> >>>>> +++ b/drivers/of/fdt.c
> >>>>> @@ -25,6 +25,7 @@
> >>>>> #include <linux/debugfs.h>
> >>>>> #include <linux/serial_core.h>
> >>>>> #include <linux/sysfs.h>
> >>>>> +#include <linux/types.h>
> >>>>>
> >>>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> >>>>> #include <asm/page.h>
> >>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >>>>> #endif
> >>>>>
> >>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>>>> +
> >>>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> >>>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> >>>>> +
> >>>>> +int fdt_prop_len(const char *prop_name, int len)
> >>>>> +{
> >>>>> + return (strlen(prop_name) + 1) +
> >>>>> + sizeof(struct fdt_property) +
> >>>>> + FDT_TAGALIGN(len);
> >>>>
> >>>> Looks like you are using this to calculate how much space you need to
> >>>> allocate in addition to the current DTB for a couple of new or
> >>>> replaced properties. I'm not sure that this calculation is completely
> >>>> accurate. And it is strange there doesn't seem to be any libfdt
> >>>> function for this already. It would be simpler to just add some fixed
> >>>> additional amount.
> >>>>
> >>>> Maybe David G has comments on this?
> >>>>
> >>>>> +}
> >>>>> +
> >>>>
> >>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> >>>> should go into libfdt, but I'm fine with having it in the kernel for
> >>>> now.
> >>>
> >>> I'd like to have this function in the kernel for now.
> >>>
> >>>>> +static void fill_property(void *buf, u64 val64, int cells)
> >>>>> +{
> >>>>> + __be32 val32;
> >>>>> +
> >>>>> + while (cells) {
> >>>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >>>>> + memcpy(buf, &val32, sizeof(val32));
> >>>>> + buf += sizeof(val32);
> >>>>
> >>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >>>
> >>> Are you sure?
> >>> I originally implemented this function in a similar way that fdt_pack_reg()
> >>> was, but, you suggested, in your past comment[1], that we'd be better to
> >>> have of_read_number()-like implementation.
> >>>
> >>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
> >>
> >> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
> >> unaligned accesses with armv5 and earlier h/w.
> >>
> >> My only comment then is I think you can drop the U32_MAX masking.
> >
> > Okay, then I will leave this function, yet renaming it to
> > cpu64_to_fdt_cells() after Frank's comment.
>
> I have second guessed myself and do not like the name I suggested
> because what the function really does is either cpu32 to be32 or
> cpu64 to be64.

Okay.

> I agree with Rob that readability is important here. Instead of
> having a fill_property() function, how about having inline code,
> something like (untested even for thinkos):
>
> prop = buf;
>
> if (addr_cells == 1) {
> *(__be32 *)prop = cpu32_to_be32(addr);
> prop += 4;
> } else {
> *(__be64 *)prop = cpu64_to_be64(addr);
> prop += 8;
> }
>
> if (size_cells == 1)
> *(__be32 *)prop = cpu32_to_be32(size);
> else
> *(__be64 *)prop = cpu64_to_be64(size);
>
> You might want to also give Rob a chance to bike shed on this
> suggestion.

Basically, I don't care either way, but what Rob suggested
is that some architecture(s) might not handle correctly
unaligned memory access here.

I just want to stay tuned with Rob.

Thanks,
-Takahiro Akashi

> -Frank
>
> >
> > Thanks,
> > -Takahiro Akashi
> >>
> >> Rob
> >
>

2018-10-10 02:33:40

by AKASHI Takahiro

[permalink] [raw]

Subject: Re: [PATCH v15 07/16] arm64: add image head flag definitions

Mark,

On Tue, Oct 09, 2018 at 04:04:05PM +0100, Mark Rutland wrote:
> On Tue, Oct 02, 2018 at 04:59:40PM +0900, AKASHI Takahiro wrote:
> > Hi Mark,
> >
> > On Mon, Oct 01, 2018 at 01:52:26PM +0100, Mark Rutland wrote:
> > > On Fri, Sep 28, 2018 at 03:48:32PM +0900, AKASHI Takahiro wrote:
> > > > Those image head's flags will be used later by kexec_file loader.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <[email protected]>
> > > > Cc: Catalin Marinas <[email protected]>
> > > > Cc: Will Deacon <[email protected]>
> > > > Acked-by: James Morse <[email protected]>
> > > > ---
> > > > arch/arm64/include/asm/boot.h | 15 +++++++++++++++
> > > > arch/arm64/kernel/head.S | 2 +-
> > > > 2 files changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
> > > > index 355e552a9175..0bab7eed3012 100644
> > > > --- a/arch/arm64/include/asm/boot.h
> > > > +++ b/arch/arm64/include/asm/boot.h
> > > > @@ -5,6 +5,21 @@
> > > >
> > > > #include <asm/sizes.h>
> > > >
> > > > +#define ARM64_MAGIC "ARM\x64"
> > > > +
> > > > +#define HEAD_FLAG_BE_SHIFT 0
> > > > +#define HEAD_FLAG_PAGE_SIZE_SHIFT 1
> > > > +#define HEAD_FLAG_BE_MASK 0x1
> > > > +#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
> > > > +
> > > > +#define HEAD_FLAG_BE 1
> > >
> > > These already exist in some form in arch/arm64/kernel/image.h; can we
> > > please factor those out rather than duplicating them?
> >
> > Sure.
> >
> > > I'd be happy if you'd update image.h to use the new HEAD_FLAG_* names,
> > > and removed the old definitions.
> >
> > I want to make sure two things;
> >
> > 1. Do you assume all the existing __HEAD_FLAG_xyz's NOT be renamed
> > (say, to HEAD_FLAG_xyz)?
>
> I'm perfectly happy for them to be renamed, I just don't want duplicate
> definitions.

IMO, it's not duplication.
HEAD_FLAG_xyz is a definition of a specific field. On the other hand,
__HEAD_FLAG_xyz is a value to be put in a kernel header.
To be clear, please look at my current code attached below.

> Let's rename them to ARM64_IMAGE_FLAG_<foo>, and place them in a new
> header, arch/arm64/include/asm/image.h, which arch/arm64/kernel/image.h
> can include and make use of.

Okay.

> > 2. Do you mind removing this check in image.h?
> > (we also need to manage 'CONFIG_EFI' part of image.h.)
>
> What exactly do we need from that? AFAICT that's all linker script
> stuff that shouldn't matter for kexec.

You're right if all the definitions, as you suggested above,
are moved to a new header, asm/image.h.

-Takahiro Akashi

> Thanks,
> Mark.

arch/arm64/kernel/image.h
---8<---
/*
* Linker script macros to generate Image header fields.
*
* Copyright (C) 2014 ARM Ltd.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
#ifndef __ASM_IMAGE_H
#define __ASM_IMAGE_H

#define HEAD_FLAG_BE_SHIFT 0
#define HEAD_FLAG_PAGE_SIZE_SHIFT (HEAD_FLAG_BE_SHIFT + 1)
#define HEAD_FLAG_PHYS_BASE_SHIFT (HEAD_FLAG_PAGE_SIZE_SHIFT + 2)
#define HEAD_FLAG_BE_MASK 0x1
#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
#define HEAD_FLAG_PHYS_BASE_MASK 0x1

#define HEAD_FLAG_LE 0
#define HEAD_FLAG_BE 1
#define HEAD_FLAG_PAGE_SIZE_4K 1
#define HEAD_FLAG_PAGE_SIZE_16K 2
#define HEAD_FLAG_PAGE_SIZE_64K 3
#define HEAD_FLAG_PHYS_BASE 1

#define head_flag_field(flags, field) \
(((flags) >> field##_SHIFT) & field##_MASK)

#ifdef LINKER_SCRIPT
/*
* There aren't any ELF relocations we can use to endian-swap values known only
* at link time (e.g. the subtraction of two symbol addresses), so we must get
* the linker to endian-swap certain values before emitting them.
*
* Note that, in order for this to work when building the ELF64 PIE executable
* (for KASLR), these values should not be referenced via R_AARCH64_ABS64
* relocations, since these are fixed up at runtime rather than at build time
* when PIE is in effect. So we need to split them up in 32-bit high and low
* words.
*/
#ifdef CONFIG_CPU_BIG_ENDIAN
#define DATA_LE32(data) \
((((data) & 0x000000ff) << 24) | \
(((data) & 0x0000ff00) << 8) | \
(((data) & 0x00ff0000) >> 8) | \
(((data) & 0xff000000) >> 24))
#else
#define DATA_LE32(data) ((data) & 0xffffffff)
#endif

#define DEFINE_IMAGE_LE64(sym, data) \
sym##_lo32 = DATA_LE32((data) & 0xffffffff); \
sym##_hi32 = DATA_LE32((data) >> 32)

#define __HEAD_FLAG(field) (__HEAD_FLAG_##field << \
HEAD_FLAG_##field##_SHIFT)

#ifdef CONFIG_CPU_BIG_ENDIAN
#define __HEAD_FLAG_BE HEAD_FLAG_BE
#else
#define __HEAD_FLAG_BE HEAD_FLAG_LE
#endif

#define __HEAD_FLAG_PAGE_SIZE ((PAGE_SHIFT - 10) / 2)

#define __HEAD_FLAG_PHYS_BASE HEAD_FLAG_PHYS_BASE

#define __HEAD_FLAGS (__HEAD_FLAG(BE) | \
__HEAD_FLAG(PAGE_SIZE) | \
__HEAD_FLAG(PHYS_BASE))

/*
* These will output as part of the Image header, which should be little-endian
* regardless of the endianness of the kernel. While constant values could be
* endian swapped in head.S, all are done here for consistency.
*/
#define HEAD_SYMBOLS \
DEFINE_IMAGE_LE64(_kernel_size_le, _end - _text); \
DEFINE_IMAGE_LE64(_kernel_offset_le, TEXT_OFFSET); \
DEFINE_IMAGE_LE64(_kernel_flags_le, __HEAD_FLAGS);

#ifdef CONFIG_EFI

__efistub_stext_offset = stext - _text;

/*
* Prevent the symbol aliases below from being emitted into the kallsyms
* table, by forcing them to be absolute symbols (which are conveniently
* ignored by scripts/kallsyms) rather than section relative symbols.
* The distinction is only relevant for partial linking, and only for symbols
* that are defined within a section declaration (which is not the case for
* the definitions below) so the resulting values will be identical.
*/
#define KALLSYMS_HIDE(sym) ABSOLUTE(sym)

/*
* The EFI stub has its own symbol namespace prefixed by __efistub_, to
* isolate it from the kernel proper. The following symbols are legally
* accessed by the stub, so provide some aliases to make them accessible.
* Only include data symbols here, or text symbols of functions that are
* guaranteed to be safe when executed at another offset than they were
* linked at. The routines below are all implemented in assembler in a
* position independent manner
*/
__efistub_memcmp = KALLSYMS_HIDE(__pi_memcmp);
__efistub_memchr = KALLSYMS_HIDE(__pi_memchr);
__efistub_memcpy = KALLSYMS_HIDE(__pi_memcpy);
__efistub_memmove = KALLSYMS_HIDE(__pi_memmove);
__efistub_memset = KALLSYMS_HIDE(__pi_memset);
__efistub_strlen = KALLSYMS_HIDE(__pi_strlen);
__efistub_strnlen = KALLSYMS_HIDE(__pi_strnlen);
__efistub_strcmp = KALLSYMS_HIDE(__pi_strcmp);
__efistub_strncmp = KALLSYMS_HIDE(__pi_strncmp);
__efistub_strrchr = KALLSYMS_HIDE(__pi_strrchr);
__efistub___flush_dcache_area = KALLSYMS_HIDE(__pi___flush_dcache_area);

#ifdef CONFIG_KASAN
__efistub___memcpy = KALLSYMS_HIDE(__pi_memcpy);
__efistub___memmove = KALLSYMS_HIDE(__pi_memmove);
__efistub___memset = KALLSYMS_HIDE(__pi_memset);
#endif

__efistub__text = KALLSYMS_HIDE(_text);
__efistub__end = KALLSYMS_HIDE(_end);
__efistub__edata = KALLSYMS_HIDE(_edata);
__efistub_screen_info = KALLSYMS_HIDE(screen_info);

#endif /* CONFIG_EFI */

#endif /* LINKER_SCRIPT */

#endif /* __ASM_IMAGE_H */

2018-10-10 02:50:25

by Frank Rowand

[permalink] [raw]

Subject: Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

On 10/09/18 18:14, AKASHI, Takahiro wrote:
> Frank, Rob,
>
> On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:
>> On 10/08/18 17:37, AKASHI, Takahiro wrote:
>>> On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
>>>> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
>>>> <[email protected]> wrote:
>>>>>
>>>>> Rob,
>>>>>
>>>>> # I haven't replied to this comment yet.
>>>>>
>>>>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
>>>>>> +David Gibson
>>>>>>
>>>>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> These functions will be used later to handle kexec-specific properties
>>>>>>> in arm64's kexec_file implementation.
>>>>>>>
>>>>>>> Signed-off-by: AKASHI Takahiro <[email protected]>
>>>>>>> Cc: Rob Herring <[email protected]>
>>>>>>> Cc: Frank Rowand <[email protected]>
>>>>>>> Cc: [email protected]
>>>>>>> ---
>>>>>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>> include/linux/of_fdt.h | 4 +++
>>>>>>> 2 files changed, 60 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>>>>> --- a/drivers/of/fdt.c
>>>>>>> +++ b/drivers/of/fdt.c
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>> #include <linux/debugfs.h>
>>>>>>> #include <linux/serial_core.h>
>>>>>>> #include <linux/sysfs.h>
>>>>>>> +#include <linux/types.h>
>>>>>>>
>>>>>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
>>>>>>> #include <asm/page.h>
>>>>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>>>>> #endif
>>>>>>>
>>>>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>>>> +
>>>>>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
>>>>>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
>>>>>>> +
>>>>>>> +int fdt_prop_len(const char *prop_name, int len)
>>>>>>> +{
>>>>>>> + return (strlen(prop_name) + 1) +
>>>>>>> + sizeof(struct fdt_property) +
>>>>>>> + FDT_TAGALIGN(len);
>>>>>>
>>>>>> Looks like you are using this to calculate how much space you need to
>>>>>> allocate in addition to the current DTB for a couple of new or
>>>>>> replaced properties. I'm not sure that this calculation is completely
>>>>>> accurate. And it is strange there doesn't seem to be any libfdt
>>>>>> function for this already. It would be simpler to just add some fixed
>>>>>> additional amount.
>>>>>>
>>>>>> Maybe David G has comments on this?
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>>>>> should go into libfdt, but I'm fine with having it in the kernel for
>>>>>> now.
>>>>>
>>>>> I'd like to have this function in the kernel for now.
>>>>>
>>>>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>>>>>> +{
>>>>>>> + __be32 val32;
>>>>>>> +
>>>>>>> + while (cells) {
>>>>>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>>>>> + memcpy(buf, &val32, sizeof(val32));
>>>>>>> + buf += sizeof(val32);
>>>>>>
>>>>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>>>
>>>>> Are you sure?
>>>>> I originally implemented this function in a similar way that fdt_pack_reg()
>>>>> was, but, you suggested, in your past comment[1], that we'd be better to
>>>>> have of_read_number()-like implementation.
>>>>>
>>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
>>>>
>>>> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
>>>> unaligned accesses with armv5 and earlier h/w.
>>>>
>>>> My only comment then is I think you can drop the U32_MAX masking.
>>>
>>> Okay, then I will leave this function, yet renaming it to
>>> cpu64_to_fdt_cells() after Frank's comment.
>>
>> I have second guessed myself and do not like the name I suggested
>> because what the function really does is either cpu32 to be32 or
>> cpu64 to be64.
>
> Okay.
>
>> I agree with Rob that readability is important here. Instead of
>> having a fill_property() function, how about having inline code,
>> something like (untested even for thinkos):
>>
>> prop = buf;
>>
>> if (addr_cells == 1) {
>> *(__be32 *)prop = cpu32_to_be32(addr);
>> prop += 4;
>> } else {
>> *(__be64 *)prop = cpu64_to_be64(addr);
>> prop += 8;
>> }
>>
>> if (size_cells == 1)
>> *(__be32 *)prop = cpu32_to_be32(size);
>> else
>> *(__be64 *)prop = cpu64_to_be64(size);
>>
>> You might want to also give Rob a chance to bike shed on this
>> suggestion.
>
> Basically, I don't care either way, but what Rob suggested
> is that some architecture(s) might not handle correctly
> unaligned memory access here.

Good point about unaligned memory access. That rules out my
suggestion above. But if using the fill_property() implementation,
either come up with a name that better describes what the function
does or add a function header comment that gives the reader a
clue as to what the function accomplishes.

-Frank

> I just want to stay tuned with Rob.
>
> Thanks,
> -Takahiro Akashi
>
>> -Frank
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>>
>>>> Rob
>>>
>>
>

2018-10-10 06:51:41

by AKASHI Takahiro

[permalink] [raw]

Subject: Re: [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel

Mark,

On Tue, Oct 09, 2018 at 04:28:00PM +0100, Mark Rutland wrote:
> On Fri, Sep 28, 2018 at 03:48:36PM +0900, 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/
> >
> > Signed-off-by: AKASHI Takahiro <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Reviewed-by: James Morse <[email protected]>
> > ---
> > arch/arm64/include/asm/kexec.h | 28 +++++++
> > arch/arm64/kernel/Makefile | 2 +-
> > arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
> > arch/arm64/kernel/machine_kexec_file.c | 1 +
> > 4 files changed, 138 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/kernel/kexec_image.c
> >
> > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > index 157b2897d911..5e673481b3a3 100644
> > --- a/arch/arm64/include/asm/kexec.h
> > +++ b/arch/arm64/include/asm/kexec.h
> > @@ -101,6 +101,34 @@ struct kimage_arch {
> > unsigned long dtb_mem;
> > };
> >
> > +/**
> > + * struct arm64_image_header - arm64 kernel image header
> > + * See Documentation/arm64/booting.txt for details
> > + *
> > + * @mz_magic: DOS header magic number ('MZ', optional)
>
> Please just call this code0. If CONFIG_EFI is disabled, it is not 'MZ'.

How about this?
(This definition will go into a new header, asm/image.h.)

---8<---
/*
* struct arm64_image_header - arm64 kernel image header
* See Documentation/arm64/booting.txt for details
*
* @code0: Executable code, or
* @mz_header alternatively used for part of MZ header
* @code1: Executable code
* @text_offset: Image load offset
* @image_size: Effective Image size
* @flags: kernel flags
* @reserved: reserved
* @magic: Magic number
* @reserved5: reserved, or
* @pe_header: alternatively used for PE COFF offset
*/

struct arm64_image_header {
union {
__le32 code0;
struct {
__le16 magic;
__le16 pad;
} mz_header;
};
__le32 code1;
__le64 text_offset;
__le64 image_size;
__le64 flags;
__le64 reserved[3];
__le32 magic;
union {
__le32 reserved5;
__le32 pe_header;
};
};
--->8---

> > + * @code1: Instruction (branch to stext)
> > + * @text_offset: Image load offset
> > + * @image_size: Effective image size
> > + * @flags: Bit-field flags
> > + * @reserved: Reserved
> > + * @magic: Magic number
> > + * @pe_header: Offset to PE COFF header (optional)
> > + **/
> > +
> > +struct arm64_image_header {
> > + __le16 mz_magic; /* also code0 */
> > + __le16 pad;
>
> Likewise, just have __le32 code0 here, please.
>
> > + __le32 code1;
> > + __le64 text_offset;
> > + __le64 image_size;
> > + __le64 flags;
> > + __le64 reserved[3];
> > + __le32 magic;
> > + __le32 pe_header;
> > +};
> > +
> > +extern const struct kexec_file_ops kexec_image_ops;
> > +
> > struct kimage;
> >
> > extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
> > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > index 030a39bff117..48868255f09c 100644
> > --- a/arch/arm64/kernel/Makefile
> > +++ b/arch/arm64/kernel/Makefile
> > @@ -51,7 +51,7 @@ arm64-obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
> > arm64-obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o
> > arm64-obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \
> > cpu-reset.o
> > -arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o
> > +arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o
> > arm64-obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o
> > arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
> > arm64-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> > diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
> > new file mode 100644
> > index 000000000000..d64f5e9f9d22
> > --- /dev/null
> > +++ b/arch/arm64/kernel/kexec_image.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Kexec image loader
> > +
> > + * Copyright (C) 2018 Linaro Limited
> > + * Author: AKASHI Takahiro <[email protected]>
> > + */
> > +
> > +#define pr_fmt(fmt) "kexec_file(Image): " fmt
> > +
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/kexec.h>
> > +#include <linux/string.h>
> > +#include <asm/boot.h>
> > +#include <asm/byteorder.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/memory.h>
> > +
> > +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(h->magic)))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +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)
> > +{
> > + struct arm64_image_header *h;
> > + u64 flags, value;
> > + struct kexec_buf kbuf;
> > + unsigned long text_offset;
> > + struct kexec_segment *kernel_segment;
> > + int ret;
> > +
> > + /* Don't support old kernel */
> > + h = (struct arm64_image_header *)kernel;
> > + if (!h->text_offset)
> > + return ERR_PTR(-EINVAL);
>
> It's entirely valid for TEXT_OFFSET to be zero when
> RANDOMIZE_TEXT_OFFSET is selected.
>
> I think you meant to check image_size here.

Right, it's been a bug since the first appearance in v10.

> We could do with a better comment, too, e.g.
>
> /*
> * We require a kernel with an unambiguous Image header. Per
> * Documentation/booting.txt, this is the case when image_size
> * is non-zero (practically speaking, since v3.17).
> */
> if (!h->image_size)
> return ERR_PTR(-EINVAL);
>
> > +
> > + /* Check cpu features */
> > + flags = le64_to_cpu(h->flags);
> > + value = head_flag_field(flags, HEAD_FLAG_BE);
> > + if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
> > + ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
> > + if (!system_supports_mixed_endian())
> > + return ERR_PTR(-EINVAL);
>
> I think this can be simplified:
>
> bool be_image = head_flag_field(flags, HEAD_FLAG_BE);
> bool be_kernel = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
>
> if ((be_image != be_kernel) && !system_supports_mixed_endian)
> return ERR_PTR(-EINVAL);

Okay.

> ... though do we need to police this at all? It's arguably policy given
> the new image has to be signed anyway), and there are other fields that
> could fall into that category in future.

The aim here is to prevent any images from being loaded
when there is no question that a new image will never be successfully
kexec'ed since the core on a given SoC obviously doesn't support cpu
features that are assumed and required by a image.

I believe that this check is a good and easy practice to avoid possible
failures before execution.

> > +
> > + value = head_flag_field(flags, HEAD_FLAG_PAGE_SIZE);
> > + if (((value == HEAD_FLAG_PAGE_SIZE_4K) &&
> > + !system_supports_4kb_granule()) ||
> > + ((value == HEAD_FLAG_PAGE_SIZE_64K) &&
> > + !system_supports_64kb_granule()) ||
> > + ((value == HEAD_FLAG_PAGE_SIZE_16K) &&
> > + !system_supports_16kb_granule()))
> > + return ERR_PTR(-EINVAL);
>
> ... likewise here?
>
> > +
> > + /* Load the kernel */
> > + kbuf.image = image;
> > + kbuf.buf_min = 0;
> > + kbuf.buf_max = ULONG_MAX;
> > + kbuf.top_down = false;
> > +
> > + kbuf.buffer = kernel;
> > + kbuf.bufsz = kernel_len;
> > + kbuf.mem = 0;
> > + kbuf.memsz = le64_to_cpu(h->image_size);
> > + text_offset = le64_to_cpu(h->text_offset);
> > + kbuf.buf_align = MIN_KIMG_ALIGN;
> > +
> > + /* Adjust kernel segment with TEXT_OFFSET */
> > + kbuf.memsz += text_offset;
>
> It's very surprising at first glance to add text_offset here, then undo
> that below. This should have a better comment explaining what we're
> doing.

To respect TEXT_OFFSET particularly for older kernels.
Will add some comments here.

> > +
> > + ret = kexec_add_buffer(&kbuf);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + kernel_segment = &image->segment[image->nr_segments - 1];
>
> I'm confused here. When/how can the image have muliple segments?

kexec_add_buffer() allocates one contiguous region, and
so nr_segments should be 1. But I think using nr_segments is resilient
if some buffer may be added before this line.
(Historically, in my old patches, a buffer for elfcoreheader was added
before a kernel segument was added to image->segment[].)

> > + kernel_segment->mem += text_offset;
> > + kernel_segment->memsz -= text_offset;
> > + image->start = kernel_segment->mem;
>
> As above, I don't like the fact that we're altering the result of
> kexec_add_buffer here. It feels fragile, even if it works today.
>
> Can we teach the core kexec buffer code that we need an offset from an
> aligned base?

Adding another parameter to kexec_add_buffer() would be simple to do,
but I doubt that there is any difference since how to fill an allocated
segment with some data is totally up to the caller (arch-specific).

(That said, let me think twice.)

Thanks,
-Takahiro Akashi

> Thanks,
> Mark.

2018-10-10 09:48:45

by Mark Rutland

[permalink] [raw]

Subject: Re: [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel

On Wed, Oct 10, 2018 at 03:52:37PM +0900, AKASHI Takahiro wrote:
> Mark,
>
> On Tue, Oct 09, 2018 at 04:28:00PM +0100, Mark Rutland wrote:
> > On Fri, Sep 28, 2018 at 03:48:36PM +0900, 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/
> > >
> > > Signed-off-by: AKASHI Takahiro <[email protected]>
> > > Cc: Catalin Marinas <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Reviewed-by: James Morse <[email protected]>
> > > ---
> > > arch/arm64/include/asm/kexec.h | 28 +++++++
> > > arch/arm64/kernel/Makefile | 2 +-
> > > arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
> > > arch/arm64/kernel/machine_kexec_file.c | 1 +
> > > 4 files changed, 138 insertions(+), 1 deletion(-)
> > > create mode 100644 arch/arm64/kernel/kexec_image.c
> > >
> > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > index 157b2897d911..5e673481b3a3 100644
> > > --- a/arch/arm64/include/asm/kexec.h
> > > +++ b/arch/arm64/include/asm/kexec.h
> > > @@ -101,6 +101,34 @@ struct kimage_arch {
> > > unsigned long dtb_mem;
> > > };
> > >
> > > +/**
> > > + * struct arm64_image_header - arm64 kernel image header
> > > + * See Documentation/arm64/booting.txt for details
> > > + *
> > > + * @mz_magic: DOS header magic number ('MZ', optional)
> >
> > Please just call this code0. If CONFIG_EFI is disabled, it is not 'MZ'.
>
> How about this?
> (This definition will go into a new header, asm/image.h.)
>
> ---8<---
> /*
> * struct arm64_image_header - arm64 kernel image header
> * See Documentation/arm64/booting.txt for details
> *
> * @code0: Executable code, or
> * @mz_header alternatively used for part of MZ header
> * @code1: Executable code
> * @text_offset: Image load offset
> * @image_size: Effective Image size
> * @flags: kernel flags
> * @reserved: reserved
> * @magic: Magic number
> * @reserved5: reserved, or
> * @pe_header: alternatively used for PE COFF offset
> */
>
> struct arm64_image_header {
> union {
> __le32 code0;
> struct {
> __le16 magic;
> __le16 pad;
> } mz_header;
> };
> __le32 code1;
> __le64 text_offset;
> __le64 image_size;
> __le64 flags;
> __le64 reserved[3];
> __le32 magic;
> union {
> __le32 reserved5;
> __le32 pe_header;
> };
> };

Do we care about the MZ header?

The definition of the Image header in Documentation/arm64/booting.txt is:

u32 code0; /* Executable code */
u32 code1; /* Executable code */
u64 text_offset; /* Image load offset, little endian */
u64 image_size; /* Effective Image size, little endian */
u64 flags; /* kernel flags, little endian */
u64 res2 = 0; /* reserved */
u64 res3 = 0; /* reserved */
u64 res4 = 0; /* reserved */
u32 magic = 0x644d5241; /* Magic number, little endian, "ARM\x64" */
u32 res5; /* reserved (used for PE COFF offset) */

I'd prefer that we aligned our header definition with that, rather than
diverging from it.

If we need to look at the MZ magic to determine if the Image is a valid PE
binary, can't we just cast to the existing struct mz_hdr from <linux/pe.h> to
do that?

[...]

> > > + /* Check cpu features */
> > > + flags = le64_to_cpu(h->flags);
> > > + value = head_flag_field(flags, HEAD_FLAG_BE);
> > > + if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
> > > + ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
> > > + if (!system_supports_mixed_endian())
> > > + return ERR_PTR(-EINVAL);
> >
> > I think this can be simplified:
> >
> > bool be_image = head_flag_field(flags, HEAD_FLAG_BE);
> > bool be_kernel = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
> >
> > if ((be_image != be_kernel) && !system_supports_mixed_endian)
> > return ERR_PTR(-EINVAL);
>
> Okay.
>
> > ... though do we need to police this at all? It's arguably policy given
> > the new image has to be signed anyway), and there are other fields that
> > could fall into that category in future.
>
> The aim here is to prevent any images from being loaded
> when there is no question that a new image will never be successfully
> kexec'ed since the core on a given SoC obviously doesn't support cpu
> features that are assumed and required by a image.
>
> I believe that this check is a good and easy practice to avoid possible
> failures before execution.

My only concern is that this is arguably placing some policy in the
kernel, and I don't want to set the expectation that we'll do this for
other things in future, as that becomes a maintenance nightmare.

I'm not necessarily opposed to these specific checks, given they're
simple. Just wanted to make sure that we've thought about it.

Thanks,
Mark.