Re: [PATCH v2 0/2] extend kexec_file_load system call

[Mark Rutland]

On Thu, Aug 11, 2016 at 08:03:56PM -0300, Thiago Jung Bauermann wrote:
> This patch series is from AKASHI Takahiro. I will use it in my next
> version of the kexec_file_load implementation for powerpc, so I am
> rebasing it on top of v4.8-rc1.

[...]

> Original cover letter:
> 
> Device tree blob must be passed to a second kernel on DTB-capable
> archs, like powerpc and arm64, but the current kernel interface
> lacks this support.
> 
> This patch extends kexec_file_load system call by adding an extra
> argument to this syscall so that an arbitrary number of file descriptors
> can be handed out from user space to the kernel.
> 
> See the background [1].
> 
> Please note that the new interface looks quite similar to the current
> system call, but that it won't always mean that it provides the "binary
> compatibility."
> 
> [1] http://lists.infradead.org/pipermail/kexec/2016-June/016276.html

As with the original posting, I have a number of concerns, and I'm
really not keen on this.

* For typical usecases, I do not believe that this is necessary (at
  least for arm64), and generally do not believe that it should be
  necessary for a user to manipulate the DTB (much like the user need
  not manipulate ACPI tables or other FW data structures).

  Other than (potentially) the case of Linux as a flashed-in bootloader,
  I don't see a compelling case for modifying the DTB that could not be
  accomplished in-kernel. For that case, if truly necessary, I think
  that we can get away with something simpler.

* This series adds architecture-specific hooks, but doesn't define what
  the architecture code is expected to do. For example, what is the
  format of the partial DTB? Is it formatted as an overlay, or a regular
  DTB that is expected to be merged somehow?

  I'm afraid that the scope is unbound, and we'll see requests to
  whitelist/blacklist arbitrary nodes or properties in arch code. This
  goes against the original simple design of kexec_file_load. It also
  implies that we're going to have varied architecture-specific
  semantics, and that arch code might not consistently check all that it
  should.
  
* Further, I believe that this offers a lot of scope for unintentionally
  allowing certain modifications to the DTB that we do not want, and
  avoiding that in general is very tricky. e.g. if we allow the
  insertion or modification of nodes, how do we prevent phandle target
  hijacking?

I really don't think that this is a good idea. Please consider this a
NAK from my end.

Thanks,
Mark.