| Message ID | 3212-5f024c00-215-220fe080@174542169 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: core: fix quirks_param_set() writing to a const pointer | expand |
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
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; }
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
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.
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);
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(-)