Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> > On 27/08/2020 15.18, Alex Dewar wrote:
> > > On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> > > > > On 25/08/2020 00.23, Alex Dewar wrote:
> > > > > > kernel/cpu.c: don't use snprintf() for sysfs attrs
> > > > > > 
> > > > > > As per the documentation (Documentation/filesystems/sysfs.rst),
> > > > > > snprintf() should not be used for formatting values returned by sysfs.
> > > > > > 
> > > > > 
> > > > > Can we have a sysfs_sprintf() (could just be a macro that does sprintf)
> > > > > to make it clear to the next reader that we know we're in a sysfs show
> > > > > method? It would make auditing uses of sprintf() much easier.
> > > > 
> > > > Code churn to keep code checkers quiet for pointless reasons?  What
> > > > could go wrong with that...
> > 
> > I did not (mean to) suggest replacing existing sprintf() calls in sysfs
> > show methods. But when changes _are_ being made, such as when replacing
> > snprintf() calls for whatever reasons, can we please not make it harder
> > for people doing manual audits (those are "code checkers" as well, I
> > suppose, but they do tend to only make noise when finding something).
> > 
> > > > It should be pretty obvious to any reader that you are in a sysfs show
> > > > method, as almost all of them are trivially tiny and obvious.
> > 
> > git grep doesn't immediately show that, not even with a suitable -C
> > argument, as you can't really know the potential callers unless you open
> > the file and see that the function is only assigned as a .show method.
> > And even that can be a pain because it's all hidden behind five levels
> > of magic macros that build identifiers with ##.
> > 
> > > Perhaps I should have mentioned this in the commit message, but the problem
> > > is that snprintf() doesn't return the number of bytes written to the
> > > destination buffer,
> > 
> > I'm perfectly well aware of that, TYVM (you may want to 'git log
> > --author Villemoes lib/vsprintf.c').
> > 
> >  but the number of bytes that *would have been written if
> > > they fitted*, which may be more than the bounds specified [1]. So "return
> > > snprintf(...)" for sysfs attributes is an antipattern. If you need bounded
> > > string ops, scnprintf() is the way to go. Using snprintf() can give a
> > > false sense of security, because it isn't necessarily safe.
> > 
> > Huh? This all seems utterly irrelevant WRT a change that replaces
> > PAGE_SIZE by INT_MAX (because that's what sprintf() is going to pretend
> > you passed). You get the same return value.
> > 
> > But I'm not at all concerned about whether one passes the proper buffer
> > size or not in sysfs show methods; with my embedded hat on, I'm all for
> > saving a few bytes of .text here and there. The problem, as far as I'm
> > concerned, is merely that adding sprintf() callers makes it harder to
> > find the problematic sprintf() instances.
> > 
> 
> Apologies, I think I might have expressed myself poorly, being a kernel noob
> ;-). I know that this is a stylistic change rather than a functional
> one -- I meant that I was hoping that it would be helpful to get rid of bad
> uses of snprintf().
> 
> I really like your idea of helper methods though :-). If in show()
> methods we could have something like:
> 	return sysfs_itoa(buf, i);
> in place of:
> 	return sprintf(buf, "%d\n", i);
> 
> ... then we wouldn't be introducing any new calls to sprintf() as you
> say, but we'd still be removing a call to snprintf() (which also may be
> problematic). Plus we'd have type checking on the argument.
> 
> For returning strings, we could have a bounded and unbounded variant of
> the function. As it seems like only single values should be returned via
> sysfs, if we did things this way then it would only be these
> string-returning functions which could cause buffer overflow problems
> and kernel devs could focus their attention accordingly...
> 
> What do people think? I'm happy to have a crack, provided this is
> actually a sensible thing to do! I'm looking for a newbie-level project
> to get started with.

Not a bad idea.

Coccinelle should be able to transform the various .show
methods to something sysfs_ prefixed in a fairly automated
way.




_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Julia Lawall]

On Thu, 27 Aug 2020, Joe Perches wrote:

> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> > On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> > > On 27/08/2020 15.18, Alex Dewar wrote:
> > > > On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> > > > > > On 25/08/2020 00.23, Alex Dewar wrote:
> > > > > > > kernel/cpu.c: don't use snprintf() for sysfs attrs
> > > > > > >
> > > > > > > As per the documentation (Documentation/filesystems/sysfs.rst),
> > > > > > > snprintf() should not be used for formatting values returned by sysfs.
> > > > > > >
> > > > > >
> > > > > > Can we have a sysfs_sprintf() (could just be a macro that does sprintf)
> > > > > > to make it clear to the next reader that we know we're in a sysfs show
> > > > > > method? It would make auditing uses of sprintf() much easier.
> > > > >
> > > > > Code churn to keep code checkers quiet for pointless reasons?  What
> > > > > could go wrong with that...
> > >
> > > I did not (mean to) suggest replacing existing sprintf() calls in sysfs
> > > show methods. But when changes _are_ being made, such as when replacing
> > > snprintf() calls for whatever reasons, can we please not make it harder
> > > for people doing manual audits (those are "code checkers" as well, I
> > > suppose, but they do tend to only make noise when finding something).
> > >
> > > > > It should be pretty obvious to any reader that you are in a sysfs show
> > > > > method, as almost all of them are trivially tiny and obvious.
> > >
> > > git grep doesn't immediately show that, not even with a suitable -C
> > > argument, as you can't really know the potential callers unless you open
> > > the file and see that the function is only assigned as a .show method.
> > > And even that can be a pain because it's all hidden behind five levels
> > > of magic macros that build identifiers with ##.
> > >
> > > > Perhaps I should have mentioned this in the commit message, but the problem
> > > > is that snprintf() doesn't return the number of bytes written to the
> > > > destination buffer,
> > >
> > > I'm perfectly well aware of that, TYVM (you may want to 'git log
> > > --author Villemoes lib/vsprintf.c').
> > >
> > >  but the number of bytes that *would have been written if
> > > > they fitted*, which may be more than the bounds specified [1]. So "return
> > > > snprintf(...)" for sysfs attributes is an antipattern. If you need bounded
> > > > string ops, scnprintf() is the way to go. Using snprintf() can give a
> > > > false sense of security, because it isn't necessarily safe.
> > >
> > > Huh? This all seems utterly irrelevant WRT a change that replaces
> > > PAGE_SIZE by INT_MAX (because that's what sprintf() is going to pretend
> > > you passed). You get the same return value.
> > >
> > > But I'm not at all concerned about whether one passes the proper buffer
> > > size or not in sysfs show methods; with my embedded hat on, I'm all for
> > > saving a few bytes of .text here and there. The problem, as far as I'm
> > > concerned, is merely that adding sprintf() callers makes it harder to
> > > find the problematic sprintf() instances.
> > >
> >
> > Apologies, I think I might have expressed myself poorly, being a kernel noob
> > ;-). I know that this is a stylistic change rather than a functional
> > one -- I meant that I was hoping that it would be helpful to get rid of bad
> > uses of snprintf().
> >
> > I really like your idea of helper methods though :-). If in show()
> > methods we could have something like:
> > 	return sysfs_itoa(buf, i);
> > in place of:
> > 	return sprintf(buf, "%d\n", i);
> >
> > ... then we wouldn't be introducing any new calls to sprintf() as you
> > say, but we'd still be removing a call to snprintf() (which also may be
> > problematic). Plus we'd have type checking on the argument.
> >
> > For returning strings, we could have a bounded and unbounded variant of
> > the function. As it seems like only single values should be returned via
> > sysfs, if we did things this way then it would only be these
> > string-returning functions which could cause buffer overflow problems
> > and kernel devs could focus their attention accordingly...
> >
> > What do people think? I'm happy to have a crack, provided this is
> > actually a sensible thing to do! I'm looking for a newbie-level project
> > to get started with.
>
> Not a bad idea.
>
> Coccinelle should be able to transform the various .show
> methods to something sysfs_ prefixed in a fairly automated
> way.

Something like

identifier f;
fresh identifier = "sysfs" ## f;

may be useful.  Let me know if further help is needed.

julia



>
>
>
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 21:42 +0200, Julia Lawall wrote:
> 
> On Thu, 27 Aug 2020, Joe Perches wrote:
> 
> > On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> > > On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> > > > On 27/08/2020 15.18, Alex Dewar wrote:
> > > > > On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> > > > > > > On 25/08/2020 00.23, Alex Dewar wrote:
> > > > > > > > kernel/cpu.c: don't use snprintf() for sysfs attrs
> > > > > > > > 
> > > > > > > > As per the documentation (Documentation/filesystems/sysfs.rst),
> > > > > > > > snprintf() should not be used for formatting values returned by sysfs.
> > > > > > > > 
> > > > > > > 
> > > > > > > Can we have a sysfs_sprintf() (could just be a macro that does sprintf)
> > > > > > > to make it clear to the next reader that we know we're in a sysfs show
> > > > > > > method? It would make auditing uses of sprintf() much easier.
> > > > > > 
> > > > > > Code churn to keep code checkers quiet for pointless reasons?  What
> > > > > > could go wrong with that...
> > > > 
> > > > I did not (mean to) suggest replacing existing sprintf() calls in sysfs
> > > > show methods. But when changes _are_ being made, such as when replacing
> > > > snprintf() calls for whatever reasons, can we please not make it harder
> > > > for people doing manual audits (those are "code checkers" as well, I
> > > > suppose, but they do tend to only make noise when finding something).
> > > > 
> > > > > > It should be pretty obvious to any reader that you are in a sysfs show
> > > > > > method, as almost all of them are trivially tiny and obvious.
> > > > 
> > > > git grep doesn't immediately show that, not even with a suitable -C
> > > > argument, as you can't really know the potential callers unless you open
> > > > the file and see that the function is only assigned as a .show method.
> > > > And even that can be a pain because it's all hidden behind five levels
> > > > of magic macros that build identifiers with ##.
> > > > 
> > > > > Perhaps I should have mentioned this in the commit message, but the problem
> > > > > is that snprintf() doesn't return the number of bytes written to the
> > > > > destination buffer,
> > > > 
> > > > I'm perfectly well aware of that, TYVM (you may want to 'git log
> > > > --author Villemoes lib/vsprintf.c').
> > > > 
> > > >  but the number of bytes that *would have been written if
> > > > > they fitted*, which may be more than the bounds specified [1]. So "return
> > > > > snprintf(...)" for sysfs attributes is an antipattern. If you need bounded
> > > > > string ops, scnprintf() is the way to go. Using snprintf() can give a
> > > > > false sense of security, because it isn't necessarily safe.
> > > > 
> > > > Huh? This all seems utterly irrelevant WRT a change that replaces
> > > > PAGE_SIZE by INT_MAX (because that's what sprintf() is going to pretend
> > > > you passed). You get the same return value.
> > > > 
> > > > But I'm not at all concerned about whether one passes the proper buffer
> > > > size or not in sysfs show methods; with my embedded hat on, I'm all for
> > > > saving a few bytes of .text here and there. The problem, as far as I'm
> > > > concerned, is merely that adding sprintf() callers makes it harder to
> > > > find the problematic sprintf() instances.
> > > > 
> > > 
> > > Apologies, I think I might have expressed myself poorly, being a kernel noob
> > > ;-). I know that this is a stylistic change rather than a functional
> > > one -- I meant that I was hoping that it would be helpful to get rid of bad
> > > uses of snprintf().
> > > 
> > > I really like your idea of helper methods though :-). If in show()
> > > methods we could have something like:
> > > 	return sysfs_itoa(buf, i);
> > > in place of:
> > > 	return sprintf(buf, "%d\n", i);
> > > 
> > > ... then we wouldn't be introducing any new calls to sprintf() as you
> > > say, but we'd still be removing a call to snprintf() (which also may be
> > > problematic). Plus we'd have type checking on the argument.
> > > 
> > > For returning strings, we could have a bounded and unbounded variant of
> > > the function. As it seems like only single values should be returned via
> > > sysfs, if we did things this way then it would only be these
> > > string-returning functions which could cause buffer overflow problems
> > > and kernel devs could focus their attention accordingly...
> > > 
> > > What do people think? I'm happy to have a crack, provided this is
> > > actually a sensible thing to do! I'm looking for a newbie-level project
> > > to get started with.
> > 
> > Not a bad idea.
> > 
> > Coccinelle should be able to transform the various .show
> > methods to something sysfs_ prefixed in a fairly automated
> > way.
> 
> Something like
> 
> identifier f;
> fresh identifier = "sysfs" ## f;
> 
> may be useful.  Let me know if further help is needed.

Perhaps it's a bit more complicated.

Perhaps what's necessary is to find any
appropriate .show function and change
any use of strcpy/sprintf within those
function to some other name.

For instance:

drivers/isdn/mISDN/core.c-static ssize_t name_show(struct device *dev,
drivers/isdn/mISDN/core.c-                       struct device_attribute *attr, char *buf)
drivers/isdn/mISDN/core.c-{
drivers/isdn/mISDN/core.c:      strcpy(buf, dev_name(dev));
drivers/isdn/mISDN/core.c-      return strlen(buf);
drivers/isdn/mISDN/core.c-}
drivers/isdn/mISDN/core.c-static DEVICE_ATTR_RO(name);

and macroized uses like:

drivers/base/node.c-#define CACHE_ATTR(name, fmt)                                               \
drivers/base/node.c-static ssize_t name##_show(struct device *dev,                              \
drivers/base/node.c-                       struct device_attribute *attr,               \
drivers/base/node.c-                       char *buf)                                   \
drivers/base/node.c-{                                                                   \
drivers/base/node.c-    return sprintf(buf, fmt "\n", to_cache_info(dev)->cache_attrs.name);\
drivers/base/node.c-}                                                                   \
drivers/base/node.c:DEVICE_ATTR_RO(name);
drivers/base/node.c-
drivers/base/node.c-CACHE_ATTR(size, "%llu")
drivers/base/node.c-CACHE_ATTR(line_size, "%u")
drivers/base/node.c-CACHE_ATTR(indexing, "%u")
drivers/base/node.c-CACHE_ATTR(write_policy, "%u")

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 13:29 -0700, Joe Perches wrote:
> On Thu, 2020-08-27 at 21:42 +0200, Julia Lawall wrote:
> > On Thu, 27 Aug 2020, Joe Perches wrote:
> > 
> > > On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> > > > On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> > > > > On 27/08/2020 15.18, Alex Dewar wrote:
> > > > > > On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> > > > > > > > On 25/08/2020 00.23, Alex Dewar wrote:
> > > > > > > > > kernel/cpu.c: don't use snprintf() for sysfs attrs
> > > > > > > > > 
> > > > > > > > > As per the documentation (Documentation/filesystems/sysfs.rst),
> > > > > > > > > snprintf() should not be used for formatting values returned by sysfs.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Can we have a sysfs_sprintf() (could just be a macro that does sprintf)
> > > > > > > > to make it clear to the next reader that we know we're in a sysfs show
> > > > > > > > method? It would make auditing uses of sprintf() much easier.
> > > > > > > 
> > > > > > > Code churn to keep code checkers quiet for pointless reasons?  What
> > > > > > > could go wrong with that...
> > > > > 
> > > > > I did not (mean to) suggest replacing existing sprintf() calls in sysfs
> > > > > show methods. But when changes _are_ being made, such as when replacing
> > > > > snprintf() calls for whatever reasons, can we please not make it harder
> > > > > for people doing manual audits (those are "code checkers" as well, I
> > > > > suppose, but they do tend to only make noise when finding something).
> > > > > 
> > > > > > > It should be pretty obvious to any reader that you are in a sysfs show
> > > > > > > method, as almost all of them are trivially tiny and obvious.
> > > > > 
> > > > > git grep doesn't immediately show that, not even with a suitable -C
> > > > > argument, as you can't really know the potential callers unless you open
> > > > > the file and see that the function is only assigned as a .show method.
> > > > > And even that can be a pain because it's all hidden behind five levels
> > > > > of magic macros that build identifiers with ##.
> > > > > 
> > > > > > Perhaps I should have mentioned this in the commit message, but the problem
> > > > > > is that snprintf() doesn't return the number of bytes written to the
> > > > > > destination buffer,
> > > > > 
> > > > > I'm perfectly well aware of that, TYVM (you may want to 'git log
> > > > > --author Villemoes lib/vsprintf.c').
> > > > > 
> > > > >  but the number of bytes that *would have been written if
> > > > > > they fitted*, which may be more than the bounds specified [1]. So "return
> > > > > > snprintf(...)" for sysfs attributes is an antipattern. If you need bounded
> > > > > > string ops, scnprintf() is the way to go. Using snprintf() can give a
> > > > > > false sense of security, because it isn't necessarily safe.
> > > > > 
> > > > > Huh? This all seems utterly irrelevant WRT a change that replaces
> > > > > PAGE_SIZE by INT_MAX (because that's what sprintf() is going to pretend
> > > > > you passed). You get the same return value.
> > > > > 
> > > > > But I'm not at all concerned about whether one passes the proper buffer
> > > > > size or not in sysfs show methods; with my embedded hat on, I'm all for
> > > > > saving a few bytes of .text here and there. The problem, as far as I'm
> > > > > concerned, is merely that adding sprintf() callers makes it harder to
> > > > > find the problematic sprintf() instances.
> > > > > 
> > > > 
> > > > Apologies, I think I might have expressed myself poorly, being a kernel noob
> > > > ;-). I know that this is a stylistic change rather than a functional
> > > > one -- I meant that I was hoping that it would be helpful to get rid of bad
> > > > uses of snprintf().
> > > > 
> > > > I really like your idea of helper methods though :-). If in show()
> > > > methods we could have something like:
> > > > 	return sysfs_itoa(buf, i);
> > > > in place of:
> > > > 	return sprintf(buf, "%d\n", i);
> > > > 
> > > > ... then we wouldn't be introducing any new calls to sprintf() as you
> > > > say, but we'd still be removing a call to snprintf() (which also may be
> > > > problematic). Plus we'd have type checking on the argument.
> > > > 
> > > > For returning strings, we could have a bounded and unbounded variant of
> > > > the function. As it seems like only single values should be returned via
> > > > sysfs, if we did things this way then it would only be these
> > > > string-returning functions which could cause buffer overflow problems
> > > > and kernel devs could focus their attention accordingly...
> > > > 
> > > > What do people think? I'm happy to have a crack, provided this is
> > > > actually a sensible thing to do! I'm looking for a newbie-level project
> > > > to get started with.
> > > 
> > > Not a bad idea.
> > > 
> > > Coccinelle should be able to transform the various .show
> > > methods to something sysfs_ prefixed in a fairly automated
> > > way.
> > 
> > Something like
> > 
> > identifier f;
> > fresh identifier = "sysfs" ## f;
> > 
> > may be useful.  Let me know if further help is needed.

cocci syntax eludes me, but I imagine something like:

@@
identifier f_show =~ "^.*_show$";
@@
ssize_t f_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
-	sprintf
+	kobj_sprintf
	(...);


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Denis Efremov]

Hi all,

On 8/27/20 10:42 PM, Julia Lawall wrote:
> 
> 
> On Thu, 27 Aug 2020, Joe Perches wrote:
> 
>> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
>>> On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
>>>> On 27/08/2020 15.18, Alex Dewar wrote:
>>>>> On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
>>>>>> On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
>>>>>>> On 25/08/2020 00.23, Alex Dewar wrote:
>>>>>>>> kernel/cpu.c: don't use snprintf() for sysfs attrs
>>>>>>>>
>>>>>>>> As per the documentation (Documentation/filesystems/sysfs.rst),
>>>>>>>> snprintf() should not be used for formatting values returned by sysfs.

Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would
like it more than changing snprintf to scnprintf. As for me, I don't like the idea
of automated altering of the original logic from bounded snprint to unbouded one
with sprintf.

[1] https://lkml.org/lkml/2020/8/13/786

Regarding current device_attr_show.cocci implementation, it detects the functions
by declaration:
ssize_t any_name(struct device *dev, struct device_attribute *attr, char *buf)

and I limited the check to:
"return snprintf"
pattern because there are already too many warnings.

Actually, it looks more correct to check for:
ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
{
        <...
*       snprintf@p(...);
        ...>
}

This pattern should also highlight the snprintf calls there we save returned
value in a var, e.g.:

ret += snprintf(...);
...
ret += snprintf(...);
...
ret += snprintf(...);

return ret;

> 
> Something like
> 
> identifier f;
> fresh identifier = "sysfs" ## f;
> 
> may be useful.  Let me know if further help is needed.

Initially, I wrote the rule to search for DEVICE_ATTR(..., ..., func_name, ...)
functions. However, it looks like matching function prototype is enough. At least,
I failed to find false positives. I rejected the initial DEVICE_ATTR() searching
because I thought that it's impossible to handle DEVICE_ATTR_RO()/DEVICE_ATTR_RW()
macroses with coccinelle as they "generate" function names internally with
"##". "fresh identifier" should really help here, but now I doubt it's required in
device_attr_show.cocci, function prototype is enough.

Thanks,
Denis

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Julia Lawall]

On Thu, 27 Aug 2020, Joe Perches wrote:

> On Thu, 2020-08-27 at 21:42 +0200, Julia Lawall wrote:
> >
> > On Thu, 27 Aug 2020, Joe Perches wrote:
> >
> > > On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> > > > On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> > > > > On 27/08/2020 15.18, Alex Dewar wrote:
> > > > > > On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> > > > > > > > On 25/08/2020 00.23, Alex Dewar wrote:
> > > > > > > > > kernel/cpu.c: don't use snprintf() for sysfs attrs
> > > > > > > > >
> > > > > > > > > As per the documentation (Documentation/filesystems/sysfs.rst),
> > > > > > > > > snprintf() should not be used for formatting values returned by sysfs.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Can we have a sysfs_sprintf() (could just be a macro that does sprintf)
> > > > > > > > to make it clear to the next reader that we know we're in a sysfs show
> > > > > > > > method? It would make auditing uses of sprintf() much easier.
> > > > > > >
> > > > > > > Code churn to keep code checkers quiet for pointless reasons?  What
> > > > > > > could go wrong with that...
> > > > >
> > > > > I did not (mean to) suggest replacing existing sprintf() calls in sysfs
> > > > > show methods. But when changes _are_ being made, such as when replacing
> > > > > snprintf() calls for whatever reasons, can we please not make it harder
> > > > > for people doing manual audits (those are "code checkers" as well, I
> > > > > suppose, but they do tend to only make noise when finding something).
> > > > >
> > > > > > > It should be pretty obvious to any reader that you are in a sysfs show
> > > > > > > method, as almost all of them are trivially tiny and obvious.
> > > > >
> > > > > git grep doesn't immediately show that, not even with a suitable -C
> > > > > argument, as you can't really know the potential callers unless you open
> > > > > the file and see that the function is only assigned as a .show method.
> > > > > And even that can be a pain because it's all hidden behind five levels
> > > > > of magic macros that build identifiers with ##.
> > > > >
> > > > > > Perhaps I should have mentioned this in the commit message, but the problem
> > > > > > is that snprintf() doesn't return the number of bytes written to the
> > > > > > destination buffer,
> > > > >
> > > > > I'm perfectly well aware of that, TYVM (you may want to 'git log
> > > > > --author Villemoes lib/vsprintf.c').
> > > > >
> > > > >  but the number of bytes that *would have been written if
> > > > > > they fitted*, which may be more than the bounds specified [1]. So "return
> > > > > > snprintf(...)" for sysfs attributes is an antipattern. If you need bounded
> > > > > > string ops, scnprintf() is the way to go. Using snprintf() can give a
> > > > > > false sense of security, because it isn't necessarily safe.
> > > > >
> > > > > Huh? This all seems utterly irrelevant WRT a change that replaces
> > > > > PAGE_SIZE by INT_MAX (because that's what sprintf() is going to pretend
> > > > > you passed). You get the same return value.
> > > > >
> > > > > But I'm not at all concerned about whether one passes the proper buffer
> > > > > size or not in sysfs show methods; with my embedded hat on, I'm all for
> > > > > saving a few bytes of .text here and there. The problem, as far as I'm
> > > > > concerned, is merely that adding sprintf() callers makes it harder to
> > > > > find the problematic sprintf() instances.
> > > > >
> > > >
> > > > Apologies, I think I might have expressed myself poorly, being a kernel noob
> > > > ;-). I know that this is a stylistic change rather than a functional
> > > > one -- I meant that I was hoping that it would be helpful to get rid of bad
> > > > uses of snprintf().
> > > >
> > > > I really like your idea of helper methods though :-). If in show()
> > > > methods we could have something like:
> > > > 	return sysfs_itoa(buf, i);
> > > > in place of:
> > > > 	return sprintf(buf, "%d\n", i);
> > > >
> > > > ... then we wouldn't be introducing any new calls to sprintf() as you
> > > > say, but we'd still be removing a call to snprintf() (which also may be
> > > > problematic). Plus we'd have type checking on the argument.
> > > >
> > > > For returning strings, we could have a bounded and unbounded variant of
> > > > the function. As it seems like only single values should be returned via
> > > > sysfs, if we did things this way then it would only be these
> > > > string-returning functions which could cause buffer overflow problems
> > > > and kernel devs could focus their attention accordingly...
> > > >
> > > > What do people think? I'm happy to have a crack, provided this is
> > > > actually a sensible thing to do! I'm looking for a newbie-level project
> > > > to get started with.
> > >
> > > Not a bad idea.
> > >
> > > Coccinelle should be able to transform the various .show
> > > methods to something sysfs_ prefixed in a fairly automated
> > > way.
> >
> > Something like
> >
> > identifier f;
> > fresh identifier = "sysfs" ## f;
> >
> > may be useful.  Let me know if further help is needed.
>
> Perhaps it's a bit more complicated.
>
> Perhaps what's necessary is to find any
> appropriate .show function and change
> any use of strcpy/sprintf within those
> function to some other name.
>
> For instance:
>
> drivers/isdn/mISDN/core.c-static ssize_t name_show(struct device *dev,
> drivers/isdn/mISDN/core.c-                       struct device_attribute *attr, char *buf)
> drivers/isdn/mISDN/core.c-{
> drivers/isdn/mISDN/core.c:      strcpy(buf, dev_name(dev));
> drivers/isdn/mISDN/core.c-      return strlen(buf);
> drivers/isdn/mISDN/core.c-}
> drivers/isdn/mISDN/core.c-static DEVICE_ATTR_RO(name);
>
> and macroized uses like:
>
> drivers/base/node.c-#define CACHE_ATTR(name, fmt)                                               \
> drivers/base/node.c-static ssize_t name##_show(struct device *dev,                              \
> drivers/base/node.c-                       struct device_attribute *attr,               \
> drivers/base/node.c-                       char *buf)                                   \
> drivers/base/node.c-{                                                                   \
> drivers/base/node.c-    return sprintf(buf, fmt "\n", to_cache_info(dev)->cache_attrs.name);\
> drivers/base/node.c-}                                                                   \
> drivers/base/node.c:DEVICE_ATTR_RO(name);
> drivers/base/node.c-
> drivers/base/node.c-CACHE_ATTR(size, "%llu")
> drivers/base/node.c-CACHE_ATTR(line_size, "%u")
> drivers/base/node.c-CACHE_ATTR(indexing, "%u")
> drivers/base/node.c-CACHE_ATTR(write_policy, "%u")

Coccinelle would fail on these.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Julia Lawall]

On Fri, 28 Aug 2020, Denis Efremov wrote:

> Hi all,
>
> On 8/27/20 10:42 PM, Julia Lawall wrote:
> >
> >
> > On Thu, 27 Aug 2020, Joe Perches wrote:
> >
> >> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> >>> On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> >>>> On 27/08/2020 15.18, Alex Dewar wrote:
> >>>>> On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> >>>>>> On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> >>>>>>> On 25/08/2020 00.23, Alex Dewar wrote:
> >>>>>>>> kernel/cpu.c: don't use snprintf() for sysfs attrs
> >>>>>>>>
> >>>>>>>> As per the documentation (Documentation/filesystems/sysfs.rst),
> >>>>>>>> snprintf() should not be used for formatting values returned by sysfs.
>
> Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
> simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would
> like it more than changing snprintf to scnprintf. As for me, I don't like the idea
> of automated altering of the original logic from bounded snprint to unbouded one
> with sprintf.
>
> [1] https://lkml.org/lkml/2020/8/13/786
>
> Regarding current device_attr_show.cocci implementation, it detects the functions
> by declaration:
> ssize_t any_name(struct device *dev, struct device_attribute *attr, char *buf)
>
> and I limited the check to:
> "return snprintf"
> pattern because there are already too many warnings.
>
> Actually, it looks more correct to check for:
> ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> {
>         <...
> *       snprintf@p(...);
>         ...>
> }
>
> This pattern should also highlight the snprintf calls there we save returned
> value in a var, e.g.:
>
> ret += snprintf(...);
> ...
> ret += snprintf(...);
> ...
> ret += snprintf(...);
>
> return ret;
>
> >
> > Something like
> >
> > identifier f;
> > fresh identifier = "sysfs" ## f;
> >
> > may be useful.  Let me know if further help is needed.
>
> Initially, I wrote the rule to search for DEVICE_ATTR(..., ..., func_name, ...)

This is what I would have expected.

> functions. However, it looks like matching function prototype is enough. At least,
> I failed to find false positives. I rejected the initial DEVICE_ATTR() searching
> because I thought that it's impossible to handle DEVICE_ATTR_RO()/DEVICE_ATTR_RW()
> macroses with coccinelle as they "generate" function names internally with
> "##". "fresh identifier" should really help here, but now I doubt it's required in
> device_attr_show.cocci, function prototype is enough.

It's true that it is probably unique enough.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 23:36 +0200, Julia Lawall wrote:
> On Fri, 28 Aug 2020, Denis Efremov wrote:
[]
> Regarding current device_attr_show.cocci implementation, it detects the functions
> > by declaration:
> > ssize_t any_name(struct device *dev, struct device_attribute *attr, char *buf)
> > 
> > and I limited the check to:
> > "return snprintf"
> > pattern because there are already too many warnings.
> > 
> > Actually, it looks more correct to check for:
> > ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> > {
> >         <...
> > *       snprintf@p(...);
> >         ...>
> > }
> > 
> > This pattern should also highlight the snprintf calls there we save returned
> > value in a var, e.g.:
> > 
> > ret += snprintf(...);
> > ...
> > ret += snprintf(...);
> > ...
> > ret += snprintf(...);
> > 
> > return ret;
> > 
> > > Something like
> > > 
> > > identifier f;
> > > fresh identifier = "sysfs" ## f;
> > > 
> > > may be useful.  Let me know if further help is needed.
> > 
> > Initially, I wrote the rule to search for DEVICE_ATTR(..., ..., func_name, ...)
> 
> This is what I would have expected.
> 
> > functions. However, it looks like matching function prototype is enough. At least,
> > I failed to find false positives. I rejected the initial DEVICE_ATTR() searching
> > because I thought that it's impossible to handle DEVICE_ATTR_RO()/DEVICE_ATTR_RW()
> > macroses with coccinelle as they "generate" function names internally with
> > "##". "fresh identifier" should really help here, but now I doubt it's required in
> > device_attr_show.cocci, function prototype is enough.
> 
> It's true that it is probably unique enough.

I tried:
@@
identifier f_show =~ "^.*_show$";
identifier dev, attr, buf;
const char *chr;
@@
ssize_t f_show(struct device *dev, struct device_attribute *attr, char
*buf)
{
	<...
(
-	sprintf
+	sysfs_sprintf
	(...);
|
-	snprintf(buf, PAGE_SIZE,
+	sysfs_sprintf(buf,
	...);
|
-	scnprintf(buf, PAGE_SIZE,
+	sysfs_sprintf(buf,
	...);
|
	strcpy(buf, chr);
	sysfs_strcpy(buf, chr);
)
	...>
}

which finds direct statements without an assign
but that doesn't find

arch/arm/common/dmabounce.c:static ssize_t dmabounce_show(struct device *dev, struct device_attribute *attr, char *buf)
arch/arm/common/dmabounce.c-{
arch/arm/common/dmabounce.c-    struct dmabounce_device_info *device_info = dev->archdata.dmabounce;
arch/arm/common/dmabounce.c-    return sprintf(buf, "%lu %lu %lu %lu %lu %lu\n",
arch/arm/common/dmabounce.c-            device_info->small.allocs,
arch/arm/common/dmabounce.c-            device_info->large.allocs,
arch/arm/common/dmabounce.c-            device_info->total_allocs - device_info->small.allocs -
arch/arm/common/dmabounce.c-                    device_info->large.allocs,
arch/arm/common/dmabounce.c-            device_info->total_allocs,
arch/arm/common/dmabounce.c-            device_info->map_op_count,
arch/arm/common/dmabounce.c-            device_info->bounce_count);
arch/arm/common/dmabounce.c-}


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[David Laight]

From: Joe Perches
> Sent: 27 August 2020 17:59
> To: Alex Dewar <alex.dewar90@gmail.com>; Rasmus Villemoes <linux@rasmusvillemoes.dk>; cocci
> <cocci@systeme.lip6.fr>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Kees Cook <keescook@chromium.org>; Gustavo A. R.
> Silva <gustavoars@kernel.org>; accessrunner-general@lists.sourceforge.net; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] usb: atm: don't use snprintf() for sysfs attrs
> 
> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
> > On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
> > > On 27/08/2020 15.18, Alex Dewar wrote:
> > > > On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
> > > > > > On 25/08/2020 00.23, Alex Dewar wrote:
> > > > > > > kernel/cpu.c: don't use snprintf() for sysfs attrs
> > > > > > >
> > > > > > > As per the documentation (Documentation/filesystems/sysfs.rst),
> > > > > > > snprintf() should not be used for formatting values returned by sysfs.
> > > > > > >
> > > > > >
> > > > > > Can we have a sysfs_sprintf() (could just be a macro that does sprintf)
> > > > > > to make it clear to the next reader that we know we're in a sysfs show
> > > > > > method? It would make auditing uses of sprintf() much easier.
> > > > >
> > > > > Code churn to keep code checkers quiet for pointless reasons?  What
> > > > > could go wrong with that...
> > >
> > > I did not (mean to) suggest replacing existing sprintf() calls in sysfs
> > > show methods. But when changes _are_ being made, such as when replacing
> > > snprintf() calls for whatever reasons, can we please not make it harder
> > > for people doing manual audits (those are "code checkers" as well, I
> > > suppose, but they do tend to only make noise when finding something).
> > >
> > > > > It should be pretty obvious to any reader that you are in a sysfs show
> > > > > method, as almost all of them are trivially tiny and obvious.
> > >
> > > git grep doesn't immediately show that, not even with a suitable -C
> > > argument, as you can't really know the potential callers unless you open
> > > the file and see that the function is only assigned as a .show method.
> > > And even that can be a pain because it's all hidden behind five levels
> > > of magic macros that build identifiers with ##.
> > >
> > > > Perhaps I should have mentioned this in the commit message, but the problem
> > > > is that snprintf() doesn't return the number of bytes written to the
> > > > destination buffer,
> > >
> > > I'm perfectly well aware of that, TYVM (you may want to 'git log
> > > --author Villemoes lib/vsprintf.c').
> > >
> > >  but the number of bytes that *would have been written if
> > > > they fitted*, which may be more than the bounds specified [1]. So "return
> > > > snprintf(...)" for sysfs attributes is an antipattern. If you need bounded
> > > > string ops, scnprintf() is the way to go. Using snprintf() can give a
> > > > false sense of security, because it isn't necessarily safe.
> > >
> > > Huh? This all seems utterly irrelevant WRT a change that replaces
> > > PAGE_SIZE by INT_MAX (because that's what sprintf() is going to pretend
> > > you passed). You get the same return value.
> > >
> > > But I'm not at all concerned about whether one passes the proper buffer
> > > size or not in sysfs show methods; with my embedded hat on, I'm all for
> > > saving a few bytes of .text here and there. The problem, as far as I'm
> > > concerned, is merely that adding sprintf() callers makes it harder to
> > > find the problematic sprintf() instances.
> > >
> >
> > Apologies, I think I might have expressed myself poorly, being a kernel noob
> > ;-). I know that this is a stylistic change rather than a functional
> > one -- I meant that I was hoping that it would be helpful to get rid of bad
> > uses of snprintf().
> >
> > I really like your idea of helper methods though :-). If in show()
> > methods we could have something like:
> > 	return sysfs_itoa(buf, i);
> > in place of:
> > 	return sprintf(buf, "%d\n", i);
> >
> > ... then we wouldn't be introducing any new calls to sprintf() as you
> > say, but we'd still be removing a call to snprintf() (which also may be
> > problematic). Plus we'd have type checking on the argument.
> >
> > For returning strings, we could have a bounded and unbounded variant of
> > the function. As it seems like only single values should be returned via
> > sysfs, if we did things this way then it would only be these
> > string-returning functions which could cause buffer overflow problems
> > and kernel devs could focus their attention accordingly...
> >
> > What do people think? I'm happy to have a crack, provided this is
> > actually a sensible thing to do! I'm looking for a newbie-level project
> > to get started with.

The problem with that idea is that is the code needs to
merge the output of two values or split an integer as nnn.nn
then it needs to do something different from the 'normal' code.

If the buffer is always PAGE_SIZE the why not embed it in
a structure.
The generated code will be the same, but it will be absolutely
explicit that the size is PAGE_SIZE if the code filling in the
buffer decides it needs to check.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[David Laight]

From: Joe Perches
> Sent: 27 August 2020 21:30
...
> Perhaps what's necessary is to find any
> appropriate .show function and change
> any use of strcpy/sprintf within those
> function to some other name.
> 
> For instance:
> 
> drivers/isdn/mISDN/core.c-static ssize_t name_show(struct device *dev,
> drivers/isdn/mISDN/core.c-                       struct device_attribute *attr, char *buf)
> drivers/isdn/mISDN/core.c-{
> drivers/isdn/mISDN/core.c:      strcpy(buf, dev_name(dev));
> drivers/isdn/mISDN/core.c-      return strlen(buf);
> drivers/isdn/mISDN/core.c-}
> drivers/isdn/mISDN/core.c-static DEVICE_ATTR_RO(name);

That form ends up calculating the string length twice.
Better would be:
	len = strlen(msg);
	memcpy(buf, msg, len);
	return len;


	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 22:03 +0000, David Laight wrote:
> From: Joe Perches
> > Sent: 27 August 2020 21:30
> ...
> > Perhaps what's necessary is to find any
> > appropriate .show function and change
> > any use of strcpy/sprintf within those
> > function to some other name.
> > 
> > For instance:
> > 
> > drivers/isdn/mISDN/core.c-static ssize_t name_show(struct device *dev,
> > drivers/isdn/mISDN/core.c-                       struct device_attribute *attr, char *buf)
> > drivers/isdn/mISDN/core.c-{
> > drivers/isdn/mISDN/core.c:      strcpy(buf, dev_name(dev));
> > drivers/isdn/mISDN/core.c-      return strlen(buf);
> > drivers/isdn/mISDN/core.c-}
> > drivers/isdn/mISDN/core.c-static DEVICE_ATTR_RO(name);
> 
> That form ends up calculating the string length twice.
> Better would be:
> 	len = strlen(msg);
> 	memcpy(buf, msg, len);
> 	return len;

or given clang's requirement for stpcpy

	return stpcpy(buf, dev_name(dev)) - buf;

(I do not advocate for this ;)

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Kees Cook]

On Thu, Aug 27, 2020 at 03:11:57PM -0700, Joe Perches wrote:
> On Thu, 2020-08-27 at 22:03 +0000, David Laight wrote:
> > From: Joe Perches
> > > Sent: 27 August 2020 21:30
> > ...
> > > Perhaps what's necessary is to find any
> > > appropriate .show function and change
> > > any use of strcpy/sprintf within those
> > > function to some other name.
> > > 
> > > For instance:
> > > 
> > > drivers/isdn/mISDN/core.c-static ssize_t name_show(struct device *dev,
> > > drivers/isdn/mISDN/core.c-                       struct device_attribute *attr, char *buf)
> > > drivers/isdn/mISDN/core.c-{
> > > drivers/isdn/mISDN/core.c:      strcpy(buf, dev_name(dev));
> > > drivers/isdn/mISDN/core.c-      return strlen(buf);
> > > drivers/isdn/mISDN/core.c-}
> > > drivers/isdn/mISDN/core.c-static DEVICE_ATTR_RO(name);
> > 
> > That form ends up calculating the string length twice.
> > Better would be:
> > 	len = strlen(msg);
> > 	memcpy(buf, msg, len);
> > 	return len;
> 
> or given clang's requirement for stpcpy
> 
> 	return stpcpy(buf, dev_name(dev)) - buf;
> 
> (I do not advocate for this ;)

Heh. And humans aren't allowed to use stpcpy() in the kernel. :)

-- 
Kees Cook
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Kees Cook]

On Fri, Aug 28, 2020 at 12:01:34AM +0300, Denis Efremov wrote:
> Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
> simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would
> like it more than changing snprintf to scnprintf. As for me, I don't like the idea
> of automated altering of the original logic from bounded snprint to unbouded one
> with sprintf.

Agreed. This just makes me cringe. If the API design declares that when
a show() callback starts, buf has been allocated with PAGE_SIZE bytes,
then that's how the logic should proceed, and it should be using
scnprintf...

show(...) {
	size_t remaining = PAGE_SIZE;

	...
	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
	remaining -= scnprintf(buf, remaining, "fmt", var args ...);

	return PAGE_SIZE - remaining;
}

-- 
Kees Cook
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Denis Efremov]

> 
> I tried:
> @@
> identifier f_show =~ "^.*_show$";


This will miss this kind of functions:
./drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1953:static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
./drivers/gpu/drm/amd/amdgpu/df_v3_6.c:266:static DEVICE_ATTR(df_cntr_avail, S_IRUGO, df_v3_6_get_df_cntr_avail, NULL);
./drivers/input/touchscreen/melfas_mip4.c:1348:static DEVICE_ATTR(fw_version, S_IRUGO, mip4_sysfs_read_fw_version, NULL);
./drivers/input/touchscreen/melfas_mip4.c:1373:static DEVICE_ATTR(hw_version, S_IRUGO, mip4_sysfs_read_hw_version, NULL);
./drivers/input/touchscreen/melfas_mip4.c:1392:static DEVICE_ATTR(product_id, S_IRUGO, mip4_sysfs_read_product_id, NULL);
...

> identifier dev, attr, buf;
> const char *chr;
> @@
> ssize_t f_show(struct device *dev, struct device_attribute *attr, char
> *buf)
> {
> 	<...
> (
> -	sprintf
> +	sysfs_sprintf
> 	(...);
> |
> -	snprintf(buf, PAGE_SIZE,
> +	sysfs_sprintf(buf,
> 	...);
> |
> -	scnprintf(buf, PAGE_SIZE,
> +	sysfs_sprintf(buf,
> 	...);
> |
> 	strcpy(buf, chr);
> 	sysfs_strcpy(buf, chr);
> )
> 	...>
> }
> 
> which finds direct statements without an assign
> but that doesn't find
> 
> arch/arm/common/dmabounce.c:static ssize_t dmabounce_show(struct device *dev, struct device_attribute *attr, char *buf)
> arch/arm/common/dmabounce.c-{
> arch/arm/common/dmabounce.c-    struct dmabounce_device_info *device_info = dev->archdata.dmabounce;
> arch/arm/common/dmabounce.c-    return sprintf(buf, "%lu %lu %lu %lu %lu %lu\n",
> arch/arm/common/dmabounce.c-            device_info->small.allocs,
> arch/arm/common/dmabounce.c-            device_info->large.allocs,
> arch/arm/common/dmabounce.c-            device_info->total_allocs - device_info->small.allocs -
> arch/arm/common/dmabounce.c-                    device_info->large.allocs,
> arch/arm/common/dmabounce.c-            device_info->total_allocs,
> arch/arm/common/dmabounce.c-            device_info->map_op_count,
> arch/arm/common/dmabounce.c-            device_info->bounce_count);
> arch/arm/common/dmabounce.c-}
>

This will match it (the difference is in the ';'):
@@
identifier f_show =~ "^.*_show$";
identifier dev, attr, buf;
@@

ssize_t f_show(struct device *dev, struct device_attribute *attr, char *buf)

{

	<...
-	sprintf
+	sysfs_sprintf
	(...)
	...>
}

Regards,
Denis
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 15:20 -0700, Kees Cook wrote:
> On Fri, Aug 28, 2020 at 12:01:34AM +0300, Denis Efremov wrote:
> > Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
> > simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would
> > like it more than changing snprintf to scnprintf. As for me, I don't like the idea
> > of automated altering of the original logic from bounded snprint to unbouded one
> > with sprintf.
> 
> Agreed. This just makes me cringe. If the API design declares that when
> a show() callback starts, buf has been allocated with PAGE_SIZE bytes,
> then that's how the logic should proceed, and it should be using
> scnprintf...
> 
> show(...) {
> 	size_t remaining = PAGE_SIZE;
> 
> 	...
> 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> 
> 	return PAGE_SIZE - remaining;
> }

It seems likely that coccinelle could do those transform
with any of sprintf/snprintf/scnprint too.

Though my bikeshed would use a single function and have
that function know the maximum output size

Something like:

With single line use:

	return sysfs_emit(buf, buf, fmt, ...) - buf;

and multi-line use:

	char *pos = buf;

	pos = sysfs_emit(buf, pos, fmt1, ...);
	pos = sysfs_emit(buf, pos, fmt2, ...);
	...

	return pos - buf;


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Fri, 2020-08-28 at 01:38 +0300, Denis Efremov wrote:
> > This will match it (the difference is in the ';'):

thanks.


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Thu, 2020-08-27 at 15:45 -0700, Joe Perches wrote:
> On Thu, 2020-08-27 at 15:20 -0700, Kees Cook wrote:
> > On Fri, Aug 28, 2020 at 12:01:34AM +0300, Denis Efremov wrote:
> > > Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
> > > simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would
> > > like it more than changing snprintf to scnprintf. As for me, I don't like the idea
> > > of automated altering of the original logic from bounded snprint to unbouded one
> > > with sprintf.
> > 
> > Agreed. This just makes me cringe. If the API design declares that when
> > a show() callback starts, buf has been allocated with PAGE_SIZE bytes,
> > then that's how the logic should proceed, and it should be using
> > scnprintf...
> > 
> > show(...) {
> > 	size_t remaining = PAGE_SIZE;
> > 
> > 	...
> > 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> > 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> > 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> > 
> > 	return PAGE_SIZE - remaining;
> > }
> 
> It seems likely that coccinelle could do those transform
> with any of sprintf/snprintf/scnprint too.
> 
> Though my bikeshed would use a single function and have
> that function know the maximum output size

Perhaps something like the below with a sample conversion
that uses single and multiple sysfs_emit uses.

I believe coccinelle can _mostly_ automated this.

---
 fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  8 ++++++++
 kernel/power/main.c   | 49 ++++++++++++++++++++++++++-----------------------
 3 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index eb6897ab78e7..c0ff3ba8e373 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
+ *	@fmt:	format
+ *	@...:	arguments to format
+ *
+ *
+ * Returns number of characters written at pos.
+ */
+int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
+{
+	int len;
+	va_list args;
+
+	WARN(pos < buf, "pos < buf\n");
+	WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
+	     pos - buf, PAGE_SIZE);
+	if (pos < buf || pos - buf >= PAGE_SIZE)
+		return 0;
+
+	va_start(args, fmt);
+	len = vscnprintf(pos, PAGE_SIZE - (pos - buf), fmt, args);
+	va_end(args);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(sysfs_emit);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 34e84122f635..5a21d3d30016 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -329,6 +329,8 @@ int sysfs_groups_change_owner(struct kobject *kobj,
 int sysfs_group_change_owner(struct kobject *kobj,
 			     const struct attribute_group *groups, kuid_t kuid,
 			     kgid_t kgid);
+__printf(3, 4)
+int sysfs_emit(char *buf, char *pos, const char *fmt, ...);
 
 #else /* CONFIG_SYSFS */
 
@@ -576,6 +578,12 @@ static inline int sysfs_group_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+__printf(3, 4)
+static inline int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 40f86ec4ab30..f3fb9f255234 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -100,7 +100,7 @@ int pm_async_enabled = 1;
 static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%d\n", pm_async_enabled);
+	return sysfs_emit(buf, buf, "%d\n", pm_async_enabled);
 }
 
 static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -124,7 +124,7 @@ power_attr(pm_async);
 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++)
@@ -132,16 +132,16 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			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;
 }
 
 static suspend_state_t decode_suspend_state(const char *buf, size_t n)
@@ -202,7 +202,7 @@ bool sync_on_suspend_enabled = !IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
 static ssize_t sync_on_suspend_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", sync_on_suspend_enabled);
+	return sysfs_emit(buf, buf, "%d\n", sync_on_suspend_enabled);
 }
 
 static ssize_t sync_on_suspend_store(struct kobject *kobj,
@@ -336,7 +336,7 @@ static ssize_t last_failed_dev_show(struct kobject *kobj,
 	index %= REC_FAILED_NUM;
 	last_failed_dev = suspend_stats.failed_devs[index];
 
-	return sprintf(buf, "%s\n", last_failed_dev);
+	return sysfs_emit(buf, buf, "%s\n", last_failed_dev);
 }
 static struct kobj_attribute last_failed_dev = __ATTR_RO(last_failed_dev);
 
@@ -350,7 +350,7 @@ static ssize_t last_failed_errno_show(struct kobject *kobj,
 	index %= REC_FAILED_NUM;
 	last_failed_errno = suspend_stats.errno[index];
 
-	return sprintf(buf, "%d\n", last_failed_errno);
+	return sysfs_emit(buf, buf, "%d\n", last_failed_errno);
 }
 static struct kobj_attribute last_failed_errno = __ATTR_RO(last_failed_errno);
 
@@ -366,7 +366,7 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
 	step = suspend_stats.failed_steps[index];
 	last_failed_step = suspend_step_name(step);
 
-	return sprintf(buf, "%s\n", last_failed_step);
+	return sysfs_emit(buf, buf, "%s\n", last_failed_step);
 }
 static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
 
@@ -474,7 +474,7 @@ bool pm_print_times_enabled;
 static ssize_t pm_print_times_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", pm_print_times_enabled);
+	return sysfs_emit(buf, buf, "%d\n", pm_print_times_enabled);
 }
 
 static ssize_t pm_print_times_store(struct kobject *kobj,
@@ -504,7 +504,9 @@ static ssize_t pm_wakeup_irq_show(struct kobject *kobj,
 					struct kobj_attribute *attr,
 					char *buf)
 {
-	return pm_wakeup_irq ? sprintf(buf, "%u\n", pm_wakeup_irq) : -ENODATA;
+	if (!pm_wakeup_irq)
+		return -ENODATA;
+	return sysfs_emit(buf, buf, "%u\n", pm_wakeup_irq);
 }
 
 power_attr_ro(pm_wakeup_irq);
@@ -514,7 +516,7 @@ bool pm_debug_messages_on __read_mostly;
 static ssize_t pm_debug_messages_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", pm_debug_messages_on);
+	return sysfs_emit(buf, buf, "%d\n", pm_debug_messages_on);
 }
 
 static ssize_t pm_debug_messages_store(struct kobject *kobj,
@@ -704,8 +706,9 @@ static ssize_t wakeup_count_show(struct kobject *kobj,
 {
 	unsigned int val;
 
-	return pm_get_wakeup_count(&val, true) ?
-		sprintf(buf, "%u\n", val) : -EINTR;
+	if (!pm_get_wakeup_count(&val, true))
+		return -EINTR;
+	return sysfs_emit(buf, buf, "%u\n", val);
 }
 
 static ssize_t wakeup_count_store(struct kobject *kobj,
@@ -747,17 +750,17 @@ static ssize_t autosleep_show(struct kobject *kobj,
 	suspend_state_t state = pm_autosleep_state();
 
 	if (state == PM_SUSPEND_ON)
-		return sprintf(buf, "off\n");
+		return sysfs_emit(buf, buf, "off\n");
 
 #ifdef CONFIG_SUSPEND
 	if (state < PM_SUSPEND_MAX)
-		return sprintf(buf, "%s\n", pm_states[state] ?
-					pm_states[state] : "error");
+		return sysfs_emit(buf, buf, "%s\n",
+				  pm_states[state] ?: "error");
 #endif
 #ifdef CONFIG_HIBERNATION
-	return sprintf(buf, "disk\n");
+	return sysfs_emit(buf, buf, "disk\n");
 #else
-	return sprintf(buf, "error");
+	return sysfs_emit(buf, buf, "error\n");
 #endif
 }
 
@@ -826,7 +829,7 @@ int pm_trace_enabled;
 static ssize_t pm_trace_show(struct kobject *kobj, struct kobj_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%d\n", pm_trace_enabled);
+	return sysfs_emit(buf, buf, "%d\n", pm_trace_enabled);
 }
 
 static ssize_t
@@ -863,7 +866,7 @@ power_attr_ro(pm_trace_dev_match);
 static ssize_t pm_freeze_timeout_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%u\n", freeze_timeout_msecs);
+	return sysfs_emit(buf, buf, "%u\n", freeze_timeout_msecs);
 }
 
 static ssize_t pm_freeze_timeout_store(struct kobject *kobj,


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[David Laight]

From: Kees Cook
> Sent: 27 August 2020 23:21
...
> 
> Agreed. This just makes me cringe. If the API design declares that when
> a show() callback starts, buf has been allocated with PAGE_SIZE bytes,
> then that's how the logic should proceed, and it should be using
> scnprintf...
> 
> show(...) {
> 	size_t remaining = PAGE_SIZE;
> 
> 	...
> 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);

Not quite what you had in mind, maybe:
	char *end = buf + PAGE_SIZE;

	buf += scnprintf(buf, end - buf, ...);

	return PAGE_SIZE - (end - buf);

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Kees Cook]

On Thu, Aug 27, 2020 at 09:12:06PM -0700, Joe Perches wrote:
> Perhaps something like the below with a sample conversion
> that uses single and multiple sysfs_emit uses.

On quick review, I like it. :)

> [...]
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> +	int len;
> +	va_list args;
> +
> +	WARN(pos < buf, "pos < buf\n");
> +	WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
> +	     pos - buf, PAGE_SIZE);
> +	if (pos < buf || pos - buf >= PAGE_SIZE)
> +		return 0;

This can be:

	if (WARN(pos < buf, "pos < buf\n") ||
	    WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
		 pos - buf, PAGE_SIZE))
		return 0;

-- 
Kees Cook
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Fri, 2020-08-28 at 00:58 -0700, Kees Cook wrote:
> On Thu, Aug 27, 2020 at 09:12:06PM -0700, Joe Perches wrote:
> > Perhaps something like the below with a sample conversion
> > that uses single and multiple sysfs_emit uses.
> 
> On quick review, I like it. :)
> 
> > [...]
> > +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> > +{
> > +	int len;
> > +	va_list args;
> > +
> > +	WARN(pos < buf, "pos < buf\n");
> > +	WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
> > +	     pos - buf, PAGE_SIZE);
> > +	if (pos < buf || pos - buf >= PAGE_SIZE)
> > +		return 0;
> 
> This can be:
> 
> 	if (WARN(pos < buf, "pos < buf\n") ||
> 	    WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
> 		 pos - buf, PAGE_SIZE))
> 		return 0;

I had some vague recollection that WARN could be compiled
away to nothing somehow.  True or false?

If false, sure, of course, it'd be faster too.

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

[Joe Perches]

On Fri, 2020-08-28 at 01:10 -0700, Joe Perches wrote:
> On Fri, 2020-08-28 at 00:58 -0700, Kees Cook wrote:
> > On Thu, Aug 27, 2020 at 09:12:06PM -0700, Joe Perches wrote:
> > > Perhaps something like the below with a sample conversion
> > > that uses single and multiple sysfs_emit uses.
> > 
> > On quick review, I like it. :)
> > 
> > > [...]
> > > +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> > > +{
> > > +	int len;
> > > +	va_list args;
> > > +
> > > +	WARN(pos < buf, "pos < buf\n");
> > > +	WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
> > > +	     pos - buf, PAGE_SIZE);
> > > +	if (pos < buf || pos - buf >= PAGE_SIZE)
> > > +		return 0;
> > 
> > This can be:
> > 
> > 	if (WARN(pos < buf, "pos < buf\n") ||
> > 	    WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
> > 		 pos - buf, PAGE_SIZE))
> > 		return 0;
> 
> I have some vague recollection that WARN could be compiled
> away to nothing somehow.  True or false?
> 
> If false, sure, of course, it'd be faster too.

I can't find an instance where WARN doesn't return the
condition.

And likely even faster would be to just show "invalid pos"
instead of specific messages.

	if (WARN(pos < buf || (pos - buf) >= PAGE_SIZE,
		 "Invalid pos\n");
		return 0;

or maybe use WARN_ONCE or no WARN at all.

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci