linux-kernel - RE: Writing to a const pointer: is this supposed to happen?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <297d-5efe5600-1cf-7eab9a80@67481175>
Date:   Thu, 02 Jul 2020 23:48:02 +0200
From:   "Kars Mulder" <kerneldev@...smulder.nl>
To:     "David Laight" <David.Laight@...LAB.COM>
Cc:     "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Pavel Machek" <pavel@....cz>,
        linux-kernel@...r.kernel.org 
        <linux-kernel@...r.kernel.org>,
        "Kai-Heng Feng" <kai.heng.feng@...onical.com>
Subject: RE: Writing to a const pointer: is this 
 supposed to happen?

On Thursday, July 02, 2020 09:55 CEST, David Laight wrote: 
> Hmm... sscanf() is also horrid.
> Surprisingly difficult to use correctly.
> 
> It is usually best to use strchr() (and maybe str[c]scn())
> to parse strings.
> For numbers use whatever the kernels current 'favourite' implementation
> of strtoul() is called.

I thought that using sscanf would clean up the code a bit compared to
several haphazard calls, but I can see your point about sscanf being
difficult to use correctly.

The kernel functions kstrtou16 seem to expect a null-terminated string
as argument. Since there are no null-bytes after the numbers we want to
parse, it becomes necessary to copy at least part of the strings to a
buffer.

If we're copying strings to buffers anyway, I think the simplest
solution would be to just kstrdup the entire parameter and not touch
the rest of the string parsing code. This has the disadvantage of
having an extra memory allocation to keep track of.

Since the parameter is currently restricted to 128 characters at
most, it may alternatively be possible to copy the parameter to
a 128-byte buffer on the stack. This has the advantage of having
to keep track of one less memory allocation, but the disadvantage
of using 128 bytes more stack space; I'm not sure whether that's
acceptable.

Here's a sample patch involving kstrdup:

---
 drivers/usb/core/quirks.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..3b64b0be2563 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -25,17 +25,23 @@ static unsigned int quirk_count;

 static char quirks_param[128];

-static int quirks_param_set(const char *val, const struct kernel_param *kp)
+static int quirks_param_set(const char *value, const struct kernel_param *kp)
 {
-	char *p, *field;
+	char *val, *p, *field;
 	u16 vid, pid;
 	u32 flags;
 	size_t i;
 	int err;

+	val = kstrdup(value, GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
 	err = param_set_copystring(val, kp);
-	if (err)
+	if (err) {
+		kfree(val);
 		return err;
+	}

 	mutex_lock(&quirk_mutex);

@@ -60,6 +66,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 	if (!quirk_list) {
 		quirk_count = 0;
 		mutex_unlock(&quirk_mutex);
+		kfree(val);
 		return -ENOMEM;
 	}

@@ -144,6 +151,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)

 unlock:
 	mutex_unlock(&quirk_mutex);
+	kfree(val);

 	return 0;
 }
--
2.27.0