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