Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Greg Kroah-Hartman]

On Fri, Aug 28, 2020 at 11:41:00PM -0700, Joe Perches wrote:
> On Sat, 2020-08-29 at 08:22 +0200, Greg Kroah-Hartman wrote:
> > On Fri, Aug 28, 2020 at 03:52:13PM -0700, Joe Perches wrote:
> > > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > > used for outputting sysfs content requests and it's possible to
> > > overrun the buffer length.
> > > 
> > > Add a generic sysfs_emit mechanism that knows that the size of the
> > > temporary buffer and ensures that no overrun is done.
> > > 
> > > Signed-off-by: Joe Perches <joe@xxxxxxxxxxx>
> > > ---
> > >  fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
> > >  include/linux/sysfs.h |  8 ++++++++
> > >  2 files changed, 38 insertions(+)
> > > 
> > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > index eb6897ab78e7..06a13bbd7080 100644
> > > --- a/fs/sysfs/file.c
> > > +++ b/fs/sysfs/file.c
> > > @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(sysfs_change_owner);
> > > +
> > > +/**
> > > + *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> > > + *	@buf:	start of PAGE_SIZE buffer.
> > > + *	@pos:	current position in buffer
> > > + *              (pos - buf) must always be < PAGE_SIZE
> > 
> > sysfs files are always supposed to be "one value per file", so why would
> > you ever need a 'pos' variable to show the location in the buffer?
> 
> I've done treewide conversions using cocci.
> It's used all over the place.
> Especially in loops with arrays.
> 
> Sometimes the output is single line.
> Sometimes multiple lines.
> 
> Look at the sample conversion of mem_sleep_show I posted earlier.
> 
> #ifdef CONFIG_SUSPEND
>  static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>                               char *buf)
>  {
> -       char *s = buf;
> +       char *pos = buf;
>         suspend_state_t i;
>  
>         for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
>                 if (mem_sleep_states[i]) {
>                         const char *label = mem_sleep_states[i];
>  
>                         if (mem_sleep_current == i)
> -                               s += sprintf(s, "[%s] ", label);
> +                               pos += sysfs_emit(buf, pos, "[%s] ", label);
>                         else
> -                               s += sprintf(s, "%s ", label);
> +                               pos += sysfs_emit(buf, pos, "%s ", label);
>                 }
>  
>         /* Convert the last space to a newline if needed. */
> -       if (s != buf)
> -               *(s-1) = '\n';
> +       if (pos != buf)
> +               *(pos - 1) = '\n';
>  
> -       return (s - buf);
> +       return pos - buf;
>  }

And again, this is the rare exception, not the rule, please do not make
a generic helper function "easy" to do crazy things like this in sysfs.

Heck, make it explicit, call this function sysfs_emit_pos() and the
non-pos version sysfs_emit().  That way I can easily search for the
"offending" users of the sysfs api.

thanks,

greg k-h