params: harden string ops and allocatio ops

PATCH Applied Andy Shevchenko 2 participants 9 messages Nov 20, 2023 – Dec 1, 2023 Module support

lore

Share & tools

lore

Andy Shevchenko

author

2023-11-20 15:14 UTC #0 lore raw

v3 cover params: harden string ops and allocatio ops

A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups.

Since the main part is rather hardening, I think the Kees' tree is the best fit for the series. It also possible to route via Greg's sysfs (driver core?), but I'm open for another option(s).

Changelog v3:
- added tags (Kees, Luis)

Changelog v2:
- dropped the s*printf() --> sysfs_emit() conversion as it revealed an issue, i.e. reuse getters with non-page-aligned pointer, which would be addressed separately
- added cover letter and clarified the possible route for the series (Luis)

Andy Shevchenko (5):
params: Introduce the param_unknown_fn type
params: Do not go over the limit when getting the string length
params: Use size_add() for kmalloc()
params: Sort headers
params: Fix multi-line comment style

 include/linux/moduleparam.h |  6 ++--
 kernel/params.c             | 56 ++++++++++++++++++++-----------------
 2 files changed, 33 insertions(+), 29 deletions(-)

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Andy Shevchenko

author

2023-11-20 15:14 UTC #1 lore raw

PATCH v3 1/5 params: Introduce the param_unknown_fn type

Introduce a new type for the callback to parse an unknown argument. This unifies function prototypes which takes that as a parameter.

Reviewed-by Luis Chamberlain <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/moduleparam.h | 6 +++---
 kernel/params.c             | 8 ++------
 2 files changed, 5 insertions(+), 9 deletions(-)

include/linux/moduleparam.h +3 -3

index 4fa9726bc328..bfb85fd13e1f 100644
@@ -385,6 +385,8 @@ extern bool parameq(const char *name1, const char *name2);
  */
 extern bool parameqn(const char *name1, const char *name2, size_t n);
 
+typedef int (*parse_unknown_fn)(char *param, char *val, const char *doing, void *arg);
+
 /* Called on module insert or kernel boot */
 extern char *parse_args(const char *name,
 		      char *args,

@@ -392,9 +394,7 @@ extern char *parse_args(const char *name,
 		      unsigned num,
 		      s16 level_min,
 		      s16 level_max,
-		      void *arg,
-		      int (*unknown)(char *param, char *val,
-				     const char *doing, void *arg));
+		      void *arg, parse_unknown_fn unknown);
 
 /* Called by module remove. */
 #ifdef CONFIG_SYSFS

kernel/params.c +2 -6

index 2d4a0564697e..626fa8265932 100644
@@ -120,9 +120,7 @@ static int parse_one(char *param,
 		     unsigned num_params,
 		     s16 min_level,
 		     s16 max_level,
-		     void *arg,
-		     int (*handle_unknown)(char *param, char *val,
-				     const char *doing, void *arg))
+		     void *arg, parse_unknown_fn handle_unknown)
 {
 	unsigned int i;
 	int err;

@@ -165,9 +163,7 @@ char *parse_args(const char *doing,
 		 unsigned num,
 		 s16 min_level,
 		 s16 max_level,
-		 void *arg,
-		 int (*unknown)(char *param, char *val,
-				const char *doing, void *arg))
+		 void *arg, parse_unknown_fn unknown)
 {
 	char *param, *val, *err = NULL;

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Andy Shevchenko

author

2023-11-20 15:14 UTC #2 lore raw

PATCH v3 2/5 params: Do not go over the limit when getting the string length

In reply to Andy Shevchenko

We can use strnlen() even on early stages and it prevents from going over the string boundaries in case it's already too long.

Reviewed-by Luis Chamberlain <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 kernel/params.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

kernel/params.c +6 -3

index 626fa8265932..f8e3c4139854 100644
@@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
 
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
-	if (strlen(val) > 1024) {
+	size_t len, maxlen = 1024;
+
+	len = strnlen(val, maxlen + 1);
+	if (len == maxlen + 1) {
 		pr_err("%s: string parameter too long\n", kp->name);
 		return -ENOSPC;
 	}

@@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct kernel_param *kp)
 	/* This is a hack.  We can't kmalloc in early boot, and we
 	 * don't need to; this mangled commandline is preserved. */
 	if (slab_is_available()) {
-		*(char **)kp->arg = kmalloc_parameter(strlen(val)+1);
+		*(char **)kp->arg = kmalloc_parameter(len + 1);
 		if (!*(char **)kp->arg)
 			return -ENOMEM;
 		strcpy(*(char **)kp->arg, val);

@@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct kernel_param *kp)
 {
 	const struct kparam_string *kps = kp->str;
 
-	if (strlen(val)+1 > kps->maxlen) {
+	if (strnlen(val, kps->maxlen) == kps->maxlen) {
 		pr_err("%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);
 		return -ENOSPC;

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Andy Shevchenko

author

2023-11-20 15:14 UTC #3 lore raw

PATCH v3 3/5 params: Use size_add() for kmalloc()

In reply to Andy Shevchenko

Prevent allocations from integer overflow by using size_add().

Reviewed-by Luis Chamberlain <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 kernel/params.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

kernel/params.c +2 -1

index f8e3c4139854..c3a029fe183d 100644
@@ -11,6 +11,7 @@
 #include <linux/moduleparam.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 #include <linux/ctype.h>
 #include <linux/security.h>

@@ -48,7 +49,7 @@ static void *kmalloc_parameter(unsigned int size)
 {
 	struct kmalloced_param *p;
 
-	p = kmalloc(sizeof(*p) + size, GFP_KERNEL);
+	p = kmalloc(size_add(sizeof(*p), size), GFP_KERNEL);
 	if (!p)
 		return NULL;

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Andy Shevchenko

author

2023-11-20 15:14 UTC #4 lore raw

PATCH v3 4/5 params: Sort headers

In reply to Andy Shevchenko

Sort the headers in alphabetic order in order to ease the maintenance for this part.

Reviewed-by Luis Chamberlain <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 kernel/params.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

kernel/params.c +8 -8

index c3a029fe183d..eb55b32399b4 100644
@@ -3,18 +3,18 @@
    Copyright (C) 2001 Rusty Russell.
 
 */
-#include <linux/kernel.h>
-#include <linux/kstrtox.h>
-#include <linux/string.h>
-#include <linux/errno.h>
-#include <linux/module.h>
-#include <linux/moduleparam.h>
+#include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/kstrtox.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/overflow.h>
-#include <linux/slab.h>
-#include <linux/ctype.h>
 #include <linux/security.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 
 #ifdef CONFIG_SYSFS
 /* Protects all built-in parameters, modules use their own param_lock */

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Andy Shevchenko

author

2023-11-20 15:14 UTC #5 lore raw

PATCH v3 5/5 params: Fix multi-line comment style

In reply to Andy Shevchenko

The multi-line comment style in the file is rather arbitrary. Make it follow the standard one.

Reviewed-by Luis Chamberlain <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 kernel/params.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

kernel/params.c +12 -8

index eb55b32399b4..2e447f8ae183 100644
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/* Helpers for initial module or kernel cmdline parsing
-   Copyright (C) 2001 Rusty Russell.
-
-*/
+/*
+ * Helpers for initial module or kernel cmdline parsing
+ * Copyright (C) 2001 Rusty Russell.
+ */
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/err.h>

@@ -271,8 +271,10 @@ int param_set_charp(const char *val, const struct kernel_param *kp)
 
 	maybe_kfree_parameter(*(char **)kp->arg);
 
-	/* This is a hack.  We can't kmalloc in early boot, and we
-	 * don't need to; this mangled commandline is preserved. */
+	/*
+	 * This is a hack. We can't kmalloc() in early boot, and we
+	 * don't need to; this mangled commandline is preserved.
+	 */
 	if (slab_is_available()) {
 		*(char **)kp->arg = kmalloc_parameter(len + 1);
 		if (!*(char **)kp->arg)

@@ -743,8 +745,10 @@ void module_param_sysfs_remove(struct module *mod)
 {
 	if (mod->mkobj.mp) {
 		sysfs_remove_group(&mod->mkobj.kobj, &mod->mkobj.mp->grp);
-		/* We are positive that no one is using any param
-		 * attrs at this point.  Deallocate immediately. */
+		/*
+		 * We are positive that no one is using any param
+		 * attrs at this point. Deallocate immediately.
+		 */
 		free_module_param_attrs(&mod->mkobj);
 	}
 }

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Andy Shevchenko

author

2023-12-01 15:51 UTC #6 lore raw

In reply to Andy Shevchenko

Quoted 6 lines · Andy Shevchenko

A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups.
Since the main part is rather hardening, I think the Kees' tree is the best fit for the series. It also possible to route via Greg's sysfs (driver core?), but I'm open for another option(s).

Kees, Greg, can you apply this series?
Or should I do something about it?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Kees Cook

Applied

2023-12-01 17:43 UTC #7 lore raw

In reply to Andy Shevchenko

Quoted 8 lines · Andy Shevchenko

A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups.
Since the main part is rather hardening, I think the Kees' tree is the best fit for the series. It also possible to route via Greg's sysfs (driver core?), but I'm open for another option(s).
[...]

Applied to for-next/hardening, thanks!

[1/5] params: Introduce the param_unknown_fn type

      https://git.kernel.org/kees/c/aa61d651412a
[2/5] params: Do not go over the limit when getting the string length
      https://git.kernel.org/kees/c/e6c5b15619a2
[3/5] params: Use size_add() for kmalloc()
      https://git.kernel.org/kees/c/9a4a4b528bff
[4/5] params: Sort headers
      https://git.kernel.org/kees/c/18bdb5a032e8
[5/5] params: Fix multi-line comment style
      https://git.kernel.org/kees/c/c62c9771b7d6

Take care,

Signature · 1 line

Kees Cook

Andy Shevchenko

author

2023-12-01 19:05 UTC #8 lore raw

Quoted 7 lines · Kees Cook

Quoted 6 lines · Andy Shevchenko
A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups.
Since the main part is rather hardening, I think the Kees' tree is the best fit for the series. It also possible to route via Greg's sysfs (driver core?), but I'm open for another option(s).

[...]

Quoted 1 line

Applied to for-next/hardening, thanks!

Awesome, thanks!

Signature · 2 lines

With Best Regards,
Andy Shevchenko