| Message ID | 20190516102817.188519-2-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] include/of_fdt.h: add a weak arch hook to update fdt pgprot | expand |
On Thu, May 16, 2019 at 5:28 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > Basically does similar things like __fixmap_remap_fdt(). It's supposed > to be called after fixmap_remap_fdt() is called at least once, so region > checking can be skipped. Since it needs to know dt physical address, make > a copy of the value of __fdt_pointer. > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > arch/arm64/kernel/setup.c | 2 ++ > arch/arm64/mm/mmu.c | 17 +++++++++++++++++ > 2 files changed, 19 insertions(+) Why not just map the FDT R/W at the start and change it to RO just before calling unflatten_device_tree? Then all the FDT scanning functions or any future fixups we need can just assume R/W. That is essentially what Stephen suggested. However, there's no need for a weak function as it can all be done within the arch code. However, I'm still wondering why the FDT needs to be RO in the first place. Rob
On Thu, 16 May 2019 at 16:37, Rob Herring <robh+dt@kernel.org> wrote: > > On Thu, May 16, 2019 at 5:28 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > Basically does similar things like __fixmap_remap_fdt(). It's supposed > > to be called after fixmap_remap_fdt() is called at least once, so region > > checking can be skipped. Since it needs to know dt physical address, make > > a copy of the value of __fdt_pointer. > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > --- > > arch/arm64/kernel/setup.c | 2 ++ > > arch/arm64/mm/mmu.c | 17 +++++++++++++++++ > > 2 files changed, 19 insertions(+) > > Why not just map the FDT R/W at the start and change it to RO just > before calling unflatten_device_tree? Then all the FDT scanning > functions or any future fixups we need can just assume R/W. That is > essentially what Stephen suggested. However, there's no need for a > weak function as it can all be done within the arch code. > > However, I'm still wondering why the FDT needs to be RO in the first place. > It was RO because it could be RO, and we wanted to ensure that it didn't get modified inadvertently (hence the CRC check we added as well) If there is a need for the FDT to be RW, let's make it RW.
On Thu, May 16, 2019 at 09:37:05AM -0500, Rob Herring wrote: > On Thu, May 16, 2019 at 5:28 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > Basically does similar things like __fixmap_remap_fdt(). It's supposed > > to be called after fixmap_remap_fdt() is called at least once, so region > > checking can be skipped. Since it needs to know dt physical address, make > > a copy of the value of __fdt_pointer. > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > --- > > arch/arm64/kernel/setup.c | 2 ++ > > arch/arm64/mm/mmu.c | 17 +++++++++++++++++ > > 2 files changed, 19 insertions(+) > > Why not just map the FDT R/W at the start and change it to RO just > before calling unflatten_device_tree? Then all the FDT scanning > functions or any future fixups we need can just assume R/W. That is > essentially what Stephen suggested. However, there's no need for a > weak function as it can all be done within the arch code. > > However, I'm still wondering why the FDT needs to be RO in the first place. We want to preserve the original FDT in a pristine form for kexec (and when exposed to userspace), and mapping it RO was the easiest way to catch it being randomly modified (e.g. without fixups applied). I'd prefer to keep it RO once we've removed/cleared certain properties from the chosen node that don't make sense to pass on for kexec Thanks, Mark.
On Thu, May 16, 2019 at 10:37 PM Rob Herring <robh+dt@kernel.org> wrote: > > Why not just map the FDT R/W at the start and change it to RO just > before calling unflatten_device_tree? Then all the FDT scanning > functions or any future fixups we need can just assume R/W. That is > essentially what Stephen suggested. However, there's no need for a > weak function as it can all be done within the arch code. > We need to add a new seed for kexec
On Thu, May 16, 2019 at 9:43 AM Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, May 16, 2019 at 09:37:05AM -0500, Rob Herring wrote: > > On Thu, May 16, 2019 at 5:28 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > Basically does similar things like __fixmap_remap_fdt(). It's supposed > > > to be called after fixmap_remap_fdt() is called at least once, so region > > > checking can be skipped. Since it needs to know dt physical address, make > > > a copy of the value of __fdt_pointer. > > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > --- > > > arch/arm64/kernel/setup.c | 2 ++ > > > arch/arm64/mm/mmu.c | 17 +++++++++++++++++ > > > 2 files changed, 19 insertions(+) > > > > Why not just map the FDT R/W at the start and change it to RO just > > before calling unflatten_device_tree? Then all the FDT scanning > > functions or any future fixups we need can just assume R/W. That is > > essentially what Stephen suggested. However, there's no need for a > > weak function as it can all be done within the arch code. > > > > However, I'm still wondering why the FDT needs to be RO in the first place. > > We want to preserve the original FDT in a pristine form for kexec (and > when exposed to userspace), and mapping it RO was the easiest way to > catch it being randomly modified (e.g. without fixups applied). The CRC check already existed for this purpose and that works for every arch including ones where the FDT is copied. BTW, This version of the patchset disables the export to userspace since the CRC will be wrong. > I'd prefer to keep it RO once we've removed/cleared certain properties > from the chosen node that don't make sense to pass on for kexec I want clear rules about when the FDT can be modified or not which are not arch specific. It's really only a question of with what granularity it's made R/W. Wrapping every modification seems like overkill to me. Rob
On Thu, May 16, 2019 at 9:51 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > On Thu, May 16, 2019 at 10:37 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > Why not just map the FDT R/W at the start and change it to RO just > > before calling unflatten_device_tree? Then all the FDT scanning > > functions or any future fixups we need can just assume R/W. That is > > essentially what Stephen suggested. However, there's no need for a > > weak function as it can all be done within the arch code. > > > We need to add a new seed for kexec Doesn't kexec operate on a copy because it already does modifications. Rob
On Thu, May 16, 2019 at 11:32 PM Rob Herring <robh+dt@kernel.org> wrote: > Doesn't kexec operate on a copy because it already does modifications. > Hi Rob, This patch is to assist "[PATCH v3 3/3] fdt: add support for rng-seed" (https://lkml.org/lkml/2019/5/16/257). I thought that by default second kernel would use original fdt, so I write new seed back to original fdt. Might be wrong. ** "[PATCH v3 3/3] fdt: add support for rng-seed" is supposed to handle for adding new seed in kexec case, discussed in v2 (https://lkml.org/lkml/2019/5/13/425) By default (not considering user defines their own fdt), if second kernel uses copied fdt, when is it copied and can we modify that? Thanks!
Hi! On 16/05/2019 17:48, Hsin-Yi Wang wrote: > On Thu, May 16, 2019 at 11:32 PM Rob Herring <robh+dt@kernel.org> wrote: >> Doesn't kexec operate on a copy because it already does modifications. It does! > This patch is to assist "[PATCH v3 3/3] fdt: add support for rng-seed" > (https://lkml.org/lkml/2019/5/16/257). I thought that by default > second kernel would use original fdt, so I write new seed back to > original fdt. Might be wrong. > > ** "[PATCH v3 3/3] fdt: add support for rng-seed" is supposed to > handle for adding new seed in kexec case, discussed in v2 > (https://lkml.org/lkml/2019/5/13/425) > > By default (not considering user defines their own fdt), if second > kernel uses copied fdt, when is it copied and can we modify that? Regular kexec's user-space already updates the dtb for the cmdline and maybe the initrd. For KASLR, it generates its own seed with getrandom(): https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/arch/arm64/kexec-arm64.c#n483 If user-space can do it, user-space should do it! Thanks, James
Quoting James Morse (2019-05-16 10:34:16) > Hi! > > On 16/05/2019 17:48, Hsin-Yi Wang wrote: > > On Thu, May 16, 2019 at 11:32 PM Rob Herring <robh+dt@kernel.org> wrote: > >> Doesn't kexec operate on a copy because it already does modifications. > > It does! > > > This patch is to assist "[PATCH v3 3/3] fdt: add support for rng-seed" > > (https://lkml.org/lkml/2019/5/16/257). I thought that by default > > second kernel would use original fdt, so I write new seed back to > > original fdt. Might be wrong. > > > > ** "[PATCH v3 3/3] fdt: add support for rng-seed" is supposed to > > handle for adding new seed in kexec case, discussed in v2 > > (https://lkml.org/lkml/2019/5/13/425) > > > > By default (not considering user defines their own fdt), if second > > kernel uses copied fdt, when is it copied and can we modify that? > > Regular kexec's user-space already updates the dtb for the cmdline and maybe the initrd. > For KASLR, it generates its own seed with getrandom(): > > https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/arch/arm64/kexec-arm64.c#n483 > > If user-space can do it, user-space should do it! > Doesn't it need to be done in two places? Userspace and also in the kernel when kexec_file_load() is used? At least, I see a bit of code that does kaslr seed updates to the copied dtb in setup_dtb() of arch/arm64/kernel/machine_kexec_file.c that probably needs to get an update for this new property too.
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 413d566405d1..207cbb5f7965 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -66,6 +66,7 @@ static int num_standard_resources; static struct resource *standard_resources; phys_addr_t __fdt_pointer __initdata; +phys_addr_t fdt_pointer; /* * Standard memory resources @@ -292,6 +293,7 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init(); + fdt_pointer = __fdt_pointer; setup_machine_fdt(__fdt_pointer); parse_early_param(); diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index a170c6369a68..196ab4d9e92a 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -32,6 +32,7 @@ #include <linux/io.h> #include <linux/mm.h> #include <linux/vmalloc.h> +#include <linux/of_fdt.h> #include <asm/barrier.h> #include <asm/cputype.h> @@ -953,6 +954,22 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys) return dt_virt; } +extern phys_addr_t fdt_pointer; + +/* Should be called after fixmap_remap_fdt() is called. */ +void update_fdt_pgprot(pgprot_t prot) +{ + u64 dt_virt_base = __fix_to_virt(FIX_FDT); + int offset, size; + + offset = fdt_pointer % SWAPPER_BLOCK_SIZE; + size = fdt_totalsize((void *)dt_virt_base + offset); + + update_mapping_prot(round_down(fdt_pointer, SWAPPER_BLOCK_SIZE), + dt_virt_base, + round_up(offset + size, SWAPPER_BLOCK_SIZE), prot); +} + int __init arch_ioremap_pud_supported(void) { /* only 4k granule supports level 1 block mappings */
Basically does similar things like __fixmap_remap_fdt(). It's supposed to be called after fixmap_remap_fdt() is called at least once, so region checking can be skipped. Since it needs to know dt physical address, make a copy of the value of __fdt_pointer. Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> --- arch/arm64/kernel/setup.c | 2 ++ arch/arm64/mm/mmu.c | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)