Re: [PATCH v3 1/4] kexec: simplify compat_sys_kexec_load

3 messages, 3 authors, 2021-05-18 · open the first message on its own page

Re: [PATCH v3 1/4] kexec: simplify compat_sys_kexec_load

From: Eric W. Biederman <hidden>
Date: 2021-05-18 03:58:11

Arnd Bergmann [off-list ref] writes:
From: Arnd Bergmann <arnd@arndb.de>

The compat version of sys_kexec_load() uses compat_alloc_user_space to
convert the user-provided arguments into the native format.

Move the conversion into the regular implementation with
an in_compat_syscall() check to simplify it and avoid the
compat_alloc_user_space() call.

compat_sys_kexec_load() now behaves the same as sys_kexec_load().
Is it possible to do this without in_compat_syscall(),
and casting pointers to a wrong type?

We open ourselves up to bugs whenever we lie to the type system.

Skimming through the code it looks like it should be possible
to not need the in_compat_syscall and the casts to the wrong
type by changing the order of the code a little bit.

Eric

quoted hunk
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/kexec.h |  2 -
 kernel/kexec.c        | 95 +++++++++++++++++++------------------------
 2 files changed, 42 insertions(+), 55 deletions(-)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0c994ae37729..f61e310d7a85 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -88,14 +88,12 @@ struct kexec_segment {
 	size_t memsz;
 };
 
-#ifdef CONFIG_COMPAT
 struct compat_kexec_segment {
 	compat_uptr_t buf;
 	compat_size_t bufsz;
 	compat_ulong_t mem;	/* User space sees this as a (void *) ... */
 	compat_size_t memsz;
 };
-#endif
 
 #ifdef CONFIG_KEXEC_FILE
 struct purgatory_info {
diff --git a/kernel/kexec.c b/kernel/kexec.c
index c82c6c06f051..6618b1d9f00b 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -19,21 +19,46 @@
 
 #include "kexec_internal.h"
 
+static int copy_user_compat_segment_list(struct kimage *image,
+					 unsigned long nr_segments,
+					 void __user *segments)
+{
+	struct compat_kexec_segment __user *cs = segments;
+	struct compat_kexec_segment segment;
+	int i;
+
+	for (i = 0; i < nr_segments; i++) {
+		if (copy_from_user(&segment, &cs[i], sizeof(segment)))
+			return -EFAULT;
+
+		image->segment[i] = (struct kexec_segment) {
+			.buf   = compat_ptr(segment.buf),
+			.bufsz = segment.bufsz,
+			.mem   = segment.mem,
+			.memsz = segment.memsz,
+		};
+	}
+
+	return 0;
+}
+
+
 static int copy_user_segment_list(struct kimage *image,
 				  unsigned long nr_segments,
 				  struct kexec_segment __user *segments)
 {
-	int ret;
 	size_t segment_bytes;
 
 	/* Read in the segments */
 	image->nr_segments = nr_segments;
 	segment_bytes = nr_segments * sizeof(*segments);
-	ret = copy_from_user(image->segment, segments, segment_bytes);
-	if (ret)
-		ret = -EFAULT;
+	if (in_compat_syscall())
+		return copy_user_compat_segment_list(image, nr_segments, segments);
 
-	return ret;
+	if (copy_from_user(image->segment, segments, segment_bytes))
+		return -EFAULT;
+
+	return 0;
 }
 
 static int kimage_alloc_init(struct kimage **rimage, unsigned long entry,
@@ -233,8 +258,9 @@ static inline int kexec_load_check(unsigned long nr_segments,
 	return 0;
 }
 
-SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
-		struct kexec_segment __user *, segments, unsigned long, flags)
+static int kernel_kexec_load(unsigned long entry, unsigned long nr_segments,
+			     struct kexec_segment __user * segments,
+			     unsigned long flags)
 {
 	int result;
 
@@ -265,57 +291,20 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 	return result;
 }
 
+SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
+		struct kexec_segment __user *, segments, unsigned long, flags)
+{
+	return kernel_kexec_load(entry, nr_segments, segments, flags);
+}
+
 #ifdef CONFIG_COMPAT
 COMPAT_SYSCALL_DEFINE4(kexec_load, compat_ulong_t, entry,
 		       compat_ulong_t, nr_segments,
 		       struct compat_kexec_segment __user *, segments,
 		       compat_ulong_t, flags)
 {
-	struct compat_kexec_segment in;
-	struct kexec_segment out, __user *ksegments;
-	unsigned long i, result;
-
-	result = kexec_load_check(nr_segments, flags);
-	if (result)
-		return result;
-
-	/* Don't allow clients that don't understand the native
-	 * architecture to do anything.
-	 */
-	if ((flags & KEXEC_ARCH_MASK) == KEXEC_ARCH_DEFAULT)
-		return -EINVAL;
-
-	ksegments = compat_alloc_user_space(nr_segments * sizeof(out));
-	for (i = 0; i < nr_segments; i++) {
-		result = copy_from_user(&in, &segments[i], sizeof(in));
-		if (result)
-			return -EFAULT;
-
-		out.buf   = compat_ptr(in.buf);
-		out.bufsz = in.bufsz;
-		out.mem   = in.mem;
-		out.memsz = in.memsz;
-
-		result = copy_to_user(&ksegments[i], &out, sizeof(out));
-		if (result)
-			return -EFAULT;
-	}
-
-	/* Because we write directly to the reserved memory
-	 * region when loading crash kernels we need a mutex here to
-	 * prevent multiple crash  kernels from attempting to load
-	 * simultaneously, and to prevent a crash kernel from loading
-	 * over the top of a in use crash kernel.
-	 *
-	 * KISS: always take the mutex.
-	 */
-	if (!mutex_trylock(&kexec_mutex))
-		return -EBUSY;
-
-	result = do_kexec_load(entry, nr_segments, ksegments, flags);
-
-	mutex_unlock(&kexec_mutex);
-
-	return result;
+	return kernel_kexec_load(entry, nr_segments,
+				 (struct kexec_segment __user *)segments,
+				 flags);
 }
 #endif

Re: [PATCH v3 1/4] kexec: simplify compat_sys_kexec_load

From: Christoph Hellwig <hch@infradead.org>
Date: 2021-05-18 06:41:53

On Mon, May 17, 2021 at 10:57:24PM -0500, Eric W. Biederman wrote:
We open ourselves up to bugs whenever we lie to the type system.

Skimming through the code it looks like it should be possible
to not need the in_compat_syscall and the casts to the wrong
type by changing the order of the code a little bit.
What kind of bug do you expect?  We must only copy from user addresses
once anyway.  I've never seen bugs due the use of in_compat_syscall,
but plenty due to cruft code trying to avoid it.

Re: [PATCH v3 1/4] kexec: simplify compat_sys_kexec_load

From: Arnd Bergmann <arnd@kernel.org>
Date: 2021-05-18 07:45:46

On Tue, May 18, 2021 at 8:40 AM Christoph Hellwig [off-list ref] wrote:
On Mon, May 17, 2021 at 10:57:24PM -0500, Eric W. Biederman wrote:
quoted
We open ourselves up to bugs whenever we lie to the type system.

Skimming through the code it looks like it should be possible
to not need the in_compat_syscall and the casts to the wrong
type by changing the order of the code a little bit.
There are obviously other ways of doing the same. The reason for doing it
this specific way is so I can eliminate the compat entry point entirely in
patch 4/4.
What kind of bug do you expect?  We must only copy from user addresses
once anyway.  I've never seen bugs due the use of in_compat_syscall,
but plenty due to cruft code trying to avoid it.
Right, I've used the same approach of passing a native-typed __user pointer
and converting it in a copy_from_user/copy_to_user wrapper in a number of
other places, as this tends to produce the most readable version by
concentrating the tricky logic in the one place that already has to be careful.

Most of the bugs I've seen with compat code are from duplicated code paths
that diverge over time when a bugfix for the native version is applied
incorrectly
or not at all to the compat version.

        Arnd
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help