Re: [PATCH v3 2/3] arm64: implement update_fdt_pgprot()

From: Rob Herring <robh+dt@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Hsin-Yi Wang <hsinyi@chromium.org>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	devicetree@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@linux.ibm.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Miles Chen <miles.chen@mediatek.com>,
	James Morse <james.morse@arm.com>,
	Andrew Murray <andrew.murray@arm.com>,
	Chintan Pandya <cpandya@codeaurora.org>,
	Jun Yao <yaojun8558363@gmail.com>, Yu Zhao <yuzhao@google.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Laura Abbott <labbott@redhat.com>,
	Stephen Boyd <swboyd@chromium.org>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v3 2/3] arm64: implement update_fdt_pgprot()
Date: Thu, 16 May 2019 10:17:56 -0500	[thread overview]
Message-ID: <CAL_Jsq+Tmht+oxdmjtyhzwrYw4NNAkVnimAuEGODC_2fAprSjg@mail.gmail.com> (raw)
In-Reply-To: <20190516144303.GF43059@lakrids.cambridge.arm.com>

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