[PATCH v2 0/5] params: harden string ops and allocatio ops

[PATCH v2 0/5] params: harden string ops and allocatio ops

[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, but I'm open for another option(s).

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             | 52 ++++++++++++++++++++-----------------
 2 files changed, 31 insertions(+), 27 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 1/5] params: Introduce the param_unknown_fn type

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/moduleparam.h | 6 +++---
 kernel/params.c             | 8 ++------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 4fa9726bc328..bfb85fd13e1f 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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
diff --git a/kernel/params.c b/kernel/params.c
index 2d4a0564697e..626fa8265932 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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;
 
-- 
2.40.0.1.gaa8946217a0b

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

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/params.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 626fa8265932..f8e3c4139854 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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;
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 2/5] params: Do not go over the limit when getting the string length

[Kees Cook]

On Mon, Oct 02, 2023 at 03:48:53PM +0300, Andy Shevchenko wrote:
> We can use strnlen() even on early stages and it prevents from
> going over the string boundaries in case it's already too long.

It makes sense to avoid calling strlen() multiple times. I don't have
much opinion one way or another about using strnlen() here, since we
know the string will be terminated.

-Kees

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  kernel/params.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index 626fa8265932..f8e3c4139854 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -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;
> -- 
> 2.40.0.1.gaa8946217a0b
> 

-- 
Kees Cook

[PATCH v2 3/5] params: Use size_add() for kmalloc()

[Andy Shevchenko]

Prevent allocations from integer overflow by using size_add().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/params.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/params.c b/kernel/params.c
index f8e3c4139854..c3a029fe183d 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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;
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 4/5] params: Sort headers

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/params.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index c3a029fe183d..eb55b32399b4 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -3,18 +3,18 @@
    Copyright (C) 2001 Rusty Russell.
 
 */
+#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/string.h>
-#include <linux/errno.h>
 #include <linux/module.h>
 #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>
+#include <linux/slab.h>
+#include <linux/string.h>
 
 #ifdef CONFIG_SYSFS
 /* Protects all built-in parameters, modules use their own param_lock */
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 5/5] params: Fix multi-line comment style

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/params.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index eb55b32399b4..2e447f8ae183 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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);
 	}
 }
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 0/5] params: harden string ops and allocatio ops

[Kees Cook]

On Mon, Oct 02, 2023 at 03:48:51PM +0300, Andy Shevchenko wrote:
> 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, but I'm open for another option(s).
> 
> 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

Seems like a nice bit of clean-up.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v2 0/5] params: harden string ops and allocatio ops

[Luis Chamberlain]

On Mon, Oct 02, 2023 at 09:57:59AM -0700, Kees Cook wrote:
> On Mon, Oct 02, 2023 at 03:48:51PM +0300, Andy Shevchenko wrote:
> > 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, but I'm open for another option(s).
> > 
> > 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
> 
> Seems like a nice bit of clean-up.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis