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
