driver core: platform: fix race condition with driver_override
PATCH 1/1 driver core: platform: fix race condition with driver_override
The driver_override implementation is susceptible to race condition when different threads are reading vs storing a different driver override. Add locking to avoid race condition.
--- drivers/base/platform.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/base/platform.c +9 -2
@@ -866,7 +866,7 @@ static ssize_t driver_override_store(struct device *dev, const char *buf, size_t count) {struct platform_device *pdev =to_platform_device (dev); -char *driver_override, *old = pdev->driver_override, *cp; +char *driver_override, *old, *cp;if (count >PATH_MAX )return -EINVAL ;
@@ -879,12 +879,15 @@ static ssize_t driver_override_store(struct device *dev,if (cp) *cp ='\0' ; +device_lock (dev); + old = pdev->driver_override;if (strlen (driver_override)) { pdev->driver_override = driver_override; }else {kfree (driver_override); pdev->driver_override =NULL ; } +device_unlock (dev);kfree (old);
@@ -895,8 +898,12 @@ static ssize_t driver_override_show(struct device *dev, struct device_attribute *attr, char *buf) { struct platform_device *pdev = to_platform_device(dev); +ssize_t len; -return sprintf (buf,"%s\n" , pdev->driver_override); +device_lock (dev); + len =sprintf (buf,"%s\n" , pdev->driver_override); +device_unlock (dev); +return len; }static DEVICE_ATTR_RW (driver_override);
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
Greg KH
maintainerQuoted 29 lines · Adrian Salido
The
driver_overrideimplementation is susceptible to race condition when different threads are reading vs storing a different driver override. Add locking to avoid race condition.Fixes 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")Signed-off-by Adrian Salido <[email protected]>--- drivers/base/platform.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/base/platform.c +2
-1@@ -866,7 +866,7 @@ static ssize_t driver_override_store(struct device *dev, const char *buf, size_t count) {struct platform_device *pdev =to_platform_device (dev); -char *driver_override, *old = pdev->driver_override, *cp; +char *driver_override, *old, *cp;if (count >PATH_MAX )return -EINVAL ;@@ -879,12 +879,15 @@ static ssize_t driver_override_store(struct device *dev,if (cp) *cp ='\0' ; +device_lock (dev);
drivers/base/platform.c static ssize_t driver_override_store(struct device *dev, · line 892
Quoted 10 lines
drivers/base/platform.c +2
-0+ old = pdev->driver_override;if (strlen (driver_override)) { pdev->driver_override = driver_override; }else {kfree (driver_override); pdev->driver_override =NULL ; } +device_unlock (dev);kfree (old);
Shouldn't you move the lock until after the kfree()? Or am I missing what the lock is trying to protect here?
drivers/base/platform.c static ssize_t driver_override_show(struct device *dev, · line 906
Quoted 12 lines
drivers/base/platform.c +5
-1@@ -895,8 +898,12 @@ static ssize_t driver_override_show(struct device *dev, struct device_attribute *attr, char *buf) { struct platform_device *pdev = to_platform_device(dev); +ssize_t len; -return sprintf (buf,"%s\n" , pdev->driver_override); +device_lock (dev); + len =sprintf (buf,"%s\n" , pdev->driver_override); +device_unlock (dev); +return len;
Why does the show function need to be changed at all? How can anything "race" here?
thanks,
greg k-h
Quoted 38 lines
Quoted 38 lines
The
driver_overrideimplementation is susceptible to race condition when different threads are reading vs storing a different driver override. Add locking to avoid race condition.Fixes 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")Signed-off-by Adrian Salido <[email protected]>--- drivers/base/platform.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/base/platform.c +4
-1@@ -866,7 +866,7 @@ static ssize_t driver_override_store(struct device *dev, const char *buf, size_t count) {struct platform_device *pdev =to_platform_device (dev); -char *driver_override, *old = pdev->driver_override, *cp; +char *driver_override, *old, *cp;if (count >PATH_MAX )return -EINVAL ;@@ -879,12 +879,15 @@ static ssize_t driver_override_store(struct device *dev,if (cp) *cp ='\0' ; +device_lock (dev); + old = pdev->driver_override;if (strlen (driver_override)) { pdev->driver_override = driver_override; }else {kfree (driver_override); pdev->driver_override =NULL ; } +device_unlock (dev);
Quoted 4 lines
drivers/base/platform.c static ssize_t driver_override_store(struct device *dev, · line 892
Quoted 1 line
drivers/base/platform.c +0
-0kfree (old);Shouldn't you move the lock until after the
kfree()? Or am I missing what the lock is trying to protect here?
not really, the lock only protecting the variable pdev->driver_override. Once the value has changed we no longer care about "old" variable
Quoted 22 lines
Quoted 22 lines
drivers/base/platform.c +0
-0if (cp)*cp = '\0';drivers/base/platform.c +4
-0+device_lock (dev); + old = pdev->driver_override;if (strlen (driver_override)) { pdev->driver_override = driver_override; }else {kfree (driver_override); pdev->driver_override =NULL ; } +device_unlock (dev);kfree (old);@@ -895,8 +898,12 @@ static ssize_t driver_override_show(struct device *dev, struct device_attribute *attr, char *buf) { struct platform_device *pdev = to_platform_device(dev); +ssize_t len;
Quoted 8 lines
drivers/base/platform.c static ssize_t driver_override_show(struct device *dev, · line 906
Quoted 5 lines
drivers/base/platform.c +4
-1-return sprintf (buf,"%s\n" , pdev->driver_override); +device_lock (dev); + len =sprintf (buf,"%s\n" , pdev->driver_override); +device_unlock (dev); +return len;Why does the show function need to be changed at all? How can anything "race" here?
The lock is trying to protect again race between store and show. Suppose there are 2 threads:
Thread1:
while (1) {
driver_override_store("foo");
driver_override_store("");
}
Thread2:
while (1) driver_override_show();
Thread 1 | Thread 2
----------------------------------------|-----------------------
old = pdev->driver_override; |
| len = sprintf(buf,
"%s\n", pdev->driver_override);
| /* snprintf starts reading */pdev->driver_override = |
driver_override; |
kfree(old); | /* use after free before
snprintf finishes execution */
Similarly there could be a race between multiple threads doing store where memory leaks could happen
Thanks,
Adrian
Greg KH
maintainerQuoted 38 lines · Adrian Salido
Quoted 38 lines
Quoted 38 lines
The
driver_overrideimplementation is susceptible to race condition when different threads are reading vs storing a different driver override. Add locking to avoid race condition.Fixes 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")Signed-off-by Adrian Salido <[email protected]>--- drivers/base/platform.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/base/platform.c +4
-1@@ -866,7 +866,7 @@ static ssize_t driver_override_store(struct device *dev, const char *buf, size_t count) {struct platform_device *pdev =to_platform_device (dev); -char *driver_override, *old = pdev->driver_override, *cp; +char *driver_override, *old, *cp;if (count >PATH_MAX )return -EINVAL ;@@ -879,12 +879,15 @@ static ssize_t driver_override_store(struct device *dev,if (cp) *cp ='\0' ; +device_lock (dev); + old = pdev->driver_override;if (strlen (driver_override)) { pdev->driver_override = driver_override; }else {kfree (driver_override); pdev->driver_override =NULL ; } +device_unlock (dev);
Quoted 8 lines
Quoted 4 lines
drivers/base/platform.c static ssize_t driver_override_store(struct device *dev, · line 892
Quoted 1 line
drivers/base/platform.c +0
-0kfree (old);Shouldn't you move the lock until after the
kfree()? Or am I missing what the lock is trying to protect here?not really, the lock only protecting the variable
pdev->driver_override. Once the value has changed we no longer care about "old" variable
What are you protecting it from? Being overwritten twice? Or something else?
Quoted 54 lines
Quoted 22 lines
Quoted 22 lines
drivers/base/platform.c +0
-0if (cp)*cp = '\0';drivers/base/platform.c +4
-0+device_lock (dev); + old = pdev->driver_override;if (strlen (driver_override)) { pdev->driver_override = driver_override; }else {kfree (driver_override); pdev->driver_override =NULL ; } +device_unlock (dev);kfree (old);@@ -895,8 +898,12 @@ static ssize_t driver_override_show(struct device *dev, struct device_attribute *attr, char *buf) { struct platform_device *pdev = to_platform_device(dev); +ssize_t len;Quoted 8 lines
drivers/base/platform.c static ssize_t driver_override_show(struct device *dev, · line 906
Quoted 5 lines
drivers/base/platform.c +4
-1-return sprintf (buf,"%s\n" , pdev->driver_override); +device_lock (dev); + len =sprintf (buf,"%s\n" , pdev->driver_override); +device_unlock (dev); +return len;Why does the show function need to be changed at all? How can anything "race" here?
The lock is trying to protect again race between store and show. Suppose there are 2 threads:
Thread1:
while (1) { driver_override_store("foo"); driver_override_store("");}
Thread2:
while (1)driver_override_show();Thread 1 | Thread 2
----------------------------------------|-----------------------
old =pdev->driver_override; |
| len = sprintf(buf,
"%s\n",pdev->driver_override);
| /* snprintf starts reading */pdev->driver_override= |
driver_override; |
kfree(old); | /* use after free before
snprintf finishes execution */
Quoted 2 lines
Similarly there could be a race between multiple threads doing store where memory leaks could happen
Ah, the printing of the string, that makes more sense, thanks, I was thinking of the assignment of the pointer itself, which is atomic on all sane platforms.
As writing this is only allowed by root, it's not really a big deal, I'll queue it up for the next release.
thanks,
greg k-h