[PATCH 0/3] make PCI's and platform's driver_override_store()/show() converge

[PATCH 0/3] make PCI's and platform's driver_override_store()/show() converge

[Nicolai Stange]

Hi,

both, drivers/pci/pci-sysfs.c and drivers/base/platform.c implement
a driver_override_store/show() pair which used to coincide between these
two subsystems (up to ->driver_override's storage location).

Now, both have received a fix in the meanwhile which the other subsystem
lacks each:
  commit 6265539776a0 ("driver core: platform: fix race condition with
                        driver_override")
and
  commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
                       "driver_override" buffer")

Port the missing fix to the other subsystem each.

There's also the style fixup [2/3] which isn't strictly needed but
will make both driver_override_show() variants look the same again.

Applies to next-20170908.

Note that checkpatch complains about commit references to the PCI commit
mentioned above, which is, as I believe, a false positive due to the
quotation marks in the title.

Thanks,

Nicolai

Nicolai Stange (3):
  PCI: fix race condition with driver_override
  PCI: don't use snprintf() in driver_override_show()
  driver core: platform: Don't read past the end of "driver_override"
    buffer

 drivers/base/platform.c |  3 ++-
 drivers/pci/pci-sysfs.c | 11 +++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.13.5

[PATCH 1/3] PCI: fix race condition with driver_override

[Nicolai Stange]

The driver_override implementation is susceptible to a race condition when
different threads are reading vs. storing a different driver override.
Add locking to avoid the race condition.

This is in close analogy to commit 6265539776a0 ("driver core: platform:
fix race condition with driver_override") from Adrian Salido.

Fixes: 782a985d7af2 ("PCI: Introduce new device binding path using pci_dev.driver_override")
Cc: stable@vger.kernel.org	# v3.16+
Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 drivers/pci/pci-sysfs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 1eecfa301f7f..8e075ea2743e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -686,7 +686,7 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	char *driver_override, *old = pdev->driver_override, *cp;
+	char *driver_override, *old, *cp;
 
 	/* We need to keep extra room for a newline */
 	if (count >= (PAGE_SIZE - 1))
@@ -700,12 +700,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);
 
@@ -716,8 +719,12 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	ssize_t len;
 
-	return snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
+	device_lock(dev);
+	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
+	device_unlock(dev);
+	return len;
 }
 static DEVICE_ATTR_RW(driver_override);
 
-- 
2.13.5

[PATCH 2/3] PCI: don't use snprintf() in driver_override_show()

[Nicolai Stange]

Quote from Documentation/filesystems/sysfs.txt:

  show() must not use snprintf() when formatting the value to be
  returned to user space. If you can guarantee that an overflow
  will never happen you can use sprintf() otherwise you must use
  scnprintf().

Commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
"driver_override" buffer") introduced such a snprintf() usage from
driver_override_show() while at the same time tweaking
driver_override_store() such that the write buffer can't ever get
overflowed.

Reasoning:
Since aforementioned commit, driver_override_store() only accepts to be
written buffers less than PAGE_SIZE - 1 in size.

The then kstrndup()'ed driver_override string will be at most PAGE_SIZE - 1
in length, including the trailing '\0'.

After the addition of a '\n' in driver_override_show(), the result won't
exceed PAGE_SIZE characters in length, again including the trailing '\0'.

Hence, snprintf(buf, PAGE_SIZE, ...) and sprintf(buf, ...) are equivalent
at this point.

Replace the former by the latter in order to adhere to the rules in
Documentation/filesystems/sysfs.txt.

This is a style fix only and there's no change in functionality.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 drivers/pci/pci-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8e075ea2743e..43f7fbede448 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -722,7 +722,7 @@ static ssize_t driver_override_show(struct device *dev,
 	ssize_t len;
 
 	device_lock(dev);
-	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
+	len = sprintf(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
-- 
2.13.5

[PATCH 3/3] driver core: platform: Don't read past the end of "driver_override" buffer

[Nicolai Stange]

When printing the driver_override parameter when it is 4095 and 4094 bytes
long, the printing code would access invalid memory because we need count+1
bytes for printing.

Reject driver_override values of these lengths in driver_override_store().

This is in close analogy to commit 4efe874aace5 ("PCI: Don't read past the
end of sysfs "driver_override" buffer") from Sasha Levin.

Fixes: 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")
Cc: stable@vger.kernel.org	# v3.17+
Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 drivers/base/platform.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index d1bd99271066..9045c5f3734e 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -868,7 +868,8 @@ static ssize_t driver_override_store(struct device *dev,
 	struct platform_device *pdev = to_platform_device(dev);
 	char *driver_override, *old, *cp;
 
-	if (count > PATH_MAX)
+	/* We need to keep extra room for a newline */
+	if (count >= (PAGE_SIZE - 1))
 		return -EINVAL;
 
 	driver_override = kstrndup(buf, count, GFP_KERNEL);
-- 
2.13.5

Re: [PATCH 2/3] PCI: don't use snprintf() in driver_override_show()

[Greg Kroah-Hartman]

On Mon, Sep 11, 2017 at 09:45:41AM +0200, Nicolai Stange wrote:
> Quote from Documentation/filesystems/sysfs.txt:
> 
>   show() must not use snprintf() when formatting the value to be
>   returned to user space. If you can guarantee that an overflow
>   will never happen you can use sprintf() otherwise you must use
>   scnprintf().
> 
> Commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
> "driver_override" buffer") introduced such a snprintf() usage from
> driver_override_show() while at the same time tweaking
> driver_override_store() such that the write buffer can't ever get
> overflowed.
> 
> Reasoning:
> Since aforementioned commit, driver_override_store() only accepts to be
> written buffers less than PAGE_SIZE - 1 in size.
> 
> The then kstrndup()'ed driver_override string will be at most PAGE_SIZE - 1
> in length, including the trailing '\0'.
> 
> After the addition of a '\n' in driver_override_show(), the result won't
> exceed PAGE_SIZE characters in length, again including the trailing '\0'.
> 
> Hence, snprintf(buf, PAGE_SIZE, ...) and sprintf(buf, ...) are equivalent
> at this point.
> 
> Replace the former by the latter in order to adhere to the rules in
> Documentation/filesystems/sysfs.txt.
> 
> This is a style fix only and there's no change in functionality.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>  drivers/pci/pci-sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 8e075ea2743e..43f7fbede448 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -722,7 +722,7 @@ static ssize_t driver_override_show(struct device *dev,
>  	ssize_t len;
>  
>  	device_lock(dev);
> -	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
> +	len = sprintf(buf, "%s\n", pdev->driver_override);

While I'm all for changes like this, it's an uphill battle to change
them, usually it's best to just catch them before they go into the tree.

Anyway, nice summary, very good job with that.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/3] PCI: fix race condition with driver_override

[Bjorn Helgaas]

On Mon, Sep 11, 2017 at 09:45:40AM +0200, Nicolai Stange wrote:
> The driver_override implementation is susceptible to a race condition when
> different threads are reading vs. storing a different driver override.
> Add locking to avoid the race condition.
> 
> This is in close analogy to commit 6265539776a0 ("driver core: platform:
> fix race condition with driver_override") from Adrian Salido.
> 
> Fixes: 782a985d7af2 ("PCI: Introduce new device binding path using pci_dev.driver_override")
> Cc: stable@vger.kernel.org	# v3.16+
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Applied to for-linus for v4.14, thanks!

> ---
>  drivers/pci/pci-sysfs.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 1eecfa301f7f..8e075ea2743e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -686,7 +686,7 @@ static ssize_t driver_override_store(struct device *dev,
>  				     const char *buf, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	char *driver_override, *old = pdev->driver_override, *cp;
> +	char *driver_override, *old, *cp;
>  
>  	/* We need to keep extra room for a newline */
>  	if (count >= (PAGE_SIZE - 1))
> @@ -700,12 +700,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);
>  
> @@ -716,8 +719,12 @@ static ssize_t driver_override_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t len;
>  
> -	return snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
> +	device_lock(dev);
> +	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
> +	device_unlock(dev);
> +	return len;
>  }
>  static DEVICE_ATTR_RW(driver_override);
>  
> -- 
> 2.13.5
> 

Re: [PATCH 2/3] PCI: don't use snprintf() in driver_override_show()

[Bjorn Helgaas]

On Mon, Sep 11, 2017 at 04:55:11AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 11, 2017 at 09:45:41AM +0200, Nicolai Stange wrote:
> > Quote from Documentation/filesystems/sysfs.txt:
> > 
> >   show() must not use snprintf() when formatting the value to be
> >   returned to user space. If you can guarantee that an overflow
> >   will never happen you can use sprintf() otherwise you must use
> >   scnprintf().
> > 
> > Commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
> > "driver_override" buffer") introduced such a snprintf() usage from
> > driver_override_show() while at the same time tweaking
> > driver_override_store() such that the write buffer can't ever get
> > overflowed.
> > 
> > Reasoning:
> > Since aforementioned commit, driver_override_store() only accepts to be
> > written buffers less than PAGE_SIZE - 1 in size.
> > 
> > The then kstrndup()'ed driver_override string will be at most PAGE_SIZE - 1
> > in length, including the trailing '\0'.
> > 
> > After the addition of a '\n' in driver_override_show(), the result won't
> > exceed PAGE_SIZE characters in length, again including the trailing '\0'.
> > 
> > Hence, snprintf(buf, PAGE_SIZE, ...) and sprintf(buf, ...) are equivalent
> > at this point.
> > 
> > Replace the former by the latter in order to adhere to the rules in
> > Documentation/filesystems/sysfs.txt.
> > 
> > This is a style fix only and there's no change in functionality.
> > 
> > Signed-off-by: Nicolai Stange <nstange@suse.de>
> > ---
> >  drivers/pci/pci-sysfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 8e075ea2743e..43f7fbede448 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -722,7 +722,7 @@ static ssize_t driver_override_show(struct device *dev,
> >  	ssize_t len;
> >  
> >  	device_lock(dev);
> > -	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
> > +	len = sprintf(buf, "%s\n", pdev->driver_override);
> 
> While I'm all for changes like this, it's an uphill battle to change
> them, usually it's best to just catch them before they go into the tree.
> 
> Anyway, nice summary, very good job with that.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Why use snprintf() instead of scnprintf()?  It looks like snprintf()
is probably safe, but requires a bunch of analysis to prove that,
while scnprintf() would be obviously safe.

Bjorn

Re: [PATCH 3/3] driver core: platform: Don't read past the end of "driver_override" buffer

[Bjorn Helgaas]

Greg, I assume you'll deal with this one?  Just let me know if I
should do something with it.

On Mon, Sep 11, 2017 at 09:45:42AM +0200, Nicolai Stange wrote:
> When printing the driver_override parameter when it is 4095 and 4094 bytes
> long, the printing code would access invalid memory because we need count+1
> bytes for printing.
> 
> Reject driver_override values of these lengths in driver_override_store().
> 
> This is in close analogy to commit 4efe874aace5 ("PCI: Don't read past the
> end of sysfs "driver_override" buffer") from Sasha Levin.
> 
> Fixes: 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")
> Cc: stable@vger.kernel.org	# v3.17+
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>  drivers/base/platform.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9045c5f3734e 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -868,7 +868,8 @@ static ssize_t driver_override_store(struct device *dev,
>  	struct platform_device *pdev = to_platform_device(dev);
>  	char *driver_override, *old, *cp;
>  
> -	if (count > PATH_MAX)
> +	/* We need to keep extra room for a newline */
> +	if (count >= (PAGE_SIZE - 1))
>  		return -EINVAL;
>  
>  	driver_override = kstrndup(buf, count, GFP_KERNEL);
> -- 
> 2.13.5
> 

Re: [PATCH 2/3] PCI: don't use snprintf() in driver_override_show()

[Nicolai Stange]

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Mon, Sep 11, 2017 at 04:55:11AM -0700, Greg Kroah-Hartman wrote:
>> On Mon, Sep 11, 2017 at 09:45:41AM +0200, Nicolai Stange wrote:
>> > Quote from Documentation/filesystems/sysfs.txt:
>> > 
>> >   show() must not use snprintf() when formatting the value to be
>> >   returned to user space. If you can guarantee that an overflow
>> >   will never happen you can use sprintf() otherwise you must use
>> >   scnprintf().
>> > 
>> > Commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
>> > "driver_override" buffer") introduced such a snprintf() usage from
>> > driver_override_show() while at the same time tweaking
>> > driver_override_store() such that the write buffer can't ever get
>> > overflowed.
>> > 
>> > Reasoning:
>> > Since aforementioned commit, driver_override_store() only accepts to be
>> > written buffers less than PAGE_SIZE - 1 in size.
>> > 
>> > The then kstrndup()'ed driver_override string will be at most PAGE_SIZE - 1
>> > in length, including the trailing '\0'.
>> > 
>> > After the addition of a '\n' in driver_override_show(), the result won't
>> > exceed PAGE_SIZE characters in length, again including the trailing '\0'.
>> > 
>> > Hence, snprintf(buf, PAGE_SIZE, ...) and sprintf(buf, ...) are equivalent
>> > at this point.
>> > 
>> > Replace the former by the latter in order to adhere to the rules in
>> > Documentation/filesystems/sysfs.txt.
>> > 
>> > This is a style fix only and there's no change in functionality.
>> > 
>> > Signed-off-by: Nicolai Stange <nstange@suse.de>
>> > ---
>> >  drivers/pci/pci-sysfs.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> > index 8e075ea2743e..43f7fbede448 100644
>> > --- a/drivers/pci/pci-sysfs.c
>> > +++ b/drivers/pci/pci-sysfs.c
>> > @@ -722,7 +722,7 @@ static ssize_t driver_override_show(struct device *dev,
>> >  	ssize_t len;
>> >  
>> >  	device_lock(dev);
>> > -	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
>> > +	len = sprintf(buf, "%s\n", pdev->driver_override);
>> 
>> While I'm all for changes like this, it's an uphill battle to change
>> them, usually it's best to just catch them before they go into the tree.

I did this patch mostly for demonstrating why the exclusion of the
sprintf() -> snprintf() change from the port of

  3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")

to platform in [3/3] is safe. Because I'm pretty sure that [3/3] would
have been rejected if it had included that chunk -- due to the
non-conformity to Documentation/.


>> Anyway, nice summary, very good job with that.
>> 
>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Why use snprintf() instead of scnprintf()?  

s/snprintf/sprintf/, right?


> It looks like snprintf() is probably safe, but requires a bunch of
> analysis to prove that, while scnprintf() would be obviously safe.

Personal preference, I guess. Something like "using sprintf() would
implicitly document that the buffer is always large enough", i.e. that
the size check in driver_override_store() is already strict enough / correct.

Anyway, I don't have a strong opinion on that. So if you want me to
change this, I'll do, of course. Just note that Greg K-H. has queued up
[3/3] somewhere already and thus, it would probably require two patches
rather than only this one to make PCI and platform look the same again,
provided that's what is wanted.

So, given that this patch has fulfilled its purpose already, I'm fine
with either of
- dropping it
- taking it
- s/sprintf/scnprintf/ and do the same for platform.


Thanks,

Nicolai

Re: [PATCH 2/3] PCI: don't use snprintf() in driver_override_show()

[Nicolai Stange]

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Mon, Sep 11, 2017 at 04:55:11AM -0700, Greg Kroah-Hartman wrote:
>> On Mon, Sep 11, 2017 at 09:45:41AM +0200, Nicolai Stange wrote:
>> > Quote from Documentation/filesystems/sysfs.txt:
>> > 
>> >   show() must not use snprintf() when formatting the value to be
>> >   returned to user space. If you can guarantee that an overflow
>> >   will never happen you can use sprintf() otherwise you must use
>> >   scnprintf().
>> > 
>> > Commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
>> > "driver_override" buffer") introduced such a snprintf() usage from
>> > driver_override_show() while at the same time tweaking
>> > driver_override_store() such that the write buffer can't ever get
>> > overflowed.
>> > 
>> > Reasoning:
>> > Since aforementioned commit, driver_override_store() only accepts to be
>> > written buffers less than PAGE_SIZE - 1 in size.
>> > 
>> > The then kstrndup()'ed driver_override string will be at most PAGE_SIZE - 1
>> > in length, including the trailing '\0'.
>> > 
>> > After the addition of a '\n' in driver_override_show(), the result won't
>> > exceed PAGE_SIZE characters in length, again including the trailing '\0'.
>> > 
>> > Hence, snprintf(buf, PAGE_SIZE, ...) and sprintf(buf, ...) are equivalent
>> > at this point.
>> > 
>> > Replace the former by the latter in order to adhere to the rules in
>> > Documentation/filesystems/sysfs.txt.
>> > 
>> > This is a style fix only and there's no change in functionality.
>> > 
>> > Signed-off-by: Nicolai Stange <nstange@suse.de>
>> > ---
>> >  drivers/pci/pci-sysfs.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> > index 8e075ea2743e..43f7fbede448 100644
>> > --- a/drivers/pci/pci-sysfs.c
>> > +++ b/drivers/pci/pci-sysfs.c
>> > @@ -722,7 +722,7 @@ static ssize_t driver_override_show(struct device *dev,
>> >  	ssize_t len;
>> >  
>> >  	device_lock(dev);
>> > -	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
>> > +	len = sprintf(buf, "%s\n", pdev->driver_override);
>> 
>> While I'm all for changes like this, it's an uphill battle to change
>> them, usually it's best to just catch them before they go into the tree.

I did this patch mostly for demonstrating why the exclusion of the
sprintf() -> snprintf() change from the port of

  3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")

to platform in [3/3] is safe. Because I'm pretty sure that [3/3] would
have been rejected if it had included that chunk -- due to the
non-conformity to Documentation/.


>> Anyway, nice summary, very good job with that.
>> 
>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Why use snprintf() instead of scnprintf()?  

s/snprintf/sprintf/, right?


> It looks like snprintf() is probably safe, but requires a bunch of
> analysis to prove that, while scnprintf() would be obviously safe.

Personal preference, I guess. Something like "using sprintf() would
implicitly document that the buffer is always large enough", i.e. that
the size check in driver_override_store() is already strict enough / correct.

Anyway, I don't have a strong opinion on that. So if you want me to
change this, I'll do, of course. Just note that Greg K-H. has queued up
[3/3] somewhere already and thus, it would probably require two patches
rather than only this one to make PCI and platform look the same again,
provided that's what is wanted.

So, given that this patch has fulfilled its purpose already, I'm fine
with either of
- dropping it
- taking it
- s/sprintf/scnprintf/ and do the same for platform.


Thanks,

Nicolai

Re: [PATCH 3/3] driver core: platform: Don't read past the end of "driver_override" buffer

[Greg Kroah-Hartman]

On Mon, Sep 25, 2017 at 06:50:00PM -0500, Bjorn Helgaas wrote:
> Greg, I assume you'll deal with this one?  Just let me know if I
> should do something with it.

I've already applied it to my driver-core-next tree, thanks.

greg k-h

Re: [PATCH 2/3] PCI: don't use snprintf() in driver_override_show()

[Bjorn Helgaas]

On Tue, Sep 26, 2017 at 08:36:11AM +0200, Nicolai Stange wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> 
> > On Mon, Sep 11, 2017 at 04:55:11AM -0700, Greg Kroah-Hartman wrote:
> >> On Mon, Sep 11, 2017 at 09:45:41AM +0200, Nicolai Stange wrote:
> >> > Quote from Documentation/filesystems/sysfs.txt:
> >> > 
> >> >   show() must not use snprintf() when formatting the value to be
> >> >   returned to user space. If you can guarantee that an overflow
> >> >   will never happen you can use sprintf() otherwise you must use
> >> >   scnprintf().
> >> > 
> >> > Commit 4efe874aace5 ("PCI: Don't read past the end of sysfs
> >> > "driver_override" buffer") introduced such a snprintf() usage from
> >> > driver_override_show() while at the same time tweaking
> >> > driver_override_store() such that the write buffer can't ever get
> >> > overflowed.
> >> > 
> >> > Reasoning:
> >> > Since aforementioned commit, driver_override_store() only accepts to be
> >> > written buffers less than PAGE_SIZE - 1 in size.
> >> > 
> >> > The then kstrndup()'ed driver_override string will be at most PAGE_SIZE - 1
> >> > in length, including the trailing '\0'.
> >> > 
> >> > After the addition of a '\n' in driver_override_show(), the result won't
> >> > exceed PAGE_SIZE characters in length, again including the trailing '\0'.
> >> > 
> >> > Hence, snprintf(buf, PAGE_SIZE, ...) and sprintf(buf, ...) are equivalent
> >> > at this point.
> >> > 
> >> > Replace the former by the latter in order to adhere to the rules in
> >> > Documentation/filesystems/sysfs.txt.
> >> > 
> >> > This is a style fix only and there's no change in functionality.
> >> > 
> >> > Signed-off-by: Nicolai Stange <nstange@suse.de>
> >> > ---
> >> >  drivers/pci/pci-sysfs.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > 
> >> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> > index 8e075ea2743e..43f7fbede448 100644
> >> > --- a/drivers/pci/pci-sysfs.c
> >> > +++ b/drivers/pci/pci-sysfs.c
> >> > @@ -722,7 +722,7 @@ static ssize_t driver_override_show(struct device *dev,
> >> >  	ssize_t len;
> >> >  
> >> >  	device_lock(dev);
> >> > -	len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->driver_override);
> >> > +	len = sprintf(buf, "%s\n", pdev->driver_override);
> >> 
> >> While I'm all for changes like this, it's an uphill battle to change
> >> them, usually it's best to just catch them before they go into the tree.
> 
> I did this patch mostly for demonstrating why the exclusion of the
> sprintf() -> snprintf() change from the port of
> 
>   3d713e0e382e ("driver core: platform: add device binding path 'driver_override'")
> 
> to platform in [3/3] is safe. Because I'm pretty sure that [3/3] would
> have been rejected if it had included that chunk -- due to the
> non-conformity to Documentation/.
> 
> 
> >> Anyway, nice summary, very good job with that.
> >> 
> >> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Why use snprintf() instead of scnprintf()?  
> 
> s/snprintf/sprintf/, right?

Yep, sorry!  I meant "why use sprintf() instead of scnprintf()?"

> > It looks like snprintf() is probably safe, but requires a bunch of
> > analysis to prove that, while scnprintf() would be obviously safe.
> 
> Personal preference, I guess. Something like "using sprintf() would
> implicitly document that the buffer is always large enough", i.e. that
> the size check in driver_override_store() is already strict enough / correct.
> 
> Anyway, I don't have a strong opinion on that. So if you want me to
> change this, I'll do, of course. Just note that Greg K-H. has queued up
> [3/3] somewhere already and thus, it would probably require two patches
> rather than only this one to make PCI and platform look the same again,
> provided that's what is wanted.
> 
> So, given that this patch has fulfilled its purpose already, I'm fine
> with either of
> - dropping it
> - taking it
> - s/sprintf/scnprintf/ and do the same for platform.

OK, I think I'll drop it.

I think the documentation is misleading.  The problem with
snprintf() is not that it might overflow the buffer; the problem is
that it might return more characters than it actually put in the
buffer.

I would be happy to see patches that change driver_override_show()
(there are three of them) to use scnprintf() instead of sprintf() or
snprintf().

Is the third case (the amba driver_override store/show functions)
susceptible to the same issues you're fixing for platform and PCI?

Bjorn