Writing to a const pointer: is this supposed to happen?

Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

In the file drivers/usb/core/quirks.c, I noticed a couple of odd things about the function "quirks_param_set", and I'd like to check whether those are ok according to the kernel programming practices. Here are the relevant lines from the function (several lines omitted):

	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
		char *p, *field;
		for (i = 0, p = (char *)val; p && *p;) {
			field = strsep(&p, ":");
			if (!field)
				break;

In here a const pointer *val is cast into a non-const pointer and then written to by the function strsep, which replaces the first occurrence of the ':' token by a null-byte. Is this allowed?

On a minor side note, this function immediately checks whether the first call to strsep(&p, ":") returned a nullpointer. From what I can learn from the documentation, strsep always returns what *&p was when the strsep was called, and p is verified to be nonzero in the loop condition right before the call to strsep. Is this check actually necessary? Is it a good idea to add a return-value check anyway even if it is not necessary, as an abundance of caution?

Re: Writing to a const pointer: is this supposed to happen?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Mon 2020-06-22 13:35:35, Kars Mulder wrote:
> In the file drivers/usb/core/quirks.c, I noticed a couple of odd things about the function "quirks_param_set", and I'd like to check whether those are ok according to the kernel programming practices. Here are the relevant lines from the function (several lines omitted):
> 
> 	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
> 		char *p, *field;
> 		for (i = 0, p = (char *)val; p && *p;) {
> 			field = strsep(&p, ":");
> 			if (!field)
> 				break;
> 
> In here a const pointer *val is cast into a non-const pointer and then written to by the function strsep, which replaces the first occurrence of the ':' token by a null-byte. Is this allowed?
> 

Odd, indeed... but not likely to cause immediate problems.

You may want to cc relevant maintainers, or even run git
blame and contact author.

You may also want to wrap your lines at 72 characters or so.

Best regards,
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Tuesday, June 23, 2020 21:55 CEST, Pavel Machek wrote: 
> Odd, indeed... but not likely to cause immediate problems.
> 
> You may want to cc relevant maintainers, or even run git
> blame and contact author.

Thank you for your response.

The code was written by Kai-Heng Feng, whom I shall CC. The code is
part of the usbcore module, which does not have a maintainer listed in
MAINTAINERS, but the patch and most other recent patches to usbcore
were signed off exclusively by Greg Kroah-Hartman, so I guess that
makes him the de facto maintainer? I'll CC him as well.

I'm not sure whether it is easy to read the previous messages of this
thread if you got CC'ed just now, so I'll repeat/paraphrase the
important part of my initial mail for your convenience:

> In the file drivers/usb/core/quirks.c, I noticed that the function
> quirks_param_set writes to a const pointer, and would like to check
> whether this is ok with the kernel programming practices. Here are
> the relevant lines from the function (several lines omitted):
> 
> 	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
> 		char *p, *field;
> 		for (i = 0, p = (char *)val; p && *p;) {
> 			field = strsep(&p, ":");
>
> In here a const pointer *val is cast into a non-const pointer and
> then written to by the function strsep, which replaces the first
> occurrence of the ':' token with a null-byte. Is this allowed?

CC: Kai-Heng Feng <kai.heng.feng@canonical.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: Writing to a const pointer: is this supposed to happen?

[Greg Kroah-Hartman]

On Wed, Jun 24, 2020 at 02:34:45PM +0200, Kars Mulder wrote:
> On Tuesday, June 23, 2020 21:55 CEST, Pavel Machek wrote:
> > Odd, indeed... but not likely to cause immediate problems.
> >
> > You may want to cc relevant maintainers, or even run git
> > blame and contact author.
> 
> Thank you for your response.
> 
> The code was written by Kai-Heng Feng, whom I shall CC. The code is
> part of the usbcore module, which does not have a maintainer listed in
> MAINTAINERS, but the patch and most other recent patches to usbcore
> were signed off exclusively by Greg Kroah-Hartman, so I guess that
> makes him the de facto maintainer? I'll CC him as well.
> 
> I'm not sure whether it is easy to read the previous messages of this
> thread if you got CC'ed just now, so I'll repeat/paraphrase the
> important part of my initial mail for your convenience:
> 
> > In the file drivers/usb/core/quirks.c, I noticed that the function
> > quirks_param_set writes to a const pointer, and would like to check
> > whether this is ok with the kernel programming practices. Here are
> > the relevant lines from the function (several lines omitted):
> >
> > 	static int quirks_param_set(const char *val, const struct kernel_param *kp) {
> > 		char *p, *field;
> > 		for (i = 0, p = (char *)val; p && *p;) {
> > 			field = strsep(&p, ":");
> >
> > In here a const pointer *val is cast into a non-const pointer and
> > then written to by the function strsep, which replaces the first
> > occurrence of the ':' token with a null-byte. Is this allowed?
> 
> CC: Kai-Heng Feng <kai.heng.feng@canonical.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

It's not the nicest thing, but as you have noticed, it all works just
fine, right?

Have you hit any runtime issues with this code doing this?  These
strings should be held in writable memory, right?  Or do you see a
codepath where that is not the case?  If so, please feel free to submit
a patch to fix this up (and probably fix up a number of other "set"
functions that deal with struct kernel_param as well.)

thanks,

greg k-h

Re: Writing to a const pointer: is this supposed to happen?

[Kars Mulder]

On Wednesday, June 24, 2020 15:10 CEST, Greg Kroah-Hartman wrote: 
> Have you hit any runtime issues with this code doing this?  These
> strings should be held in writable memory, right?  Or do you see a
> codepath where that is not the case?

I haven't ran into any issues with it; I was just looking at the code
as reference for a patch I'm working on.

I initially assumed that casting a const pointer to non-const and
writing to it would be undefined behaviour, but after reading through
the C99 standard I can't find an unambiguous statement whether it is
undefined behaviour even if the const pointer points to an object that
was originally non-const (like char* -> const char* -> char* casts); it
only says it is undefined behaviour if the object was defined with the
const qualifier.

I should probably stop bothering you with my newbie questions.

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

[David Laight]

From: Kars Mulder
> Sent: 24 June 2020 16:26
> On Wednesday, June 24, 2020 15:10 CEST, Greg Kroah-Hartman wrote:
> > Have you hit any runtime issues with this code doing this?  These
> > strings should be held in writable memory, right?  Or do you see a
> > codepath where that is not the case?
> 
> I haven't ran into any issues with it; I was just looking at the code
> as reference for a patch I'm working on.
> 
> I initially assumed that casting a const pointer to non-const and
> writing to it would be undefined behaviour, but after reading through
> the C99 standard I can't find an unambiguous statement whether it is
> undefined behaviour even if the const pointer points to an object that
> was originally non-const (like char* -> const char* -> char* casts); it
> only says it is undefined behaviour if the object was defined with the
> const qualifier.
> 
> I should probably stop bothering you with my newbie questions.

IISC The C standard 'rules' about casts only really allow a pointer to
be temporarily cast to a different type and then cast back to the
original type before being used.

One effect of that is code like:
void foo(bah_t *bahp)
{
	bah_t bah;
	/* Copy because bahp might be misaligned */
	memcpy(&bah, (void *)bahp, sizeof bah);
doesn't work because the compiler knows that 'bahp' must
point to aligned memory - because that is the only place
it can legitimately come from.
No amount of casts will stop it inlining memcpy() and using
wide register copier.

The code quoted (using strset()) is almost certainly wrong.
The caller is unlikely to expect the input be modified.
Since it doesn't fault the string must be in read-write memory.

Code can be compiled with -Wcast-qual that will generate a
warning when 'const' gets cast away.
There are some legitimate reasons to remove 'const', but not
many.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Kars Mulder]

On Saturday, June 27, 2020 12:24 CEST, David Laight wrote: 
> The code quoted (using strset()) is almost certainly wrong.
> The caller is unlikely to expect the input be modified.
> Since it doesn't fault the string must be in read-write memory.

I tried writing a patch that avoids the writing-to-const-pointer issue
by using the less intrusive sscanf function instead of strsep. It might
avoid a potential bug when somebody wrongly assumes that a
kernel_param_ops.set function will not write to its const char* argument.

Would a patch like this be acceptable, or would I first have to
demonstrate that the current implementation is actually causing real
problems?

This is not yet a formal patch submission; I have some more rigorous
testing to do first. (Relatedly, I'm a bit confused by the requirement
to "always *test* your changes on at least 4 or 5 people" mentioned in
MAINTAINERS. Where should I find those people? Can those people come
from the LKML mailing list, or should I find testers on some third party
forum before submitting my patch to the LKML?)

---
 drivers/usb/core/quirks.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..fe2059d71705 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -27,11 +27,11 @@ static char quirks_param[128];
 
 static int quirks_param_set(const char *val, const struct kernel_param *kp)
 {
-	char *p, *field;
-	u16 vid, pid;
+	const char *p;
+	unsigned short vid, pid;
 	u32 flags;
-	size_t i;
-	int err;
+	size_t i, len;
+	int err, res;
 
 	err = param_set_copystring(val, kp);
 	if (err)
@@ -63,29 +63,16 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 		return -ENOMEM;
 	}
 
-	for (i = 0, p = (char *)val; p && *p;) {
+	for (i = 0, p = val; p && *p;) {
 		/* Each entry consists of VID:PID:flags */
-		field = strsep(&p, ":");
-		if (!field)
-			break;
-
-		if (kstrtou16(field, 16, &vid))
-			break;
-
-		field = strsep(&p, ":");
-		if (!field)
-			break;
-
-		if (kstrtou16(field, 16, &pid))
-			break;
-
-		field = strsep(&p, ",");
-		if (!field || !*field)
+		res = sscanf(p, "%hx:%hx%zn", &vid, &pid, &len);
+		if (res != 2 || *(p+len) != ':')
 			break;
+		p += len+1;
 
 		/* Collect the flags */
-		for (flags = 0; *field; field++) {
-			switch (*field) {
+		for (flags = 0; *p; p++) {
+			switch (*p) {
 			case 'a':
 				flags |= USB_QUIRK_STRING_FETCH_255;
 				break;
@@ -132,11 +119,15 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 				flags |= USB_QUIRK_HUB_SLOW_RESET;
 				break;
 			/* Ignore unrecognized flag characters */
+			case ',':
+				p++;
+				goto collect_flags_end;
 			}
 		}
+collect_flags_end:
 
 		quirk_list[i++] = (struct quirk_entry)
-			{ .vid = vid, .pid = pid, .flags = flags };
+			{ .vid = (u16)vid, .pid = (u16)pid, .flags = flags };
 	}
 
 	if (i < quirk_count)
--
2.27.0

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

[David Laight]

From: Kars Mulder
> Sent: 02 July 2020 00:03
> On Saturday, June 27, 2020 12:24 CEST, David Laight wrote:
> > The code quoted (using strset()) is almost certainly wrong.
> > The caller is unlikely to expect the input be modified.
> > Since it doesn't fault the string must be in read-write memory.
> 
> I tried writing a patch that avoids the writing-to-const-pointer issue
> by using the less intrusive sscanf function instead of strsep. It might
> avoid a potential bug when somebody wrongly assumes that a
> kernel_param_ops.set function will not write to its const char* argument.

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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Kars Mulder]

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

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

[David Laight]

From: Kars Mulder
> Sent: 02 July 2020 22:48
> 
> 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.

There ought to be one that returns a pointer to the first character
that isn't converted - but I'm no expert on the full range of these
functions.

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

The problem with strdup() is you get the extra (unlikely) failure path.
128 bytes of stack won't be a problem if the function is (essentially)
a leaf.
Deep stack use is actually likely to be in the bowels of printf())
inside an obscure error path.
Many years ago (about 1984) I parsed the object code of a program
to find the deepest stack use (no recursion and no function pointers)
so we could set the stack sizes correctly - there wasn't enough
memory to do it properly!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Kars Mulder]

> There ought to be one that returns a pointer to the first character
> that isn't converted - but I'm no expert on the full range of these
> functions.

I've searched for a function that parses an int from a string and 
stores a pointer to the end; I can find some function simple_strtoul
that matches this criterion, but it's documented as

    "This function has caveats. Please use kstrtoul instead."

... and kstrtoul does not store a pointer to the end. The documentation
of kstrtoul describes simple_strtoul as obsolete as well. Also, there's
no simple_strtou16 function.

It seems that the standard C function strtoul has the behaviour you
describe as well, but this function is not defined in the kernel except
for certain specific architectures.

> The problem with strdup() is you get the extra (unlikely) failure path.
> 128 bytes of stack won't be a problem if the function is (essentially)
> a leaf.
> Deep stack use is actually likely to be in the bowels of printf())
> inside an obscure error path.

The function already makes a call to kcalloc, so the unlikely out-of-
memory error path already exists; a second memory allocation just
makes it slightly less unlikely. The two new out-of-memory conditions
do happen at different points of the function though, making them
have different side effects. I could fix this by moving my code.

As for this function being a leaf: keep in mind that this function has
the potential of calling printk in an obscure error condition (the user-
provided parameter being longer that 128 characters); quirks_param_set
calls param_set_copystring, which on its turn calls pr_err, which is a
macro for printk.

Meanwhile, here's a patch for copying the parameter to the stack:

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

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..86b1a6739b4e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -12,6 +12,8 @@
 #include <linux/usb/hcd.h>
 #include "usb.h"
 
+#define QUIRKS_PARAM_SIZE 128
+
 struct quirk_entry {
 	u16 vid;
 	u16 pid;
@@ -23,19 +25,21 @@ static DEFINE_MUTEX(quirk_mutex);
 static struct quirk_entry *quirk_list;
 static unsigned int quirk_count;
 
-static char quirks_param[128];
+static char quirks_param[QUIRKS_PARAM_SIZE];
 
-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 val[QUIRKS_PARAM_SIZE];
 	char *p, *field;
 	u16 vid, pid;
 	u32 flags;
 	size_t i;
 	int err;
 
-	err = param_set_copystring(val, kp);
+	err = param_set_copystring(value, kp);
 	if (err)
 		return err;
+	strscpy(val, value, sizeof(val));
 
 	mutex_lock(&quirk_mutex);
 
--
2.27.0

Re: Writing to a const pointer: is this supposed to happen?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

On Fri 2020-07-03 15:23:38, Kars Mulder wrote:
> > There ought to be one that returns a pointer to the first character
> > that isn't converted - but I'm no expert on the full range of these
> > functions.
> 
> I've searched for a function that parses an int from a string and
> stores a pointer to the end; I can find some function simple_strtoul
> that matches this criterion, but it's documented as
> 
>     "This function has caveats. Please use kstrtoul instead."
> 
> ... and kstrtoul does not store a pointer to the end. The documentation
> of kstrtoul describes simple_strtoul as obsolete as well. Also, there's
> no simple_strtou16 function.
> 
> It seems that the standard C function strtoul has the behaviour you
> describe as well, but this function is not defined in the kernel except
> for certain specific architectures.
> 
> > The problem with strdup() is you get the extra (unlikely) failure path.
> > 128 bytes of stack won't be a problem if the function is (essentially)
> > a leaf.
> > Deep stack use is actually likely to be in the bowels of printf())
> > inside an obscure error path.
> 
> The function already makes a call to kcalloc, so the unlikely out-of-
> memory error path already exists; a second memory allocation just
> makes it slightly less unlikely. The two new out-of-memory conditions
> do happen at different points of the function though, making them
> have different side effects. I could fix this by moving my code.
> 
> As for this function being a leaf: keep in mind that this function has
> the potential of calling printk in an obscure error condition (the user-
> provided parameter being longer that 128 characters); quirks_param_set
> calls param_set_copystring, which on its turn calls pr_err, which is a
> macro for printk.
> 
> Meanwhile, here's a patch for copying the parameter to the stack:

Looks good, I guess Signed-off-by would be useful.

								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] usb: core: fix quirks_param_set() writing to a const pointer

[Kars Mulder]

The function quirks_param_set() takes as argument a const char* pointer
to the new value of the usbcore.quirks parameter. It then casts this
pointer to a non-const char* pointer and passes it to the strsep()
function, which overwrites the value.

Fix this by copying the value to a local buffer on the stack and 
letting that buffer be written to by strsep().

Fixes: 027bd6cafd9a ("usb: core: Add "quirks" parameter for usbcore")
Signed-off-by: Kars Mulder <kerneldev@karsmulder.nl>

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

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..86b1a6739b4e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -12,6 +12,8 @@
 #include <linux/usb/hcd.h>
 #include "usb.h"
 
+#define QUIRKS_PARAM_SIZE 128
+
 struct quirk_entry {
 	u16 vid;
 	u16 pid;
@@ -23,19 +25,21 @@ static DEFINE_MUTEX(quirk_mutex);
 static struct quirk_entry *quirk_list;
 static unsigned int quirk_count;
 
-static char quirks_param[128];
+static char quirks_param[QUIRKS_PARAM_SIZE];
 
-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 val[QUIRKS_PARAM_SIZE];
 	char *p, *field;
 	u16 vid, pid;
 	u32 flags;
 	size_t i;
 	int err;
 
-	err = param_set_copystring(val, kp);
+	err = param_set_copystring(value, kp);
 	if (err)
 		return err;
+	strscpy(val, value, sizeof(val));
 
 	mutex_lock(&quirk_mutex);
 
-- 
2.27.0

Re: [PATCH] usb: core: fix quirks_param_set() writing to a const pointer

[Greg Kroah-Hartman]

On Sun, Jul 05, 2020 at 11:53:27PM +0200, Kars Mulder wrote:
> The function quirks_param_set() takes as argument a const char* pointer
> to the new value of the usbcore.quirks parameter. It then casts this
> pointer to a non-const char* pointer and passes it to the strsep()
> function, which overwrites the value.
> 
> Fix this by copying the value to a local buffer on the stack and 
> letting that buffer be written to by strsep().
> 
> Fixes: 027bd6cafd9a ("usb: core: Add "quirks" parameter for usbcore")
> Signed-off-by: Kars Mulder <kerneldev@karsmulder.nl>
> 
> ---
>  drivers/usb/core/quirks.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index e0b77674869c..86b1a6739b4e 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -12,6 +12,8 @@
>  #include <linux/usb/hcd.h>
>  #include "usb.h"
>  
> +#define QUIRKS_PARAM_SIZE 128
> +
>  struct quirk_entry {
>  	u16 vid;
>  	u16 pid;
> @@ -23,19 +25,21 @@ static DEFINE_MUTEX(quirk_mutex);
>  static struct quirk_entry *quirk_list;
>  static unsigned int quirk_count;
>  
> -static char quirks_param[128];
> +static char quirks_param[QUIRKS_PARAM_SIZE];
>  
> -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 val[QUIRKS_PARAM_SIZE];

That's a lot of stack space, is it really needed?  Can we just use a
static variable instead, or dynamically allocate this?

thanks,

greg k-h

Re: [PATCH] usb: core: fix quirks_param_set() writing to a const pointer

[Kars Mulder]

On Monday, July 06, 2020 12:34 CEST, Greg Kroah-Hartman wrote: 
> That's a lot of stack space, is it really needed?  Can we just use a
> static variable instead, or dynamically allocate this?

It is very possible to statically or dynamically allocate this.

Statically reserving an additional 128 bytes regardless of whether
this feature is actually used feels a bit wasteful, so I'd prefer
stack or dynamic allocation.

An earlier draft of my patch did dynamically allocate this memory;
early discussion (https://lkml.org/lkml/2020/7/3/248) suggested that
dynamic allocation has the disadvantage of introducing a new obscure
error condition:

On Friday, July 03, 2020 10:13 CEST, David Laight wrote: 
> The problem with strdup() is you get the extra (unlikely) failure path.
> 128 bytes of stack won't be a problem if the function is (essentially)
> a leaf.

So I rewrote my patch to use stack-based allocation instead. I've attached
a patch using kstrdup at the end of this mail.

I think that the new obscure error condition caused by kstrdup() isn't
too bad since original code already had a call to kcalloc() anyway; the
possibility for the function to fail due to out-of-memory errors
already existed. In this version of the patch, there may however be a
possible issue that different code paths are taken depending on when
the out-of-memory error occurs, resulting in different side effects:

	val = kstrdup(value, GFP_KERNEL);
	if (!val)
		return -ENOMEM;

	err = param_set_copystring(val, kp);
	[...]
	
	if (quirk_list) {
		kfree(quirk_list);
		quirk_list = NULL;
	}

	quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry),
			     GFP_KERNEL);


If the OOM happens at the kstrdup() step (which I added), then the old
value of the parameter will be retained. If the OOM happens at the
kcalloc() step, then the kernel will remember the value of the new
parameter (and return that value if asked what the parameter's current
value is), but but neither the old nor the new parameter will be in
effect: the quirk_list will be a NULL pointer and the quirks module
will behave as if the usbcore.quirks parameter is empty.

I could move my code to make an OOM at kstrdup() have the same side
effects as an OOM at kcalloc(), but I'd personally think that the first
kind of behaviour "setting the parameter failed, nothing happened" is
more correct than the latter kind "setting the parameter failed, the
parameter is now in limbo".

---
 drivers/usb/core/quirks.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index e0b77674869c..c96c50faccf7 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,10 +66,11 @@ 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;
 	}
 
-	for (i = 0, p = (char *)val; p && *p;) {
+	for (i = 0, p = val; p && *p;) {
 		/* Each entry consists of VID:PID:flags */
 		field = strsep(&p, ":");
 		if (!field)
@@ -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

Re: [PATCH] usb: core: fix quirks_param_set() writing to a const pointer

[Greg Kroah-Hartman]

On Mon, Jul 06, 2020 at 02:57:59PM +0200, Kars Mulder wrote:
> On Monday, July 06, 2020 12:34 CEST, Greg Kroah-Hartman wrote: 
> > That's a lot of stack space, is it really needed?  Can we just use a
> > static variable instead, or dynamically allocate this?
> 
> It is very possible to statically or dynamically allocate this.
> 
> Statically reserving an additional 128 bytes regardless of whether
> this feature is actually used feels a bit wasteful, so I'd prefer
> stack or dynamic allocation.
> 
> An earlier draft of my patch did dynamically allocate this memory;
> early discussion (https://lkml.org/lkml/2020/7/3/248) suggested that
> dynamic allocation has the disadvantage of introducing a new obscure
> error condition:
> 
> On Friday, July 03, 2020 10:13 CEST, David Laight wrote: 
> > The problem with strdup() is you get the extra (unlikely) failure path.
> > 128 bytes of stack won't be a problem if the function is (essentially)
> > a leaf.

Just test for memory allocation failure and handle it properly, it isn't
hard to do.

128 bytes on the stack can be a problem, don't get in the habit of doing
so please.

thanks,

greg k-h

Re: [PATCH] usb: core: fix quirks_param_set() writing to a const pointer

[Kars Mulder]

On Monday, July 06, 2020 15:07 CEST, Greg Kroah-Hartman wrote: 
> Just test for memory allocation failure and handle it properly, it isn't
> hard to do.
> 
> 128 bytes on the stack can be a problem, don't get in the habit of doing
> so please.

Thank you for the clarification. The next version of my patch shall use
kstrdup() instead of copying to the stack.