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 <j...@perches.com>
> > > ---
> > > 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
