[PATCH v2 0/2] extend kexec_file_load system call

[PATCH v2 0/2] extend kexec_file_load system call

[Thiago Jung Bauermann]

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.

I dropped the patch which adds __NR_kexec_file_load to
<asm-generic/unistd.h> for simplicity, since the powerpc patches already
add it to powerpc's <asm/unistd.h>. I don't know which approach is
better.

The first patch in this series is unchanged from v1.

The second patch is the same one I posted on July 26th. It has the
following changes from v1:

- Added the arch_kexec_verify_buffer hook, where each architecture can
  verify if the DTB is safe to load.
- Renamed KEXEC_FILE_TYPE_DTB to KEXEC_FILE_TYPE_PARTIAL_DTB.
- Limited max number of fds to KEXEC_SEGMENT_MAX.
- Changed to use fixed size buffer for fdset instead of allocating it.
- Changed to return -EINVAL if an unknown file type is found in fdset.

I am also posting a new version of the kexec_file_load syscall
implementation for powerpc which uses the arch_kexec_verify_buffer hook
to enforce a whitelist of nodes and properties that userspace can pass to
the next kernel, as suggested by Michael Ellerman.

You can find it in a new patch in the powerpc series called
"powerpc: Allow userspace to set device tree properties in kexec_file_load"

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

AKASHI Takahiro (1):
  kexec: add dtb info to struct kimage

Thiago Jung Bauermann (1):
  kexec: extend kexec_file_load system call

 include/linux/fs.h         |  1 +
 include/linux/kexec.h      | 10 ++++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 ++++++++++++
 kernel/kexec_file.c        | 86 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 114 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH v2 0/2] extend kexec_file_load system call

[Thiago Jung Bauermann]

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.

I dropped the patch which adds __NR_kexec_file_load to
<asm-generic/unistd.h> for simplicity, since the powerpc patches already
add it to powerpc's <asm/unistd.h>. I don't know which approach is
better.

The first patch in this series is unchanged from v1.

The second patch is the same one I posted on July 26th. It has the
following changes from v1:

- Added the arch_kexec_verify_buffer hook, where each architecture can
  verify if the DTB is safe to load.
- Renamed KEXEC_FILE_TYPE_DTB to KEXEC_FILE_TYPE_PARTIAL_DTB.
- Limited max number of fds to KEXEC_SEGMENT_MAX.
- Changed to use fixed size buffer for fdset instead of allocating it.
- Changed to return -EINVAL if an unknown file type is found in fdset.

I am also posting a new version of the kexec_file_load syscall
implementation for powerpc which uses the arch_kexec_verify_buffer hook
to enforce a whitelist of nodes and properties that userspace can pass to
the next kernel, as suggested by Michael Ellerman.

You can find it in a new patch in the powerpc series called
"powerpc: Allow userspace to set device tree properties in kexec_file_load"

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

AKASHI Takahiro (1):
  kexec: add dtb info to struct kimage

Thiago Jung Bauermann (1):
  kexec: extend kexec_file_load system call

 include/linux/fs.h         |  1 +
 include/linux/kexec.h      | 10 ++++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 ++++++++++++
 kernel/kexec_file.c        | 86 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 114 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH v2 0/2] extend kexec_file_load system call

[Thiago Jung Bauermann]

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.

I dropped the patch which adds __NR_kexec_file_load to
<asm-generic/unistd.h> for simplicity, since the powerpc patches already
add it to powerpc's <asm/unistd.h>. I don't know which approach is
better.

The first patch in this series is unchanged from v1.

The second patch is the same one I posted on July 26th. It has the
following changes from v1:

- Added the arch_kexec_verify_buffer hook, where each architecture can
  verify if the DTB is safe to load.
- Renamed KEXEC_FILE_TYPE_DTB to KEXEC_FILE_TYPE_PARTIAL_DTB.
- Limited max number of fds to KEXEC_SEGMENT_MAX.
- Changed to use fixed size buffer for fdset instead of allocating it.
- Changed to return -EINVAL if an unknown file type is found in fdset.

I am also posting a new version of the kexec_file_load syscall
implementation for powerpc which uses the arch_kexec_verify_buffer hook
to enforce a whitelist of nodes and properties that userspace can pass to
the next kernel, as suggested by Michael Ellerman.

You can find it in a new patch in the powerpc series called
"powerpc: Allow userspace to set device tree properties in kexec_file_load"

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

AKASHI Takahiro (1):
  kexec: add dtb info to struct kimage

Thiago Jung Bauermann (1):
  kexec: extend kexec_file_load system call

 include/linux/fs.h         |  1 +
 include/linux/kexec.h      | 10 ++++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 ++++++++++++
 kernel/kexec_file.c        | 86 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 114 insertions(+), 9 deletions(-)

-- 
1.9.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 1/2] kexec: add dtb info to struct kimage

[Thiago Jung Bauermann]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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 adds dtb buffer information to struct kimage.
When users don't specify dtb explicitly and the one used for the current
kernel can be re-used, this change will be good enough for implementing
kexec_file_load feature.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/linux/kexec.h | 3 +++
 kernel/kexec_file.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index d7437777baaa..4f85d284ed0b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -192,6 +192,9 @@ struct kimage {
 	char *cmdline_buf;
 	unsigned long cmdline_buf_len;
 
+	void *dtb_buf;
+	unsigned long dtb_buf_len;
+
 	/* File operations provided by image loader */
 	struct kexec_file_ops *fops;
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 503bc2d348e5..113af2f219b9 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -92,6 +92,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	vfree(image->initrd_buf);
 	image->initrd_buf = NULL;
 
+	vfree(image->dtb_buf);
+	image->dtb_buf = NULL;
+
 	kfree(image->cmdline_buf);
 	image->cmdline_buf = NULL;
 
-- 
1.9.1

[PATCH v2 1/2] kexec: add dtb info to struct kimage

[Thiago Jung Bauermann]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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 adds dtb buffer information to struct kimage.
When users don't specify dtb explicitly and the one used for the current
kernel can be re-used, this change will be good enough for implementing
kexec_file_load feature.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/linux/kexec.h | 3 +++
 kernel/kexec_file.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index d7437777baaa..4f85d284ed0b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -192,6 +192,9 @@ struct kimage {
 	char *cmdline_buf;
 	unsigned long cmdline_buf_len;
 
+	void *dtb_buf;
+	unsigned long dtb_buf_len;
+
 	/* File operations provided by image loader */
 	struct kexec_file_ops *fops;
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 503bc2d348e5..113af2f219b9 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -92,6 +92,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	vfree(image->initrd_buf);
 	image->initrd_buf = NULL;
 
+	vfree(image->dtb_buf);
+	image->dtb_buf = NULL;
+
 	kfree(image->cmdline_buf);
 	image->cmdline_buf = NULL;
 
-- 
1.9.1

[PATCH v2 1/2] kexec: add dtb info to struct kimage

[Thiago Jung Bauermann]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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 adds dtb buffer information to struct kimage.
When users don't specify dtb explicitly and the one used for the current
kernel can be re-used, this change will be good enough for implementing
kexec_file_load feature.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/linux/kexec.h | 3 +++
 kernel/kexec_file.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index d7437777baaa..4f85d284ed0b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -192,6 +192,9 @@ struct kimage {
 	char *cmdline_buf;
 	unsigned long cmdline_buf_len;
 
+	void *dtb_buf;
+	unsigned long dtb_buf_len;
+
 	/* File operations provided by image loader */
 	struct kexec_file_ops *fops;
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 503bc2d348e5..113af2f219b9 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -92,6 +92,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	vfree(image->initrd_buf);
 	image->initrd_buf = NULL;
 
+	vfree(image->dtb_buf);
+	image->dtb_buf = NULL;
+
 	kfree(image->cmdline_buf);
 	image->cmdline_buf = NULL;
 
-- 
1.9.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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.

	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
				 unsigned long cmdline_len,
				 const char __user *cmdline_ptr,
				 unsigned long flags,
				 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

	struct kexec_fdset {
		int nr_fds;
		struct kexec_file_fd fds[0];
	}

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  7 ++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 ++++++++++++
 kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..847d9c31f428 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
+	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
 	id(POLICY, security-policy)		\
 	id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
 	kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
 	kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES           (4096)
 #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include <linux/types.h>
 #include <linux/aio_abi.h>
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 				    unsigned long cmdline_len,
 				    const char __user *cmdline_ptr,
-				    unsigned long flags);
+				    unsigned long flags,
+				    const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_EXTRA_FDS	0x00000008
+
+enum kexec_file_type {
+	KEXEC_FILE_TYPE_KERNEL,
+	KEXEC_FILE_TYPE_INITRAMFS,
+
+	/*
+	 * Device Tree Blob containing just the nodes and properties that
+	 * the kexec_file_load caller wants to add or modify.
+	 */
+	KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+	enum kexec_file_type type;
+	int fd;
+};
+
+struct kexec_fdset {
+	int nr_fds;
+	struct kexec_file_fd fds[0];
+};
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 113af2f219b9..d6803dd884e2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,9 @@
 #include <linux/vmalloc.h>
 #include "kexec_internal.h"
 
+#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
+				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))
+
 /*
  * Declare these symbols weak so that if architecture provides a purgatory,
  * these will be overridden.
@@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * arch_kexec_verify_buffer() - check that the given kexec file is valid
+ *
+ * Device trees in particular can contain properties that may make the kernel
+ * execute code that it wasn't supposed to (e.g., use the wrong entry point
+ * when calling firmware functions). Because of this, the kernel needs to
+ * verify that it is safe to use the device tree blob passed from userspace.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size)
+{
+	return -EINVAL;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 static int
 kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			     const char __user *cmdline_ptr,
-			     unsigned long cmdline_len, unsigned flags)
+			     unsigned long cmdline_len, unsigned long flags,
+			     const struct kexec_fdset __user *ufdset)
 {
 	int ret = 0;
 	void *ldata;
@@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		image->initrd_buf_len = size;
 	}
 
+	if (flags & KEXEC_FILE_EXTRA_FDS) {
+		int nr_fds, i;
+		size_t fdset_size;
+		char fdset_buf[MAX_FDSET_SIZE];
+		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
+
+		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds > KEXEC_SEGMENT_MAX) {
+			ret = -E2BIG;
+			goto out;
+		}
+
+		fdset_size = sizeof(struct kexec_fdset)
+				+ nr_fds * sizeof(struct kexec_file_fd);
+
+		ret = copy_from_user(fdset, ufdset, fdset_size);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		for (i = 0; i < fdset->nr_fds; i++) {
+			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
+				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
+						&image->dtb_buf, &size, INT_MAX,
+						READING_KEXEC_PARTIAL_DTB);
+				if (ret)
+					goto out;
+				image->dtb_buf_len = size;
+
+				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
+							       image->dtb_buf,
+							       image->dtb_buf_len);
+				if (ret)
+					goto out;
+			} else {
+				pr_debug("unknown file type %d failed.\n",
+						fdset->fds[i].type);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
 	if (cmdline_len) {
 		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
 		if (!image->cmdline_buf) {
@@ -202,7 +271,8 @@ out:
 static int
 kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 		       int initrd_fd, const char __user *cmdline_ptr,
-		       unsigned long cmdline_len, unsigned long flags)
+		       unsigned long cmdline_len, unsigned long flags,
+		       const struct kexec_fdset __user *ufdset)
 {
 	int ret;
 	struct kimage *image;
@@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	}
 
 	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
-					   cmdline_ptr, cmdline_len, flags);
+					   cmdline_ptr, cmdline_len, flags,
+					   ufdset);
 	if (ret)
 		goto out_free_image;
 
@@ -256,9 +327,9 @@ out_free_image:
 	return ret;
 }
 
-SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
+SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
-		unsigned long, flags)
+		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
 {
 	int ret = 0, i;
 	struct kimage **dest_image, *image;
@@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		kimage_free(xchg(&kexec_crash_image, NULL));
 
 	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
-				     cmdline_len, flags);
+				     cmdline_len, flags, ufdset);
 	if (ret)
 		goto out;
 
-- 
1.9.1

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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.

	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
				 unsigned long cmdline_len,
				 const char __user *cmdline_ptr,
				 unsigned long flags,
				 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

	struct kexec_fdset {
		int nr_fds;
		struct kexec_file_fd fds[0];
	}

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  7 ++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 ++++++++++++
 kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..847d9c31f428 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
+	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
 	id(POLICY, security-policy)		\
 	id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
 	kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
 	kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES           (4096)
 #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include <linux/types.h>
 #include <linux/aio_abi.h>
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 				    unsigned long cmdline_len,
 				    const char __user *cmdline_ptr,
-				    unsigned long flags);
+				    unsigned long flags,
+				    const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_EXTRA_FDS	0x00000008
+
+enum kexec_file_type {
+	KEXEC_FILE_TYPE_KERNEL,
+	KEXEC_FILE_TYPE_INITRAMFS,
+
+	/*
+	 * Device Tree Blob containing just the nodes and properties that
+	 * the kexec_file_load caller wants to add or modify.
+	 */
+	KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+	enum kexec_file_type type;
+	int fd;
+};
+
+struct kexec_fdset {
+	int nr_fds;
+	struct kexec_file_fd fds[0];
+};
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 113af2f219b9..d6803dd884e2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,9 @@
 #include <linux/vmalloc.h>
 #include "kexec_internal.h"
 
+#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
+				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))
+
 /*
  * Declare these symbols weak so that if architecture provides a purgatory,
  * these will be overridden.
@@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * arch_kexec_verify_buffer() - check that the given kexec file is valid
+ *
+ * Device trees in particular can contain properties that may make the kernel
+ * execute code that it wasn't supposed to (e.g., use the wrong entry point
+ * when calling firmware functions). Because of this, the kernel needs to
+ * verify that it is safe to use the device tree blob passed from userspace.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size)
+{
+	return -EINVAL;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 static int
 kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			     const char __user *cmdline_ptr,
-			     unsigned long cmdline_len, unsigned flags)
+			     unsigned long cmdline_len, unsigned long flags,
+			     const struct kexec_fdset __user *ufdset)
 {
 	int ret = 0;
 	void *ldata;
@@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		image->initrd_buf_len = size;
 	}
 
+	if (flags & KEXEC_FILE_EXTRA_FDS) {
+		int nr_fds, i;
+		size_t fdset_size;
+		char fdset_buf[MAX_FDSET_SIZE];
+		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
+
+		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds > KEXEC_SEGMENT_MAX) {
+			ret = -E2BIG;
+			goto out;
+		}
+
+		fdset_size = sizeof(struct kexec_fdset)
+				+ nr_fds * sizeof(struct kexec_file_fd);
+
+		ret = copy_from_user(fdset, ufdset, fdset_size);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		for (i = 0; i < fdset->nr_fds; i++) {
+			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
+				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
+						&image->dtb_buf, &size, INT_MAX,
+						READING_KEXEC_PARTIAL_DTB);
+				if (ret)
+					goto out;
+				image->dtb_buf_len = size;
+
+				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
+							       image->dtb_buf,
+							       image->dtb_buf_len);
+				if (ret)
+					goto out;
+			} else {
+				pr_debug("unknown file type %d failed.\n",
+						fdset->fds[i].type);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
 	if (cmdline_len) {
 		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
 		if (!image->cmdline_buf) {
@@ -202,7 +271,8 @@ out:
 static int
 kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 		       int initrd_fd, const char __user *cmdline_ptr,
-		       unsigned long cmdline_len, unsigned long flags)
+		       unsigned long cmdline_len, unsigned long flags,
+		       const struct kexec_fdset __user *ufdset)
 {
 	int ret;
 	struct kimage *image;
@@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	}
 
 	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
-					   cmdline_ptr, cmdline_len, flags);
+					   cmdline_ptr, cmdline_len, flags,
+					   ufdset);
 	if (ret)
 		goto out_free_image;
 
@@ -256,9 +327,9 @@ out_free_image:
 	return ret;
 }
 
-SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
+SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
-		unsigned long, flags)
+		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
 {
 	int ret = 0, i;
 	struct kimage **dest_image, *image;
@@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		kimage_free(xchg(&kexec_crash_image, NULL));
 
 	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
-				     cmdline_len, flags);
+				     cmdline_len, flags, ufdset);
 	if (ret)
 		goto out;
 
-- 
1.9.1

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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.

	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
				 unsigned long cmdline_len,
				 const char __user *cmdline_ptr,
				 unsigned long flags,
				 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

	struct kexec_fdset {
		int nr_fds;
		struct kexec_file_fd fds[0];
	}

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  7 ++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 ++++++++++++
 kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..847d9c31f428 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
+	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
 	id(POLICY, security-policy)		\
 	id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
 	kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
 	kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES           (4096)
 #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include <linux/types.h>
 #include <linux/aio_abi.h>
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 				    unsigned long cmdline_len,
 				    const char __user *cmdline_ptr,
-				    unsigned long flags);
+				    unsigned long flags,
+				    const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_EXTRA_FDS	0x00000008
+
+enum kexec_file_type {
+	KEXEC_FILE_TYPE_KERNEL,
+	KEXEC_FILE_TYPE_INITRAMFS,
+
+	/*
+	 * Device Tree Blob containing just the nodes and properties that
+	 * the kexec_file_load caller wants to add or modify.
+	 */
+	KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+	enum kexec_file_type type;
+	int fd;
+};
+
+struct kexec_fdset {
+	int nr_fds;
+	struct kexec_file_fd fds[0];
+};
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 113af2f219b9..d6803dd884e2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,9 @@
 #include <linux/vmalloc.h>
 #include "kexec_internal.h"
 
+#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
+				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))
+
 /*
  * Declare these symbols weak so that if architecture provides a purgatory,
  * these will be overridden.
@@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * arch_kexec_verify_buffer() - check that the given kexec file is valid
+ *
+ * Device trees in particular can contain properties that may make the kernel
+ * execute code that it wasn't supposed to (e.g., use the wrong entry point
+ * when calling firmware functions). Because of this, the kernel needs to
+ * verify that it is safe to use the device tree blob passed from userspace.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size)
+{
+	return -EINVAL;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 static int
 kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			     const char __user *cmdline_ptr,
-			     unsigned long cmdline_len, unsigned flags)
+			     unsigned long cmdline_len, unsigned long flags,
+			     const struct kexec_fdset __user *ufdset)
 {
 	int ret = 0;
 	void *ldata;
@@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		image->initrd_buf_len = size;
 	}
 
+	if (flags & KEXEC_FILE_EXTRA_FDS) {
+		int nr_fds, i;
+		size_t fdset_size;
+		char fdset_buf[MAX_FDSET_SIZE];
+		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
+
+		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds > KEXEC_SEGMENT_MAX) {
+			ret = -E2BIG;
+			goto out;
+		}
+
+		fdset_size = sizeof(struct kexec_fdset)
+				+ nr_fds * sizeof(struct kexec_file_fd);
+
+		ret = copy_from_user(fdset, ufdset, fdset_size);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		for (i = 0; i < fdset->nr_fds; i++) {
+			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
+				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
+						&image->dtb_buf, &size, INT_MAX,
+						READING_KEXEC_PARTIAL_DTB);
+				if (ret)
+					goto out;
+				image->dtb_buf_len = size;
+
+				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
+							       image->dtb_buf,
+							       image->dtb_buf_len);
+				if (ret)
+					goto out;
+			} else {
+				pr_debug("unknown file type %d failed.\n",
+						fdset->fds[i].type);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
 	if (cmdline_len) {
 		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
 		if (!image->cmdline_buf) {
@@ -202,7 +271,8 @@ out:
 static int
 kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 		       int initrd_fd, const char __user *cmdline_ptr,
-		       unsigned long cmdline_len, unsigned long flags)
+		       unsigned long cmdline_len, unsigned long flags,
+		       const struct kexec_fdset __user *ufdset)
 {
 	int ret;
 	struct kimage *image;
@@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	}
 
 	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
-					   cmdline_ptr, cmdline_len, flags);
+					   cmdline_ptr, cmdline_len, flags,
+					   ufdset);
 	if (ret)
 		goto out_free_image;
 
@@ -256,9 +327,9 @@ out_free_image:
 	return ret;
 }
 
-SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
+SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
-		unsigned long, flags)
+		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
 {
 	int ret = 0, i;
 	struct kimage **dest_image, *image;
@@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		kimage_free(xchg(&kexec_crash_image, NULL));
 
 	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
-				     cmdline_len, flags);
+				     cmdline_len, flags, ufdset);
 	if (ret)
 		goto out;
 
-- 
1.9.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Balbir Singh]

On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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.
> 
> 	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
> 				 unsigned long cmdline_len,
> 				 const char __user *cmdline_ptr,
> 				 unsigned long flags,
> 				 const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
> 	struct kexec_fdset {
> 		int nr_fds;
> 		struct kexec_file_fd fds[0];
> 	}
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/fs.h         |  1 +
>  include/linux/kexec.h      |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 ++++++++++++
>  kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\

The backspace is over-indented?

>  	id(POLICY, security-policy)		\
>  	id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -				 KEXEC_FILE_NO_INITRAMFS)
> +				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include <linux/types.h>
>  #include <linux/aio_abi.h>
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>  				    unsigned long cmdline_len,
>  				    const char __user *cmdline_ptr,
> -				    unsigned long flags);
> +				    unsigned long flags,
> +				    const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD	0x00000001
>  #define KEXEC_FILE_ON_CRASH	0x00000002
>  #define KEXEC_FILE_NO_INITRAMFS	0x00000004
> +#define KEXEC_FILE_EXTRA_FDS	0x00000008
> +
> +enum kexec_file_type {
> +	KEXEC_FILE_TYPE_KERNEL,
> +	KEXEC_FILE_TYPE_INITRAMFS,
> +
> +	/*
> +	 * Device Tree Blob containing just the nodes and properties that
> +	 * the kexec_file_load caller wants to add or modify.
> +	 */
> +	KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> +	enum kexec_file_type type;
> +	int fd;
> +};
> +
> +struct kexec_fdset {
> +	int nr_fds;
> +	struct kexec_file_fd fds[0];
> +};
>  
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 113af2f219b9..d6803dd884e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,9 @@
>  #include <linux/vmalloc.h>
>  #include "kexec_internal.h"
>  
> +#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
> +				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))
> +
>  /*
>   * Declare these symbols weak so that if architecture provides a purgatory,
>   * these will be overridden.
> @@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	image->image_loader_data = NULL;
>  }
>  
> +/**
> + * arch_kexec_verify_buffer() - check that the given kexec file is valid
> + *
> + * Device trees in particular can contain properties that may make the kernel
> + * execute code that it wasn't supposed to (e.g., use the wrong entry point
> + * when calling firmware functions). Because of this, the kernel needs to
> + * verify that it is safe to use the device tree blob passed from userspace.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size)
> +{
> +	return -EINVAL;
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> @@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  static int
>  kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			     const char __user *cmdline_ptr,
> -			     unsigned long cmdline_len, unsigned flags)
> +			     unsigned long cmdline_len, unsigned long flags,
> +			     const struct kexec_fdset __user *ufdset)
>  {
>  	int ret = 0;
>  	void *ldata;
> @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  		image->initrd_buf_len = size;
>  	}
>  
> +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> +		int nr_fds, i;
> +		size_t fdset_size;
> +		char fdset_buf[MAX_FDSET_SIZE];

Do we really want this on the stack?  I presume the size is not large

> +		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
> +
> +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		if (nr_fds > KEXEC_SEGMENT_MAX) {

We need an nr_fds < 0 check as well

> +			ret = -E2BIG;
> +			goto out;
> +		}
> +
> +		fdset_size = sizeof(struct kexec_fdset)
> +				+ nr_fds * sizeof(struct kexec_file_fd);
> +
> +		ret = copy_from_user(fdset, ufdset, fdset_size);

Can the user change nr_fds between the two copy_from_users, ideally not,
but we should validate it.

> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < fdset->nr_fds; i++) {
> +			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
> +				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
> +						&image->dtb_buf, &size, INT_MAX,
> +						READING_KEXEC_PARTIAL_DTB);
> +				if (ret)
> +					goto out;
> +				image->dtb_buf_len = size;
> +
> +				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
> +							       image->dtb_buf,
> +							       image->dtb_buf_len);
> +				if (ret)
> +					goto out;
> +			} else {
> +				pr_debug("unknown file type %d failed.\n",
> +						fdset->fds[i].type);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	if (cmdline_len) {
>  		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
>  		if (!image->cmdline_buf) {
> @@ -202,7 +271,8 @@ out:
>  static int
>  kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  		       int initrd_fd, const char __user *cmdline_ptr,
> -		       unsigned long cmdline_len, unsigned long flags)
> +		       unsigned long cmdline_len, unsigned long flags,
> +		       const struct kexec_fdset __user *ufdset)
>  {
>  	int ret;
>  	struct kimage *image;
> @@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  	}
>  
>  	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> -					   cmdline_ptr, cmdline_len, flags);
> +					   cmdline_ptr, cmdline_len, flags,
> +					   ufdset);
>  	if (ret)
>  		goto out_free_image;
>  
> @@ -256,9 +327,9 @@ out_free_image:
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> +SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
> -		unsigned long, flags)
> +		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
>  {
>  	int ret = 0, i;
>  	struct kimage **dest_image, *image;
> @@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  
>  	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
> -				     cmdline_len, flags);
> +				     cmdline_len, flags, ufdset);
>  	if (ret)
>  		goto out;
>


Balbir Singh.  

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Balbir Singh]

On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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.
> 
> 	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
> 				 unsigned long cmdline_len,
> 				 const char __user *cmdline_ptr,
> 				 unsigned long flags,
> 				 const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
> 	struct kexec_fdset {
> 		int nr_fds;
> 		struct kexec_file_fd fds[0];
> 	}
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/fs.h         |  1 +
>  include/linux/kexec.h      |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 ++++++++++++
>  kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\

The backspace is over-indented?

>  	id(POLICY, security-policy)		\
>  	id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -				 KEXEC_FILE_NO_INITRAMFS)
> +				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include <linux/types.h>
>  #include <linux/aio_abi.h>
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>  				    unsigned long cmdline_len,
>  				    const char __user *cmdline_ptr,
> -				    unsigned long flags);
> +				    unsigned long flags,
> +				    const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD	0x00000001
>  #define KEXEC_FILE_ON_CRASH	0x00000002
>  #define KEXEC_FILE_NO_INITRAMFS	0x00000004
> +#define KEXEC_FILE_EXTRA_FDS	0x00000008
> +
> +enum kexec_file_type {
> +	KEXEC_FILE_TYPE_KERNEL,
> +	KEXEC_FILE_TYPE_INITRAMFS,
> +
> +	/*
> +	 * Device Tree Blob containing just the nodes and properties that
> +	 * the kexec_file_load caller wants to add or modify.
> +	 */
> +	KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> +	enum kexec_file_type type;
> +	int fd;
> +};
> +
> +struct kexec_fdset {
> +	int nr_fds;
> +	struct kexec_file_fd fds[0];
> +};
>  
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 113af2f219b9..d6803dd884e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,9 @@
>  #include <linux/vmalloc.h>
>  #include "kexec_internal.h"
>  
> +#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
> +				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))
> +
>  /*
>   * Declare these symbols weak so that if architecture provides a purgatory,
>   * these will be overridden.
> @@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	image->image_loader_data = NULL;
>  }
>  
> +/**
> + * arch_kexec_verify_buffer() - check that the given kexec file is valid
> + *
> + * Device trees in particular can contain properties that may make the kernel
> + * execute code that it wasn't supposed to (e.g., use the wrong entry point
> + * when calling firmware functions). Because of this, the kernel needs to
> + * verify that it is safe to use the device tree blob passed from userspace.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size)
> +{
> +	return -EINVAL;
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> @@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  static int
>  kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			     const char __user *cmdline_ptr,
> -			     unsigned long cmdline_len, unsigned flags)
> +			     unsigned long cmdline_len, unsigned long flags,
> +			     const struct kexec_fdset __user *ufdset)
>  {
>  	int ret = 0;
>  	void *ldata;
> @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  		image->initrd_buf_len = size;
>  	}
>  
> +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> +		int nr_fds, i;
> +		size_t fdset_size;
> +		char fdset_buf[MAX_FDSET_SIZE];

Do we really want this on the stack?  I presume the size is not large

> +		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
> +
> +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		if (nr_fds > KEXEC_SEGMENT_MAX) {

We need an nr_fds < 0 check as well

> +			ret = -E2BIG;
> +			goto out;
> +		}
> +
> +		fdset_size = sizeof(struct kexec_fdset)
> +				+ nr_fds * sizeof(struct kexec_file_fd);
> +
> +		ret = copy_from_user(fdset, ufdset, fdset_size);

Can the user change nr_fds between the two copy_from_users, ideally not,
but we should validate it.

> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < fdset->nr_fds; i++) {
> +			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
> +				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
> +						&image->dtb_buf, &size, INT_MAX,
> +						READING_KEXEC_PARTIAL_DTB);
> +				if (ret)
> +					goto out;
> +				image->dtb_buf_len = size;
> +
> +				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
> +							       image->dtb_buf,
> +							       image->dtb_buf_len);
> +				if (ret)
> +					goto out;
> +			} else {
> +				pr_debug("unknown file type %d failed.\n",
> +						fdset->fds[i].type);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	if (cmdline_len) {
>  		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
>  		if (!image->cmdline_buf) {
> @@ -202,7 +271,8 @@ out:
>  static int
>  kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  		       int initrd_fd, const char __user *cmdline_ptr,
> -		       unsigned long cmdline_len, unsigned long flags)
> +		       unsigned long cmdline_len, unsigned long flags,
> +		       const struct kexec_fdset __user *ufdset)
>  {
>  	int ret;
>  	struct kimage *image;
> @@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  	}
>  
>  	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> -					   cmdline_ptr, cmdline_len, flags);
> +					   cmdline_ptr, cmdline_len, flags,
> +					   ufdset);
>  	if (ret)
>  		goto out_free_image;
>  
> @@ -256,9 +327,9 @@ out_free_image:
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> +SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
> -		unsigned long, flags)
> +		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
>  {
>  	int ret = 0, i;
>  	struct kimage **dest_image, *image;
> @@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  
>  	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
> -				     cmdline_len, flags);
> +				     cmdline_len, flags, ufdset);
>  	if (ret)
>  		goto out;
>


Balbir Singh.  

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

[Balbir Singh]

On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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.
> 
> 	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
> 				 unsigned long cmdline_len,
> 				 const char __user *cmdline_ptr,
> 				 unsigned long flags,
> 				 const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
> 	struct kexec_fdset {
> 		int nr_fds;
> 		struct kexec_file_fd fds[0];
> 	}
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/fs.h         |  1 +
>  include/linux/kexec.h      |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 ++++++++++++
>  kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\

The backspace is over-indented?

>  	id(POLICY, security-policy)		\
>  	id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -				 KEXEC_FILE_NO_INITRAMFS)
> +				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include <linux/types.h>
>  #include <linux/aio_abi.h>
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>  				    unsigned long cmdline_len,
>  				    const char __user *cmdline_ptr,
> -				    unsigned long flags);
> +				    unsigned long flags,
> +				    const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD	0x00000001
>  #define KEXEC_FILE_ON_CRASH	0x00000002
>  #define KEXEC_FILE_NO_INITRAMFS	0x00000004
> +#define KEXEC_FILE_EXTRA_FDS	0x00000008
> +
> +enum kexec_file_type {
> +	KEXEC_FILE_TYPE_KERNEL,
> +	KEXEC_FILE_TYPE_INITRAMFS,
> +
> +	/*
> +	 * Device Tree Blob containing just the nodes and properties that
> +	 * the kexec_file_load caller wants to add or modify.
> +	 */
> +	KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> +	enum kexec_file_type type;
> +	int fd;
> +};
> +
> +struct kexec_fdset {
> +	int nr_fds;
> +	struct kexec_file_fd fds[0];
> +};
>  
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 113af2f219b9..d6803dd884e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,9 @@
>  #include <linux/vmalloc.h>
>  #include "kexec_internal.h"
>  
> +#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
> +				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))
> +
>  /*
>   * Declare these symbols weak so that if architecture provides a purgatory,
>   * these will be overridden.
> @@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	image->image_loader_data = NULL;
>  }
>  
> +/**
> + * arch_kexec_verify_buffer() - check that the given kexec file is valid
> + *
> + * Device trees in particular can contain properties that may make the kernel
> + * execute code that it wasn't supposed to (e.g., use the wrong entry point
> + * when calling firmware functions). Because of this, the kernel needs to
> + * verify that it is safe to use the device tree blob passed from userspace.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size)
> +{
> +	return -EINVAL;
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> @@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  static int
>  kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			     const char __user *cmdline_ptr,
> -			     unsigned long cmdline_len, unsigned flags)
> +			     unsigned long cmdline_len, unsigned long flags,
> +			     const struct kexec_fdset __user *ufdset)
>  {
>  	int ret = 0;
>  	void *ldata;
> @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  		image->initrd_buf_len = size;
>  	}
>  
> +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> +		int nr_fds, i;
> +		size_t fdset_size;
> +		char fdset_buf[MAX_FDSET_SIZE];

Do we really want this on the stack?  I presume the size is not large

> +		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
> +
> +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		if (nr_fds > KEXEC_SEGMENT_MAX) {

We need an nr_fds < 0 check as well

> +			ret = -E2BIG;
> +			goto out;
> +		}
> +
> +		fdset_size = sizeof(struct kexec_fdset)
> +				+ nr_fds * sizeof(struct kexec_file_fd);
> +
> +		ret = copy_from_user(fdset, ufdset, fdset_size);

Can the user change nr_fds between the two copy_from_users, ideally not,
but we should validate it.

> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < fdset->nr_fds; i++) {
> +			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
> +				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
> +						&image->dtb_buf, &size, INT_MAX,
> +						READING_KEXEC_PARTIAL_DTB);
> +				if (ret)
> +					goto out;
> +				image->dtb_buf_len = size;
> +
> +				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
> +							       image->dtb_buf,
> +							       image->dtb_buf_len);
> +				if (ret)
> +					goto out;
> +			} else {
> +				pr_debug("unknown file type %d failed.\n",
> +						fdset->fds[i].type);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	if (cmdline_len) {
>  		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
>  		if (!image->cmdline_buf) {
> @@ -202,7 +271,8 @@ out:
>  static int
>  kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  		       int initrd_fd, const char __user *cmdline_ptr,
> -		       unsigned long cmdline_len, unsigned long flags)
> +		       unsigned long cmdline_len, unsigned long flags,
> +		       const struct kexec_fdset __user *ufdset)
>  {
>  	int ret;
>  	struct kimage *image;
> @@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  	}
>  
>  	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> -					   cmdline_ptr, cmdline_len, flags);
> +					   cmdline_ptr, cmdline_len, flags,
> +					   ufdset);
>  	if (ret)
>  		goto out_free_image;
>  
> @@ -256,9 +327,9 @@ out_free_image:
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> +SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
> -		unsigned long, flags)
> +		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
>  {
>  	int ret = 0, i;
>  	struct kimage **dest_image, *image;
> @@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  
>  	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
> -				     cmdline_len, flags);
> +				     cmdline_len, flags, ufdset);
>  	if (ret)
>  		goto out;
>


Balbir Singh.  

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Thiago Jung Bauermann]

Hello Balbir,

Thank you for the review!

Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh:
> On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3523bf62f328..847d9c31f428 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
> > 
> >  	id(MODULE, kernel-module)		\
> >  	id(KEXEC_IMAGE, kexec-image)		\
> >  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> > 
> > +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
> 
> The backspace is over-indented?

Indeed, I'll fix that. But to keep it aligned with the other backslashes, 
there would be no spaces between it and the final closing parenthesis. 
Either that, or reindent the other backslashes one more level. I think I 
prefer the former.
 
> > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image,
> > int kernel_fd, int initrd_fd,> 
> >  		image->initrd_buf_len = size;
> >  	
> >  	}
> > 
> > +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> > +		int nr_fds, i;
> > +		size_t fdset_size;
> > +		char fdset_buf[MAX_FDSET_SIZE];
> 
> Do we really want this on the stack?  I presume the size is not large

It has 132 bytes. Would it be better to use kmalloc instead?

> > +		struct kexec_fdset *fdset = (struct kexec_fdset *) 
fdset_buf;
> > +
> > +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> > +		if (ret) {
> > +			ret = -EFAULT;
> > +			goto out;
> > +		}
> > +
> > +		if (nr_fds > KEXEC_SEGMENT_MAX) {
> 
> We need an nr_fds < 0 check as well

Indeed, I forgot to do that. I will add the check.

> > +			ret = -E2BIG;
> > +			goto out;
> > +		}
> > +
> > +		fdset_size = sizeof(struct kexec_fdset)
> > +				+ nr_fds * sizeof(struct kexec_file_fd);
> > +
> > +		ret = copy_from_user(fdset, ufdset, fdset_size);
> 
> Can the user change nr_fds between the two copy_from_users, ideally not,
> but we should validate it.

Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error 
if they're different.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Hello Balbir,

Thank you for the review!

Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh:
> On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3523bf62f328..847d9c31f428 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
> > 
> >  	id(MODULE, kernel-module)		\
> >  	id(KEXEC_IMAGE, kexec-image)		\
> >  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> > 
> > +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
> 
> The backspace is over-indented?

Indeed, I'll fix that. But to keep it aligned with the other backslashes, 
there would be no spaces between it and the final closing parenthesis. 
Either that, or reindent the other backslashes one more level. I think I 
prefer the former.
 
> > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image,
> > int kernel_fd, int initrd_fd,> 
> >  		image->initrd_buf_len = size;
> >  	
> >  	}
> > 
> > +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> > +		int nr_fds, i;
> > +		size_t fdset_size;
> > +		char fdset_buf[MAX_FDSET_SIZE];
> 
> Do we really want this on the stack?  I presume the size is not large

It has 132 bytes. Would it be better to use kmalloc instead?

> > +		struct kexec_fdset *fdset = (struct kexec_fdset *) 
fdset_buf;
> > +
> > +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> > +		if (ret) {
> > +			ret = -EFAULT;
> > +			goto out;
> > +		}
> > +
> > +		if (nr_fds > KEXEC_SEGMENT_MAX) {
> 
> We need an nr_fds < 0 check as well

Indeed, I forgot to do that. I will add the check.

> > +			ret = -E2BIG;
> > +			goto out;
> > +		}
> > +
> > +		fdset_size = sizeof(struct kexec_fdset)
> > +				+ nr_fds * sizeof(struct kexec_file_fd);
> > +
> > +		ret = copy_from_user(fdset, ufdset, fdset_size);
> 
> Can the user change nr_fds between the two copy_from_users, ideally not,
> but we should validate it.

Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error 
if they're different.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

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

[Thiago Jung Bauermann]

Hello Balbir,

Thank you for the review!

Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh:
> On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3523bf62f328..847d9c31f428 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
> > 
> >  	id(MODULE, kernel-module)		\
> >  	id(KEXEC_IMAGE, kexec-image)		\
> >  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> > 
> > +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
> 
> The backspace is over-indented?

Indeed, I'll fix that. But to keep it aligned with the other backslashes, 
there would be no spaces between it and the final closing parenthesis. 
Either that, or reindent the other backslashes one more level. I think I 
prefer the former.
 
> > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image,
> > int kernel_fd, int initrd_fd,> 
> >  		image->initrd_buf_len = size;
> >  	
> >  	}
> > 
> > +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> > +		int nr_fds, i;
> > +		size_t fdset_size;
> > +		char fdset_buf[MAX_FDSET_SIZE];
> 
> Do we really want this on the stack?  I presume the size is not large

It has 132 bytes. Would it be better to use kmalloc instead?

> > +		struct kexec_fdset *fdset = (struct kexec_fdset *) 
fdset_buf;
> > +
> > +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> > +		if (ret) {
> > +			ret = -EFAULT;
> > +			goto out;
> > +		}
> > +
> > +		if (nr_fds > KEXEC_SEGMENT_MAX) {
> 
> We need an nr_fds < 0 check as well

Indeed, I forgot to do that. I will add the check.

> > +			ret = -E2BIG;
> > +			goto out;
> > +		}
> > +
> > +		fdset_size = sizeof(struct kexec_fdset)
> > +				+ nr_fds * sizeof(struct kexec_file_fd);
> > +
> > +		ret = copy_from_user(fdset, ufdset, fdset_size);
> 
> Can the user change nr_fds between the two copy_from_users, ideally not,
> but we should validate it.

Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error 
if they're different.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Here is a new version implementing your suggestions.
I also changed it to kmalloc fdset instead of using the stack.

What do you think?

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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.

	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
				 unsigned long cmdline_len,
				 const char __user *cmdline_ptr,
				 unsigned long flags,
				 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

	struct kexec_fdset {
		int nr_fds;
		struct kexec_file_fd fds[0];
	}

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  7 +++-
 include/linux/syscalls.h   |  4 +-
 include/uapi/linux/kexec.h | 22 +++++++++++
 kernel/kexec_file.c        | 92 +++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 117 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..2eb0674392d1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
+	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
 	id(POLICY, security-policy)		\
 	id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
 	kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
 	kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES           (4096)
 #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include <linux/types.h>
 #include <linux/aio_abi.h>
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 				    unsigned long cmdline_len,
 				    const char __user *cmdline_ptr,
-				    unsigned long flags);
+				    unsigned long flags,
+				    const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_EXTRA_FDS	0x00000008
+
+enum kexec_file_type {
+	KEXEC_FILE_TYPE_KERNEL,
+	KEXEC_FILE_TYPE_INITRAMFS,
+
+	/*
+	 * Device Tree Blob containing just the nodes and properties that
+	 * the kexec_file_load caller wants to add or modify.
+	 */
+	KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+	enum kexec_file_type type;
+	int fd;
+};
+
+struct kexec_fdset {
+	int nr_fds;
+	struct kexec_file_fd fds[0];
+};
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 113af2f219b9..302427e5ee71 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -116,6 +116,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * arch_kexec_verify_buffer() - check that the given kexec file is valid
+ *
+ * Device trees in particular can contain properties that may make the kernel
+ * execute code that it wasn't supposed to (e.g., use the wrong entry point
+ * when calling firmware functions). Because of this, the kernel needs to
+ * verify that it is safe to use the device tree blob passed from userspace.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size)
+{
+	return -EINVAL;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -123,11 +139,13 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 static int
 kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			     const char __user *cmdline_ptr,
-			     unsigned long cmdline_len, unsigned flags)
+			     unsigned long cmdline_len, unsigned long flags,
+			     const struct kexec_fdset __user *ufdset)
 {
 	int ret = 0;
 	void *ldata;
 	loff_t size;
+	struct kexec_fdset *fdset = NULL;
 
 	ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
 				       &size, INT_MAX, READING_KEXEC_IMAGE);
@@ -160,6 +178,64 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		image->initrd_buf_len = size;
 	}
 
+	if (flags & KEXEC_FILE_EXTRA_FDS) {
+		int nr_fds, i;
+		size_t fdset_size;
+
+		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds < 0 || nr_fds > KEXEC_SEGMENT_MAX) {
+			ret = -E2BIG;
+			goto out;
+		}
+
+		fdset_size = sizeof(struct kexec_fdset)
+				+ nr_fds * sizeof(struct kexec_file_fd);
+
+		fdset = kmalloc(fdset_size, GFP_KERNEL);
+		if (fdset == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = copy_from_user(fdset, ufdset, fdset_size);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds != fdset->nr_fds) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		for (i = 0; i < fdset->nr_fds; i++) {
+			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
+				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
+						&image->dtb_buf, &size, INT_MAX,
+						READING_KEXEC_PARTIAL_DTB);
+				if (ret)
+					goto out;
+				image->dtb_buf_len = size;
+
+				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
+							       image->dtb_buf,
+							       image->dtb_buf_len);
+				if (ret)
+					goto out;
+			} else {
+				pr_debug("unknown file type %d failed.\n",
+						fdset->fds[i].type);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
 	if (cmdline_len) {
 		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
 		if (!image->cmdline_buf) {
@@ -193,6 +269,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 	image->image_loader_data = ldata;
 out:
+	kfree(fdset);
+
 	/* In case of error, free up all allocated memory in this function */
 	if (ret)
 		kimage_file_post_load_cleanup(image);
@@ -202,7 +280,8 @@ out:
 static int
 kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 		       int initrd_fd, const char __user *cmdline_ptr,
-		       unsigned long cmdline_len, unsigned long flags)
+		       unsigned long cmdline_len, unsigned long flags,
+		       const struct kexec_fdset __user *ufdset)
 {
 	int ret;
 	struct kimage *image;
@@ -221,7 +300,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	}
 
 	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
-					   cmdline_ptr, cmdline_len, flags);
+					   cmdline_ptr, cmdline_len, flags,
+					   ufdset);
 	if (ret)
 		goto out_free_image;
 
@@ -256,9 +336,9 @@ out_free_image:
 	return ret;
 }
 
-SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
+SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
-		unsigned long, flags)
+		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
 {
 	int ret = 0, i;
 	struct kimage **dest_image, *image;
@@ -295,7 +375,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		kimage_free(xchg(&kexec_crash_image, NULL));
 
 	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
-				     cmdline_len, flags);
+				     cmdline_len, flags, ufdset);
 	if (ret)
 		goto out;
 
-- 
1.9.1

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Here is a new version implementing your suggestions.
I also changed it to kmalloc fdset instead of using the stack.

What do you think?

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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.

	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
				 unsigned long cmdline_len,
				 const char __user *cmdline_ptr,
				 unsigned long flags,
				 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

	struct kexec_fdset {
		int nr_fds;
		struct kexec_file_fd fds[0];
	}

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  7 +++-
 include/linux/syscalls.h   |  4 +-
 include/uapi/linux/kexec.h | 22 +++++++++++
 kernel/kexec_file.c        | 92 +++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 117 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..2eb0674392d1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
+	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
 	id(POLICY, security-policy)		\
 	id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
 	kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
 	kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES           (4096)
 #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include <linux/types.h>
 #include <linux/aio_abi.h>
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 				    unsigned long cmdline_len,
 				    const char __user *cmdline_ptr,
-				    unsigned long flags);
+				    unsigned long flags,
+				    const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_EXTRA_FDS	0x00000008
+
+enum kexec_file_type {
+	KEXEC_FILE_TYPE_KERNEL,
+	KEXEC_FILE_TYPE_INITRAMFS,
+
+	/*
+	 * Device Tree Blob containing just the nodes and properties that
+	 * the kexec_file_load caller wants to add or modify.
+	 */
+	KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+	enum kexec_file_type type;
+	int fd;
+};
+
+struct kexec_fdset {
+	int nr_fds;
+	struct kexec_file_fd fds[0];
+};
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 113af2f219b9..302427e5ee71 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -116,6 +116,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * arch_kexec_verify_buffer() - check that the given kexec file is valid
+ *
+ * Device trees in particular can contain properties that may make the kernel
+ * execute code that it wasn't supposed to (e.g., use the wrong entry point
+ * when calling firmware functions). Because of this, the kernel needs to
+ * verify that it is safe to use the device tree blob passed from userspace.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size)
+{
+	return -EINVAL;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -123,11 +139,13 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 static int
 kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			     const char __user *cmdline_ptr,
-			     unsigned long cmdline_len, unsigned flags)
+			     unsigned long cmdline_len, unsigned long flags,
+			     const struct kexec_fdset __user *ufdset)
 {
 	int ret = 0;
 	void *ldata;
 	loff_t size;
+	struct kexec_fdset *fdset = NULL;
 
 	ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
 				       &size, INT_MAX, READING_KEXEC_IMAGE);
@@ -160,6 +178,64 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		image->initrd_buf_len = size;
 	}
 
+	if (flags & KEXEC_FILE_EXTRA_FDS) {
+		int nr_fds, i;
+		size_t fdset_size;
+
+		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds < 0 || nr_fds > KEXEC_SEGMENT_MAX) {
+			ret = -E2BIG;
+			goto out;
+		}
+
+		fdset_size = sizeof(struct kexec_fdset)
+				+ nr_fds * sizeof(struct kexec_file_fd);
+
+		fdset = kmalloc(fdset_size, GFP_KERNEL);
+		if (fdset == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = copy_from_user(fdset, ufdset, fdset_size);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds != fdset->nr_fds) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		for (i = 0; i < fdset->nr_fds; i++) {
+			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
+				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
+						&image->dtb_buf, &size, INT_MAX,
+						READING_KEXEC_PARTIAL_DTB);
+				if (ret)
+					goto out;
+				image->dtb_buf_len = size;
+
+				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
+							       image->dtb_buf,
+							       image->dtb_buf_len);
+				if (ret)
+					goto out;
+			} else {
+				pr_debug("unknown file type %d failed.\n",
+						fdset->fds[i].type);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
 	if (cmdline_len) {
 		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
 		if (!image->cmdline_buf) {
@@ -193,6 +269,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 	image->image_loader_data = ldata;
 out:
+	kfree(fdset);
+
 	/* In case of error, free up all allocated memory in this function */
 	if (ret)
 		kimage_file_post_load_cleanup(image);
@@ -202,7 +280,8 @@ out:
 static int
 kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 		       int initrd_fd, const char __user *cmdline_ptr,
-		       unsigned long cmdline_len, unsigned long flags)
+		       unsigned long cmdline_len, unsigned long flags,
+		       const struct kexec_fdset __user *ufdset)
 {
 	int ret;
 	struct kimage *image;
@@ -221,7 +300,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	}
 
 	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
-					   cmdline_ptr, cmdline_len, flags);
+					   cmdline_ptr, cmdline_len, flags,
+					   ufdset);
 	if (ret)
 		goto out_free_image;
 
@@ -256,9 +336,9 @@ out_free_image:
 	return ret;
 }
 
-SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
+SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
-		unsigned long, flags)
+		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
 {
 	int ret = 0, i;
 	struct kimage **dest_image, *image;
@@ -295,7 +375,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		kimage_free(xchg(&kexec_crash_image, NULL));
 
 	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
-				     cmdline_len, flags);
+				     cmdline_len, flags, ufdset);
 	if (ret)
 		goto out;
 
-- 
1.9.1

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Here is a new version implementing your suggestions.
I also changed it to kmalloc fdset instead of using the stack.

What do you think?

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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.

	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
				 unsigned long cmdline_len,
				 const char __user *cmdline_ptr,
				 unsigned long flags,
				 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

	struct kexec_fdset {
		int nr_fds;
		struct kexec_file_fd fds[0];
	}

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  7 +++-
 include/linux/syscalls.h   |  4 +-
 include/uapi/linux/kexec.h | 22 +++++++++++
 kernel/kexec_file.c        | 92 +++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 117 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..2eb0674392d1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
+	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
 	id(POLICY, security-policy)		\
 	id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
 	kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
 	kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES           (4096)
 #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include <linux/types.h>
 #include <linux/aio_abi.h>
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 				    unsigned long cmdline_len,
 				    const char __user *cmdline_ptr,
-				    unsigned long flags);
+				    unsigned long flags,
+				    const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_EXTRA_FDS	0x00000008
+
+enum kexec_file_type {
+	KEXEC_FILE_TYPE_KERNEL,
+	KEXEC_FILE_TYPE_INITRAMFS,
+
+	/*
+	 * Device Tree Blob containing just the nodes and properties that
+	 * the kexec_file_load caller wants to add or modify.
+	 */
+	KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+	enum kexec_file_type type;
+	int fd;
+};
+
+struct kexec_fdset {
+	int nr_fds;
+	struct kexec_file_fd fds[0];
+};
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 113af2f219b9..302427e5ee71 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -116,6 +116,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * arch_kexec_verify_buffer() - check that the given kexec file is valid
+ *
+ * Device trees in particular can contain properties that may make the kernel
+ * execute code that it wasn't supposed to (e.g., use the wrong entry point
+ * when calling firmware functions). Because of this, the kernel needs to
+ * verify that it is safe to use the device tree blob passed from userspace.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+				    unsigned long size)
+{
+	return -EINVAL;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -123,11 +139,13 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 static int
 kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			     const char __user *cmdline_ptr,
-			     unsigned long cmdline_len, unsigned flags)
+			     unsigned long cmdline_len, unsigned long flags,
+			     const struct kexec_fdset __user *ufdset)
 {
 	int ret = 0;
 	void *ldata;
 	loff_t size;
+	struct kexec_fdset *fdset = NULL;
 
 	ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
 				       &size, INT_MAX, READING_KEXEC_IMAGE);
@@ -160,6 +178,64 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		image->initrd_buf_len = size;
 	}
 
+	if (flags & KEXEC_FILE_EXTRA_FDS) {
+		int nr_fds, i;
+		size_t fdset_size;
+
+		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds < 0 || nr_fds > KEXEC_SEGMENT_MAX) {
+			ret = -E2BIG;
+			goto out;
+		}
+
+		fdset_size = sizeof(struct kexec_fdset)
+				+ nr_fds * sizeof(struct kexec_file_fd);
+
+		fdset = kmalloc(fdset_size, GFP_KERNEL);
+		if (fdset == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = copy_from_user(fdset, ufdset, fdset_size);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (nr_fds != fdset->nr_fds) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		for (i = 0; i < fdset->nr_fds; i++) {
+			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
+				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
+						&image->dtb_buf, &size, INT_MAX,
+						READING_KEXEC_PARTIAL_DTB);
+				if (ret)
+					goto out;
+				image->dtb_buf_len = size;
+
+				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
+							       image->dtb_buf,
+							       image->dtb_buf_len);
+				if (ret)
+					goto out;
+			} else {
+				pr_debug("unknown file type %d failed.\n",
+						fdset->fds[i].type);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
 	if (cmdline_len) {
 		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
 		if (!image->cmdline_buf) {
@@ -193,6 +269,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 	image->image_loader_data = ldata;
 out:
+	kfree(fdset);
+
 	/* In case of error, free up all allocated memory in this function */
 	if (ret)
 		kimage_file_post_load_cleanup(image);
@@ -202,7 +280,8 @@ out:
 static int
 kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 		       int initrd_fd, const char __user *cmdline_ptr,
-		       unsigned long cmdline_len, unsigned long flags)
+		       unsigned long cmdline_len, unsigned long flags,
+		       const struct kexec_fdset __user *ufdset)
 {
 	int ret;
 	struct kimage *image;
@@ -221,7 +300,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	}
 
 	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
-					   cmdline_ptr, cmdline_len, flags);
+					   cmdline_ptr, cmdline_len, flags,
+					   ufdset);
 	if (ret)
 		goto out_free_image;
 
@@ -256,9 +336,9 @@ out_free_image:
 	return ret;
 }
 
-SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
+SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
-		unsigned long, flags)
+		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
 {
 	int ret = 0, i;
 	struct kimage **dest_image, *image;
@@ -295,7 +375,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		kimage_free(xchg(&kexec_crash_image, NULL));
 
 	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
-				     cmdline_len, flags);
+				     cmdline_len, flags, ufdset);
 	if (ret)
 		goto out;
 
-- 
1.9.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Dave Young]

Since Eric was objecting the extension, I think you should convince him,
but I will review from code point of view.

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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.
> 
> 	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
> 				 unsigned long cmdline_len,
> 				 const char __user *cmdline_ptr,
> 				 unsigned long flags,
> 				 const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
> 	struct kexec_fdset {
> 		int nr_fds;
> 		struct kexec_file_fd fds[0];
> 	}
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/fs.h         |  1 +
>  include/linux/kexec.h      |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 ++++++++++++
>  kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
>  	id(POLICY, security-policy)		\
>  	id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -				 KEXEC_FILE_NO_INITRAMFS)
> +				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include <linux/types.h>
>  #include <linux/aio_abi.h>
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>  				    unsigned long cmdline_len,
>  				    const char __user *cmdline_ptr,
> -				    unsigned long flags);
> +				    unsigned long flags,
> +				    const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD	0x00000001
>  #define KEXEC_FILE_ON_CRASH	0x00000002
>  #define KEXEC_FILE_NO_INITRAMFS	0x00000004
> +#define KEXEC_FILE_EXTRA_FDS	0x00000008
> +
> +enum kexec_file_type {
> +	KEXEC_FILE_TYPE_KERNEL,
> +	KEXEC_FILE_TYPE_INITRAMFS,
> +
> +	/*
> +	 * Device Tree Blob containing just the nodes and properties that
> +	 * the kexec_file_load caller wants to add or modify.
> +	 */
> +	KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> +	enum kexec_file_type type;
> +	int fd;
> +};
> +
> +struct kexec_fdset {
> +	int nr_fds;
> +	struct kexec_file_fd fds[0];
> +};
>  
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 113af2f219b9..d6803dd884e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,9 @@
>  #include <linux/vmalloc.h>
>  #include "kexec_internal.h"
>  
> +#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
> +				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))

How about use KEXEC_EXTRA_FD_MAX = 1, so only allow passing one fd for
now. In the future if there's other requirement we can extend the
internal value.

> +
>  /*
>   * Declare these symbols weak so that if architecture provides a purgatory,
>   * these will be overridden.
> @@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	image->image_loader_data = NULL;
>  }
>  
> +/**
> + * arch_kexec_verify_buffer() - check that the given kexec file is valid
> + *
> + * Device trees in particular can contain properties that may make the kernel
> + * execute code that it wasn't supposed to (e.g., use the wrong entry point
> + * when calling firmware functions). Because of this, the kernel needs to
> + * verify that it is safe to use the device tree blob passed from userspace.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size)
> +{
> +	return -EINVAL;
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> @@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  static int
>  kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			     const char __user *cmdline_ptr,
> -			     unsigned long cmdline_len, unsigned flags)
> +			     unsigned long cmdline_len, unsigned long flags,
> +			     const struct kexec_fdset __user *ufdset)
>  {
>  	int ret = 0;
>  	void *ldata;
> @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  		image->initrd_buf_len = size;
>  	}
>  
> +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> +		int nr_fds, i;
> +		size_t fdset_size;
> +		char fdset_buf[MAX_FDSET_SIZE];
> +		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
> +
> +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		if (nr_fds > KEXEC_SEGMENT_MAX) {
> +			ret = -E2BIG;
> +			goto out;
> +		}
> +
> +		fdset_size = sizeof(struct kexec_fdset)
> +				+ nr_fds * sizeof(struct kexec_file_fd);
> +
> +		ret = copy_from_user(fdset, ufdset, fdset_size);
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < fdset->nr_fds; i++) {
> +			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
> +				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
> +						&image->dtb_buf, &size, INT_MAX,
> +						READING_KEXEC_PARTIAL_DTB);
> +				if (ret)
> +					goto out;
> +				image->dtb_buf_len = size;
> +
> +				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
> +							       image->dtb_buf,
> +							       image->dtb_buf_len);
> +				if (ret)
> +					goto out;
> +			} else {
> +				pr_debug("unknown file type %d failed.\n",
> +						fdset->fds[i].type);

Should be pr_err

> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	if (cmdline_len) {
>  		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
>  		if (!image->cmdline_buf) {
> @@ -202,7 +271,8 @@ out:
>  static int
>  kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  		       int initrd_fd, const char __user *cmdline_ptr,
> -		       unsigned long cmdline_len, unsigned long flags)
> +		       unsigned long cmdline_len, unsigned long flags,
> +		       const struct kexec_fdset __user *ufdset)
>  {
>  	int ret;
>  	struct kimage *image;
> @@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  	}
>  
>  	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> -					   cmdline_ptr, cmdline_len, flags);
> +					   cmdline_ptr, cmdline_len, flags,
> +					   ufdset);
>  	if (ret)
>  		goto out_free_image;
>  
> @@ -256,9 +327,9 @@ out_free_image:
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> +SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
> -		unsigned long, flags)
> +		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
>  {
>  	int ret = 0, i;
>  	struct kimage **dest_image, *image;
> @@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  
>  	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
> -				     cmdline_len, flags);
> +				     cmdline_len, flags, ufdset);
>  	if (ret)
>  		goto out;
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Dave Young]

Since Eric was objecting the extension, I think you should convince him,
but I will review from code point of view.

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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.
> 
> 	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
> 				 unsigned long cmdline_len,
> 				 const char __user *cmdline_ptr,
> 				 unsigned long flags,
> 				 const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
> 	struct kexec_fdset {
> 		int nr_fds;
> 		struct kexec_file_fd fds[0];
> 	}
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/fs.h         |  1 +
>  include/linux/kexec.h      |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 ++++++++++++
>  kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
>  	id(POLICY, security-policy)		\
>  	id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -				 KEXEC_FILE_NO_INITRAMFS)
> +				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include <linux/types.h>
>  #include <linux/aio_abi.h>
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>  				    unsigned long cmdline_len,
>  				    const char __user *cmdline_ptr,
> -				    unsigned long flags);
> +				    unsigned long flags,
> +				    const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD	0x00000001
>  #define KEXEC_FILE_ON_CRASH	0x00000002
>  #define KEXEC_FILE_NO_INITRAMFS	0x00000004
> +#define KEXEC_FILE_EXTRA_FDS	0x00000008
> +
> +enum kexec_file_type {
> +	KEXEC_FILE_TYPE_KERNEL,
> +	KEXEC_FILE_TYPE_INITRAMFS,
> +
> +	/*
> +	 * Device Tree Blob containing just the nodes and properties that
> +	 * the kexec_file_load caller wants to add or modify.
> +	 */
> +	KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> +	enum kexec_file_type type;
> +	int fd;
> +};
> +
> +struct kexec_fdset {
> +	int nr_fds;
> +	struct kexec_file_fd fds[0];
> +};
>  
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 113af2f219b9..d6803dd884e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,9 @@
>  #include <linux/vmalloc.h>
>  #include "kexec_internal.h"
>  
> +#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
> +				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))

How about use KEXEC_EXTRA_FD_MAX = 1, so only allow passing one fd for
now. In the future if there's other requirement we can extend the
internal value.

> +
>  /*
>   * Declare these symbols weak so that if architecture provides a purgatory,
>   * these will be overridden.
> @@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	image->image_loader_data = NULL;
>  }
>  
> +/**
> + * arch_kexec_verify_buffer() - check that the given kexec file is valid
> + *
> + * Device trees in particular can contain properties that may make the kernel
> + * execute code that it wasn't supposed to (e.g., use the wrong entry point
> + * when calling firmware functions). Because of this, the kernel needs to
> + * verify that it is safe to use the device tree blob passed from userspace.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size)
> +{
> +	return -EINVAL;
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> @@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  static int
>  kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			     const char __user *cmdline_ptr,
> -			     unsigned long cmdline_len, unsigned flags)
> +			     unsigned long cmdline_len, unsigned long flags,
> +			     const struct kexec_fdset __user *ufdset)
>  {
>  	int ret = 0;
>  	void *ldata;
> @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  		image->initrd_buf_len = size;
>  	}
>  
> +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> +		int nr_fds, i;
> +		size_t fdset_size;
> +		char fdset_buf[MAX_FDSET_SIZE];
> +		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
> +
> +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		if (nr_fds > KEXEC_SEGMENT_MAX) {
> +			ret = -E2BIG;
> +			goto out;
> +		}
> +
> +		fdset_size = sizeof(struct kexec_fdset)
> +				+ nr_fds * sizeof(struct kexec_file_fd);
> +
> +		ret = copy_from_user(fdset, ufdset, fdset_size);
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < fdset->nr_fds; i++) {
> +			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
> +				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
> +						&image->dtb_buf, &size, INT_MAX,
> +						READING_KEXEC_PARTIAL_DTB);
> +				if (ret)
> +					goto out;
> +				image->dtb_buf_len = size;
> +
> +				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
> +							       image->dtb_buf,
> +							       image->dtb_buf_len);
> +				if (ret)
> +					goto out;
> +			} else {
> +				pr_debug("unknown file type %d failed.\n",
> +						fdset->fds[i].type);

Should be pr_err

> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	if (cmdline_len) {
>  		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
>  		if (!image->cmdline_buf) {
> @@ -202,7 +271,8 @@ out:
>  static int
>  kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  		       int initrd_fd, const char __user *cmdline_ptr,
> -		       unsigned long cmdline_len, unsigned long flags)
> +		       unsigned long cmdline_len, unsigned long flags,
> +		       const struct kexec_fdset __user *ufdset)
>  {
>  	int ret;
>  	struct kimage *image;
> @@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  	}
>  
>  	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> -					   cmdline_ptr, cmdline_len, flags);
> +					   cmdline_ptr, cmdline_len, flags,
> +					   ufdset);
>  	if (ret)
>  		goto out_free_image;
>  
> @@ -256,9 +327,9 @@ out_free_image:
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> +SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
> -		unsigned long, flags)
> +		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
>  {
>  	int ret = 0, i;
>  	struct kimage **dest_image, *image;
> @@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  
>  	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
> -				     cmdline_len, flags);
> +				     cmdline_len, flags, ufdset);
>  	if (ret)
>  		goto out;
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

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

[Dave Young]

Since Eric was objecting the extension, I think you should convince him,
but I will review from code point of view.

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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.
> 
> 	long sys_kexec_file_load(int kernel_fd, int initrd_fd,
> 				 unsigned long cmdline_len,
> 				 const char __user *cmdline_ptr,
> 				 unsigned long flags,
> 				 const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
> 	struct kexec_fdset {
> 		int nr_fds;
> 		struct kexec_file_fd fds[0];
> 	}
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/fs.h         |  1 +
>  include/linux/kexec.h      |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 ++++++++++++
>  kernel/kexec_file.c        | 83 ++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> +	id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)		\
>  	id(POLICY, security-policy)		\
>  	id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -				 KEXEC_FILE_NO_INITRAMFS)
> +				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include <linux/types.h>
>  #include <linux/aio_abi.h>
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>  				    unsigned long cmdline_len,
>  				    const char __user *cmdline_ptr,
> -				    unsigned long flags);
> +				    unsigned long flags,
> +				    const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD	0x00000001
>  #define KEXEC_FILE_ON_CRASH	0x00000002
>  #define KEXEC_FILE_NO_INITRAMFS	0x00000004
> +#define KEXEC_FILE_EXTRA_FDS	0x00000008
> +
> +enum kexec_file_type {
> +	KEXEC_FILE_TYPE_KERNEL,
> +	KEXEC_FILE_TYPE_INITRAMFS,
> +
> +	/*
> +	 * Device Tree Blob containing just the nodes and properties that
> +	 * the kexec_file_load caller wants to add or modify.
> +	 */
> +	KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> +	enum kexec_file_type type;
> +	int fd;
> +};
> +
> +struct kexec_fdset {
> +	int nr_fds;
> +	struct kexec_file_fd fds[0];
> +};
>  
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 113af2f219b9..d6803dd884e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,9 @@
>  #include <linux/vmalloc.h>
>  #include "kexec_internal.h"
>  
> +#define MAX_FDSET_SIZE	(sizeof(struct kexec_fdset) + \
> +				KEXEC_SEGMENT_MAX * sizeof(struct kexec_file_fd))

How about use KEXEC_EXTRA_FD_MAX = 1, so only allow passing one fd for
now. In the future if there's other requirement we can extend the
internal value.

> +
>  /*
>   * Declare these symbols weak so that if architecture provides a purgatory,
>   * these will be overridden.
> @@ -116,6 +119,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	image->image_loader_data = NULL;
>  }
>  
> +/**
> + * arch_kexec_verify_buffer() - check that the given kexec file is valid
> + *
> + * Device trees in particular can contain properties that may make the kernel
> + * execute code that it wasn't supposed to (e.g., use the wrong entry point
> + * when calling firmware functions). Because of this, the kernel needs to
> + * verify that it is safe to use the device tree blob passed from userspace.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
> +				    unsigned long size)
> +{
> +	return -EINVAL;
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> @@ -123,7 +142,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  static int
>  kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			     const char __user *cmdline_ptr,
> -			     unsigned long cmdline_len, unsigned flags)
> +			     unsigned long cmdline_len, unsigned long flags,
> +			     const struct kexec_fdset __user *ufdset)
>  {
>  	int ret = 0;
>  	void *ldata;
> @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  		image->initrd_buf_len = size;
>  	}
>  
> +	if (flags & KEXEC_FILE_EXTRA_FDS) {
> +		int nr_fds, i;
> +		size_t fdset_size;
> +		char fdset_buf[MAX_FDSET_SIZE];
> +		struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf;
> +
> +		ret = copy_from_user(&nr_fds, ufdset, sizeof(int));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		if (nr_fds > KEXEC_SEGMENT_MAX) {
> +			ret = -E2BIG;
> +			goto out;
> +		}
> +
> +		fdset_size = sizeof(struct kexec_fdset)
> +				+ nr_fds * sizeof(struct kexec_file_fd);
> +
> +		ret = copy_from_user(fdset, ufdset, fdset_size);
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < fdset->nr_fds; i++) {
> +			if (fdset->fds[i].type == KEXEC_FILE_TYPE_PARTIAL_DTB) {
> +				ret = kernel_read_file_from_fd(fdset->fds[i].fd,
> +						&image->dtb_buf, &size, INT_MAX,
> +						READING_KEXEC_PARTIAL_DTB);
> +				if (ret)
> +					goto out;
> +				image->dtb_buf_len = size;
> +
> +				ret = arch_kexec_verify_buffer(KEXEC_FILE_TYPE_PARTIAL_DTB,
> +							       image->dtb_buf,
> +							       image->dtb_buf_len);
> +				if (ret)
> +					goto out;
> +			} else {
> +				pr_debug("unknown file type %d failed.\n",
> +						fdset->fds[i].type);

Should be pr_err

> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	if (cmdline_len) {
>  		image->cmdline_buf = kzalloc(cmdline_len, GFP_KERNEL);
>  		if (!image->cmdline_buf) {
> @@ -202,7 +271,8 @@ out:
>  static int
>  kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  		       int initrd_fd, const char __user *cmdline_ptr,
> -		       unsigned long cmdline_len, unsigned long flags)
> +		       unsigned long cmdline_len, unsigned long flags,
> +		       const struct kexec_fdset __user *ufdset)
>  {
>  	int ret;
>  	struct kimage *image;
> @@ -221,7 +291,8 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  	}
>  
>  	ret = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> -					   cmdline_ptr, cmdline_len, flags);
> +					   cmdline_ptr, cmdline_len, flags,
> +					   ufdset);
>  	if (ret)
>  		goto out_free_image;
>  
> @@ -256,9 +327,9 @@ out_free_image:
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> +SYSCALL_DEFINE6(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		unsigned long, cmdline_len, const char __user *, cmdline_ptr,
> -		unsigned long, flags)
> +		unsigned long, flags, const struct kexec_fdset __user *, ufdset)
>  {
>  	int ret = 0, i;
>  	struct kimage **dest_image, *image;
> @@ -295,7 +366,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  
>  	ret = kimage_file_alloc_init(&image, kernel_fd, initrd_fd, cmdline_ptr,
> -				     cmdline_len, flags);
> +				     cmdline_len, flags, ufdset);
>  	if (ret)
>  		goto out;
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 1/2] kexec: add dtb info to struct kimage

[Dave Young]

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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 adds dtb buffer information to struct kimage.
> When users don't specify dtb explicitly and the one used for the current
> kernel can be re-used, this change will be good enough for implementing
> kexec_file_load feature.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/linux/kexec.h | 3 +++
>  kernel/kexec_file.c   | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index d7437777baaa..4f85d284ed0b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -192,6 +192,9 @@ struct kimage {
>  	char *cmdline_buf;
>  	unsigned long cmdline_buf_len;
>  
> +	void *dtb_buf;
> +	unsigned long dtb_buf_len;
> +
>  	/* File operations provided by image loader */
>  	struct kexec_file_ops *fops;
>  
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 503bc2d348e5..113af2f219b9 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -92,6 +92,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	vfree(image->initrd_buf);
>  	image->initrd_buf = NULL;
>  
> +	vfree(image->dtb_buf);
> +	image->dtb_buf = NULL;
> +
>  	kfree(image->cmdline_buf);
>  	image->cmdline_buf = NULL;
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

[PATCH v2 1/2] kexec: add dtb info to struct kimage

[Dave Young]

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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 adds dtb buffer information to struct kimage.
> When users don't specify dtb explicitly and the one used for the current
> kernel can be re-used, this change will be good enough for implementing
> kexec_file_load feature.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/linux/kexec.h | 3 +++
>  kernel/kexec_file.c   | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index d7437777baaa..4f85d284ed0b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -192,6 +192,9 @@ struct kimage {
>  	char *cmdline_buf;
>  	unsigned long cmdline_buf_len;
>  
> +	void *dtb_buf;
> +	unsigned long dtb_buf_len;
> +
>  	/* File operations provided by image loader */
>  	struct kexec_file_ops *fops;
>  
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 503bc2d348e5..113af2f219b9 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -92,6 +92,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	vfree(image->initrd_buf);
>  	image->initrd_buf = NULL;
>  
> +	vfree(image->dtb_buf);
> +	image->dtb_buf = NULL;
> +
>  	kfree(image->cmdline_buf);
>  	image->cmdline_buf = NULL;
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

Re: [PATCH v2 1/2] kexec: add dtb info to struct kimage

[Dave Young]

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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 adds dtb buffer information to struct kimage.
> When users don't specify dtb explicitly and the one used for the current
> kernel can be re-used, this change will be good enough for implementing
> kexec_file_load feature.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/linux/kexec.h | 3 +++
>  kernel/kexec_file.c   | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index d7437777baaa..4f85d284ed0b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -192,6 +192,9 @@ struct kimage {
>  	char *cmdline_buf;
>  	unsigned long cmdline_buf_len;
>  
> +	void *dtb_buf;
> +	unsigned long dtb_buf_len;
> +
>  	/* File operations provided by image loader */
>  	struct kexec_file_ops *fops;
>  
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 503bc2d348e5..113af2f219b9 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -92,6 +92,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
>  	vfree(image->initrd_buf);
>  	image->initrd_buf = NULL;
>  
> +	vfree(image->dtb_buf);
> +	image->dtb_buf = NULL;
> +
>  	kfree(image->cmdline_buf);
>  	image->cmdline_buf = NULL;
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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.

[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.

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.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Thiago Jung Bauermann]

Hello Mark,

Sorry for taking this long to respond. I've been focusing on getting my 
kexec_file_load and kexec buffer hand-over series in shape.

Am Donnerstag, 18 August 2016, 11:21:13 schrieb Mark Rutland:
> On Thu, Aug 11, 2016 at 08:03:56PM -0300, Thiago Jung Bauermann wrote:
> > 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.

Thanks for laying out out the reasons for your objection. That's very 
helpful.

> * 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.

Yes, this is the case I am aiming for. I'll experiment with a couple of 
different approaches and see how well they perform.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH v2 0/2] extend kexec_file_load system call

[Thiago Jung Bauermann]

Hello Mark,

Sorry for taking this long to respond. I've been focusing on getting my 
kexec_file_load and kexec buffer hand-over series in shape.

Am Donnerstag, 18 August 2016, 11:21:13 schrieb Mark Rutland:
> On Thu, Aug 11, 2016 at 08:03:56PM -0300, Thiago Jung Bauermann wrote:
> > 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.

Thanks for laying out out the reasons for your objection. That's very 
helpful.

> * 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.

Yes, this is the case I am aiming for. I'll experiment with a couple of 
different approaches and see how well they perform.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

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

[Thiago Jung Bauermann]

Hello Mark,

Sorry for taking this long to respond. I've been focusing on getting my 
kexec_file_load and kexec buffer hand-over series in shape.

Am Donnerstag, 18 August 2016, 11:21:13 schrieb Mark Rutland:
> On Thu, Aug 11, 2016 at 08:03:56PM -0300, Thiago Jung Bauermann wrote:
> > 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.

Thanks for laying out out the reasons for your objection. That's very 
helpful.

> * 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.

Yes, this is the case I am aiming for. I'll experiment with a couple of 
different approaches and see how well they perform.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Thiago Jung Bauermann]

Hello Dave,

Sorry for the delay, I was trying to get the other patch series ready as I 
mentioned in the other email.

Am Donnerstag, 18 August 2016, 16:19:46 schrieb Dave Young:
> Since Eric was objecting the extension, I think you should convince him,
> but I will review from code point of view.

Thank you very much for your review!
It looks like this series went as far as it can go, though...

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Hello Dave,

Sorry for the delay, I was trying to get the other patch series ready as I 
mentioned in the other email.

Am Donnerstag, 18 August 2016, 16:19:46 schrieb Dave Young:
> Since Eric was objecting the extension, I think you should convince him,
> but I will review from code point of view.

Thank you very much for your review!
It looks like this series went as far as it can go, though...

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

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

[Thiago Jung Bauermann]

Hello Dave,

Sorry for the delay, I was trying to get the other patch series ready as I 
mentioned in the other email.

Am Donnerstag, 18 August 2016, 16:19:46 schrieb Dave Young:
> Since Eric was objecting the extension, I think you should convince him,
> but I will review from code point of view.

Thank you very much for your review!
It looks like this series went as far as it can go, though...

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec