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
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 <[email protected]>
---
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
Prevent allocations from integer overflow by using size_add().
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Sort the headers in alphabetic order in order to ease
the maintenance for this part.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
---
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
The multi-line comment style in the file is rather arbitrary.
Make it follow the standard one.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
--
Kees Cook
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 <[email protected]>
Reviewed-by: Luis Chamberlain <[email protected]>
Luis