diff mbox series

[v6,02/15] module: Introduce ksys_finit_module()

Message ID 20241119104922.2772571-3-roberto.sassu@huaweicloud.com (mailing list archive)
State Handled Elsewhere
Headers show
Series integrity: Introduce the Integrity Digest Cache | expand

Checks

Context Check Description
mcgrof/vmtest-modules-next-VM_Test-1 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-0 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-5 success Logs for setup / Setup kdevops environment
mcgrof/vmtest-modules-next-VM_Test-4 success Logs for setup / Setup kdevops environment
mcgrof/vmtest-modules-next-VM_Test-3 success Logs for cleanup / Archive results and cleanup
mcgrof/vmtest-modules-next-VM_Test-2 success Logs for cleanup / Archive results and cleanup
mcgrof/vmtest-modules-next-PR fail merge-conflict

Commit Message

Roberto Sassu Nov. 19, 2024, 10:49 a.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

Introduce ksys_finit_module() to let kernel components request a kernel
module without requiring running modprobe.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/syscalls.h | 10 ++++++++++
 kernel/module/main.c     | 43 ++++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 10 deletions(-)

Comments

Christoph Hellwig Nov. 19, 2024, 12:14 p.m. UTC | #1 
On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Introduce ksys_finit_module() to let kernel components request a kernel
> module without requiring running modprobe.

That does sound more than sketchy, even more so because the commit log
completely fails to explain why you'd need to do that.
Roberto Sassu Nov. 19, 2024, 2:33 p.m. UTC | #2 
On Tue, 2024-11-19 at 13:14 +0100, Christoph Hellwig wrote:
> On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Introduce ksys_finit_module() to let kernel components request a kernel
> > module without requiring running modprobe.
> 
> That does sound more than sketchy, even more so because the commit log
> completely fails to explain why you'd need to do that.

With my solution, the kernel grants access to a file in user space
depending on whether or not its calculated (or fsverity) digest is
found in an application manifest provided by the software vendor.

However, what it happens is that in the early boot phase the parser is
not loaded yet, and the kernel cannot extract the reference digests
from the application manifest.

Thus, calling request_module() and consequently executing modprobe will
fail, since the kernel does not have its reference digest yet.

Instead, loading the kernel module from the kernel itself works,
because only the kernel module needs to be verified, and that can be
done through its appended signature.

Roberto
Luis Chamberlain Nov. 19, 2024, 8:10 p.m. UTC | #3 
On Tue, Nov 19, 2024 at 01:14:02PM +0100, Christoph Hellwig wrote:
> On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Introduce ksys_finit_module() to let kernel components request a kernel
> > module without requiring running modprobe.
> 
> That does sound more than sketchy, even more so because the commit log
> completely fails to explain why you'd need to do that.

I also don't think the commit log is correct, I don't see how the
code is preventing calling modprobe, the indepotent check is intended
to prevent duplicate module init calls which may allocate extra vmalloc
space only to release it. You can test to see if your patch has any
improvments by enabling MODULE_STATS and MODULE_DEBUG_AUTOLOAD_DUPS
and check before / after results of /sys/kernel/debug/modules/stats  ,
right now this patch and commit log is not telling me anything useful.

  Luis
Roberto Sassu Nov. 20, 2024, 9:16 a.m. UTC | #4 
On Tue, 2024-11-19 at 12:10 -0800, Luis Chamberlain wrote:
> On Tue, Nov 19, 2024 at 01:14:02PM +0100, Christoph Hellwig wrote:
> > On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Introduce ksys_finit_module() to let kernel components request a kernel
> > > module without requiring running modprobe.
> > 
> > That does sound more than sketchy, even more so because the commit log
> > completely fails to explain why you'd need to do that.
> 
> I also don't think the commit log is correct, I don't see how the
> code is preventing calling modprobe, the indepotent check is intended
> to prevent duplicate module init calls which may allocate extra vmalloc
> space only to release it. You can test to see if your patch has any
> improvments by enabling MODULE_STATS and MODULE_DEBUG_AUTOLOAD_DUPS
> and check before / after results of /sys/kernel/debug/modules/stats  ,
> right now this patch and commit log is not telling me anything useful.

Maybe I misunderstood the code, but what causes modprobe to be executed
in user space is a call to request_module().

In my patch, I simply ported the code of the finit_module() system call
to _ksys_finit_module(), net the conversion from struct fd to struct
file, which is kept in the system call code.

Also, from the kernel side, I'm providing a valid address for module
arguments, and duplicating the string either with kmemdup() or
strndup_user() in load_module(), depending on where the memory belongs
to.

Again, maybe I misunderstood, but I'm not introducing any functional
change to the current behavior, the kernel side also provides a file
descriptor and module arguments as user space would do (e.g. by
executing insmod).

As for the motivation, please have a look at my response to Christian:

https://lore.kernel.org/linux-integrity/ZzzvAPetAn7CUEvx@bombadil.infradead.org/T/#ma8656b921bb5bfb60e7f10331061d462a87ce9f4


In addition, you could also see how ksys_finit_module() is used here:

https://lore.kernel.org/linux-integrity/20241119104922.2772571-8-roberto.sassu@huaweicloud.com/

Thanks

Roberto
Roberto Sassu Nov. 20, 2024, 9:18 a.m. UTC | #5 
On Wed, 2024-11-20 at 10:16 +0100, Roberto Sassu wrote:
> On Tue, 2024-11-19 at 12:10 -0800, Luis Chamberlain wrote:
> > On Tue, Nov 19, 2024 at 01:14:02PM +0100, Christoph Hellwig wrote:
> > > On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Introduce ksys_finit_module() to let kernel components request a kernel
> > > > module without requiring running modprobe.
> > > 
> > > That does sound more than sketchy, even more so because the commit log
> > > completely fails to explain why you'd need to do that.
> > 
> > I also don't think the commit log is correct, I don't see how the
> > code is preventing calling modprobe, the indepotent check is intended
> > to prevent duplicate module init calls which may allocate extra vmalloc
> > space only to release it. You can test to see if your patch has any
> > improvments by enabling MODULE_STATS and MODULE_DEBUG_AUTOLOAD_DUPS
> > and check before / after results of /sys/kernel/debug/modules/stats  ,
> > right now this patch and commit log is not telling me anything useful.
> 
> Maybe I misunderstood the code, but what causes modprobe to be executed
> in user space is a call to request_module().
> 
> In my patch, I simply ported the code of the finit_module() system call
> to _ksys_finit_module(), net the conversion from struct fd to struct
> file, which is kept in the system call code.
> 
> Also, from the kernel side, I'm providing a valid address for module
> arguments, and duplicating the string either with kmemdup() or
> strndup_user() in load_module(), depending on where the memory belongs
> to.
> 
> Again, maybe I misunderstood, but I'm not introducing any functional
> change to the current behavior, the kernel side also provides a file
> descriptor and module arguments as user space would do (e.g. by
> executing insmod).
> 
> As for the motivation, please have a look at my response to Christian:

Christoph, of course.

Roberto


> https://lore.kernel.org/linux-integrity/ZzzvAPetAn7CUEvx@bombadil.infradead.org/T/#ma8656b921bb5bfb60e7f10331061d462a87ce9f4
> 
> 
> In addition, you could also see how ksys_finit_module() is used here:
> 
> https://lore.kernel.org/linux-integrity/20241119104922.2772571-8-roberto.sassu@huaweicloud.com/
> 
> Thanks
> 
> Roberto
Luis Chamberlain Nov. 25, 2024, 11:40 p.m. UTC | #6 
On Wed, Nov 20, 2024 at 10:16:23AM +0100, Roberto Sassu wrote:
> Again, maybe I misunderstood, but I'm not introducing any functional
> change to the current behavior, the kernel side also provides a file
> descriptor and module arguments as user space would do (e.g. by
> executing insmod).

The commit log does an extremely poor job to make this clear, all you
are doing here is adding a helper. The commit log should be super clear
on that and it is not.

  Luis
Roberto Sassu Nov. 26, 2024, 7:56 a.m. UTC | #7 
On Mon, 2024-11-25 at 15:40 -0800, Luis Chamberlain wrote:
> On Wed, Nov 20, 2024 at 10:16:23AM +0100, Roberto Sassu wrote:
> > Again, maybe I misunderstood, but I'm not introducing any functional
> > change to the current behavior, the kernel side also provides a file
> > descriptor and module arguments as user space would do (e.g. by
> > executing insmod).
> 
> The commit log does an extremely poor job to make this clear, all you
> are doing here is adding a helper. The commit log should be super clear
> on that and it is not.

Correct, yes. Will do a much better and clear commit message for next
version.

Thanks

Roberto
diff mbox series

Patch

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 5758104921e6..18bb346bb793 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1233,6 +1233,16 @@  int ksys_ipc(unsigned int call, int first, unsigned long second,
 int compat_ksys_ipc(u32 call, int first, int second,
 	u32 third, u32 ptr, u32 fifth);
 
+#ifdef CONFIG_MODULES
+int ksys_finit_module(struct file *f, const char *kargs, int flags);
+#else
+static inline int ksys_finit_module(struct file *f, const char *kargs,
+				    int flags)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 /*
  * The following kernel syscall equivalents are just wrappers to fs-internal
  * functions. Therefore, provide stubs to be inlined at the callsites.
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 49b9bca9de12..81f79c9ea637 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2852,7 +2852,7 @@  static int early_mod_check(struct load_info *info, int flags)
  * zero, and we rely on this for optional sections.
  */
 static int load_module(struct load_info *info, const char __user *uargs,
-		       int flags)
+		       const char *kargs, int flags)
 {
 	struct module *mod;
 	bool module_allocated = false;
@@ -2953,7 +2953,13 @@  static int load_module(struct load_info *info, const char __user *uargs,
 	flush_module_icache(mod);
 
 	/* Now copy in args */
-	mod->args = strndup_user(uargs, ~0UL >> 1);
+	if (kargs) {
+		mod->args = kstrndup(kargs, ~0UL >> 1, GFP_KERNEL);
+		if (!mod->args)
+			mod->args = ERR_PTR(-ENOMEM);
+	} else {
+		mod->args = strndup_user(uargs, ~0UL >> 1);
+	}
 	if (IS_ERR(mod->args)) {
 		err = PTR_ERR(mod->args);
 		goto free_arch_cleanup;
@@ -3083,7 +3089,7 @@  SYSCALL_DEFINE3(init_module, void __user *, umod,
 		return err;
 	}
 
-	return load_module(&info, uargs, 0);
+	return load_module(&info, uargs, NULL, 0);
 }
 
 struct idempotent {
@@ -3170,7 +3176,8 @@  static int idempotent_wait_for_completion(struct idempotent *u)
 	return u->ret;
 }
 
-static int init_module_from_file(struct file *f, const char __user * uargs, int flags)
+static int init_module_from_file(struct file *f, const char __user * uargs,
+				 const char *kargs, int flags)
 {
 	struct load_info info = { };
 	void *buf = NULL;
@@ -3195,10 +3202,11 @@  static int init_module_from_file(struct file *f, const char __user * uargs, int
 		info.len = len;
 	}
 
-	return load_module(&info, uargs, flags);
+	return load_module(&info, uargs, kargs, flags);
 }
 
-static int idempotent_init_module(struct file *f, const char __user * uargs, int flags)
+static int idempotent_init_module(struct file *f, const char __user * uargs,
+				  const char *kargs, int flags)
 {
 	struct idempotent idem;
 
@@ -3207,7 +3215,7 @@  static int idempotent_init_module(struct file *f, const char __user * uargs, int
 
 	/* Are we the winners of the race and get to do this? */
 	if (!idempotent(&idem, file_inode(f))) {
-		int ret = init_module_from_file(f, uargs, flags);
+		int ret = init_module_from_file(f, uargs, kargs, flags);
 		return idempotent_complete(&idem, ret);
 	}
 
@@ -3217,15 +3225,16 @@  static int idempotent_init_module(struct file *f, const char __user * uargs, int
 	return idempotent_wait_for_completion(&idem);
 }
 
-SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
+static int _ksys_finit_module(struct file *f, int fd, const char __user * uargs,
+			      const char *kargs, int flags)
 {
 	int err;
-	struct fd f;
 
 	err = may_init_module();
 	if (err)
 		return err;
 
+	/* fd = -1 if called from the kernel. */
 	pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
 
 	if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS
@@ -3233,8 +3242,22 @@  SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
 		      |MODULE_INIT_COMPRESSED_FILE))
 		return -EINVAL;
 
+	err = idempotent_init_module(f, uargs, kargs, flags);
+	return err;
+}
+
+int ksys_finit_module(struct file *f, const char *kargs, int flags)
+{
+	return _ksys_finit_module(f, -1, NULL, kargs, flags);
+}
+
+SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
+{
+	int err;
+	struct fd f;
+
 	f = fdget(fd);
-	err = idempotent_init_module(fd_file(f), uargs, flags);
+	err = _ksys_finit_module(fd_file(f), fd, uargs, NULL, flags);
 	fdput(f);
 	return err;
 }