Re: [PATCH] driver core: Use scnprintf() instead of snprintf() in device show functions

[Greg Kroah-Hartman]

On Sat, Jun 20, 2020 at 06:05:29PM +0300, Denis Efremov wrote:
> >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().
> 
> The reason is that snprintf() returns the length the resulting string
> would be, assuming the data will fit into the destination array.
> scnprintf() returns the length of the string actually created in buf.
> 
> device_show_{ulong,int,bool}() functions are reference implementation
> and should respect the kernel documentation.

Ok, but you are not following that documentation below, why not?

> 
> Signed-off-by: Denis Efremov 
> ---
> 
> Actually, I don't know why snprintf() is used here initially. It looks
> safe to use sprintf().
> 
>  drivers/base/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 67d39a90b45c..2815deb511fc 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1472,7 +1472,7 @@ ssize_t device_show_ulong(struct device *dev,
> char *buf)
>  {
>   struct dev_ext_attribute *ea = to_ext_attr(attr);
> - return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
> + return scnprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));

As the documentation states, these should just be sprintf(), if you
really want to change these.

As-is, these are also ok, there is no need to change.

thanks,

greg k-h

[PATCH] driver core: Use scnprintf() instead of snprintf() in device show functions

[Denis Efremov]

>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().

The reason is that snprintf() returns the length the resulting string
would be, assuming the data will fit into the destination array.
scnprintf() returns the length of the string actually created in buf.

device_show_{ulong,int,bool}() functions are reference implementation
and should respect the kernel documentation.

Signed-off-by: Denis Efremov 
---

Actually, I don't know why snprintf() is used here initially. It looks
safe to use sprintf().

 drivers/base/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..2815deb511fc 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1472,7 +1472,7 @@ ssize_t device_show_ulong(struct device *dev,
  char *buf)
 {
struct dev_ext_attribute *ea = to_ext_attr(attr);
-   return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(device_show_ulong);
 
@@ -1502,7 +1502,7 @@ ssize_t device_show_int(struct device *dev,
 {
struct dev_ext_attribute *ea = to_ext_attr(attr);
 
-   return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(device_show_int);
 
@@ -1523,7 +1523,7 @@ ssize_t device_show_bool(struct device *dev, struct 
device_attribute *attr,
 {
struct dev_ext_attribute *ea = to_ext_attr(attr);
 
-   return snprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(device_show_bool);
 
-- 
2.26.2