* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs [not found] ` <20200827144846.yauuttjaqtxaldxg@lenovo-laptop> @ 2020-08-27 16:58 ` Joe Perches 2020-08-27 19:42 ` Julia Lawall 2020-08-27 21:54 ` David Laight 0 siblings, 2 replies; 21+ messages in thread From: Joe Perches @ 2020-08-27 16:58 UTC (permalink / raw) To: Alex Dewar, Rasmus Villemoes, cocci Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, linux-kernel, Gustavo A. R. Silva, accessrunner-general 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 16:58 ` [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs Joe Perches @ 2020-08-27 19:42 ` Julia Lawall 2020-08-27 20:29 ` Joe Perches 2020-08-27 21:01 ` Denis Efremov 2020-08-27 21:54 ` David Laight 1 sibling, 2 replies; 21+ messages in thread From: Julia Lawall @ 2020-08-27 19:42 UTC (permalink / raw) To: Joe Perches Cc: Gustavo A. R. Silva, Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, accessrunner-general, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 19:42 ` Julia Lawall @ 2020-08-27 20:29 ` Joe Perches 2020-08-27 21:00 ` Joe Perches ` (2 more replies) 2020-08-27 21:01 ` Denis Efremov 1 sibling, 3 replies; 21+ messages in thread From: Joe Perches @ 2020-08-27 20:29 UTC (permalink / raw) To: Julia Lawall Cc: Gustavo A. R. Silva, Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, accessrunner-general, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 20:29 ` Joe Perches @ 2020-08-27 21:00 ` Joe Perches 2020-08-27 21:29 ` Julia Lawall 2020-08-27 22:03 ` David Laight 2 siblings, 0 replies; 21+ messages in thread From: Joe Perches @ 2020-08-27 21:00 UTC (permalink / raw) To: Julia Lawall Cc: Gustavo A. R. Silva, Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 20:29 ` Joe Perches 2020-08-27 21:00 ` Joe Perches @ 2020-08-27 21:29 ` Julia Lawall 2020-08-27 22:03 ` David Laight 2 siblings, 0 replies; 21+ messages in thread From: Julia Lawall @ 2020-08-27 21:29 UTC (permalink / raw) To: Joe Perches Cc: Gustavo A. R. Silva, Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, accessrunner-general, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 20:29 ` Joe Perches 2020-08-27 21:00 ` Joe Perches 2020-08-27 21:29 ` Julia Lawall @ 2020-08-27 22:03 ` David Laight 2020-08-27 22:11 ` Joe Perches 2 siblings, 1 reply; 21+ messages in thread From: David Laight @ 2020-08-27 22:03 UTC (permalink / raw) To: 'Joe Perches', Julia Lawall Cc: Gustavo A. R. Silva, Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, accessrunner-general, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 22:03 ` David Laight @ 2020-08-27 22:11 ` Joe Perches 2020-08-27 22:16 ` Kees Cook 0 siblings, 1 reply; 21+ messages in thread From: Joe Perches @ 2020-08-27 22:11 UTC (permalink / raw) To: David Laight, Julia Lawall Cc: Gustavo A. R. Silva, Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, accessrunner-general, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 22:11 ` Joe Perches @ 2020-08-27 22:16 ` Kees Cook 0 siblings, 0 replies; 21+ messages in thread From: Kees Cook @ 2020-08-27 22:16 UTC (permalink / raw) To: Joe Perches Cc: Gustavo A. R. Silva, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, linux-kernel, cocci, David Laight, accessrunner-general, Alex Dewar 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 19:42 ` Julia Lawall 2020-08-27 20:29 ` Joe Perches @ 2020-08-27 21:01 ` Denis Efremov 2020-08-27 21:36 ` Julia Lawall 2020-08-27 22:20 ` Kees Cook 1 sibling, 2 replies; 21+ messages in thread From: Denis Efremov @ 2020-08-27 21:01 UTC (permalink / raw) To: Julia Lawall, Joe Perches Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 21:01 ` Denis Efremov @ 2020-08-27 21:36 ` Julia Lawall 2020-08-27 21:44 ` Joe Perches 2020-08-27 22:20 ` Kees Cook 1 sibling, 1 reply; 21+ messages in thread From: Julia Lawall @ 2020-08-27 21:36 UTC (permalink / raw) To: Denis Efremov Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, Joe Perches, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 21:36 ` Julia Lawall @ 2020-08-27 21:44 ` Joe Perches 2020-08-27 22:38 ` Denis Efremov 0 siblings, 1 reply; 21+ messages in thread From: Joe Perches @ 2020-08-27 21:44 UTC (permalink / raw) To: Julia Lawall, Denis Efremov Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 21:44 ` Joe Perches @ 2020-08-27 22:38 ` Denis Efremov 2020-08-27 22:48 ` Joe Perches 0 siblings, 1 reply; 21+ messages in thread From: Denis Efremov @ 2020-08-27 22:38 UTC (permalink / raw) To: Joe Perches, Julia Lawall Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, cocci > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 22:38 ` Denis Efremov @ 2020-08-27 22:48 ` Joe Perches 0 siblings, 0 replies; 21+ messages in thread From: Joe Perches @ 2020-08-27 22:48 UTC (permalink / raw) To: Denis Efremov, Julia Lawall Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 21:01 ` Denis Efremov 2020-08-27 21:36 ` Julia Lawall @ 2020-08-27 22:20 ` Kees Cook 2020-08-27 22:45 ` Joe Perches 2020-08-28 7:39 ` David Laight 1 sibling, 2 replies; 21+ messages in thread From: Kees Cook @ 2020-08-27 22:20 UTC (permalink / raw) To: Denis Efremov Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, Joe Perches, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 22:20 ` Kees Cook @ 2020-08-27 22:45 ` Joe Perches 2020-08-28 4:12 ` Joe Perches 2020-08-28 7:39 ` David Laight 1 sibling, 1 reply; 21+ messages in thread From: Joe Perches @ 2020-08-27 22:45 UTC (permalink / raw) To: Kees Cook, Denis Efremov Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 22:45 ` Joe Perches @ 2020-08-28 4:12 ` Joe Perches 2020-08-28 7:58 ` Kees Cook 0 siblings, 1 reply; 21+ messages in thread From: Joe Perches @ 2020-08-28 4:12 UTC (permalink / raw) To: Kees Cook, Denis Efremov Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, cocci 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-28 4:12 ` Joe Perches @ 2020-08-28 7:58 ` Kees Cook 2020-08-28 8:10 ` Joe Perches 0 siblings, 1 reply; 21+ messages in thread From: Kees Cook @ 2020-08-28 7:58 UTC (permalink / raw) To: Joe Perches Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-28 7:58 ` Kees Cook @ 2020-08-28 8:10 ` Joe Perches 2020-08-28 8:22 ` Joe Perches 0 siblings, 1 reply; 21+ messages in thread From: Joe Perches @ 2020-08-28 8:10 UTC (permalink / raw) To: Kees Cook Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-28 8:10 ` Joe Perches @ 2020-08-28 8:22 ` Joe Perches 0 siblings, 0 replies; 21+ messages in thread From: Joe Perches @ 2020-08-28 8:22 UTC (permalink / raw) To: Kees Cook Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 22:20 ` Kees Cook 2020-08-27 22:45 ` Joe Perches @ 2020-08-28 7:39 ` David Laight 1 sibling, 0 replies; 21+ messages in thread From: David Laight @ 2020-08-28 7:39 UTC (permalink / raw) To: 'Kees Cook', Denis Efremov Cc: Greg Kroah-Hartman, linux-usb, Rasmus Villemoes, Gustavo A. R. Silva, linux-kernel, Alex Dewar, accessrunner-general, Joe Perches, cocci 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs 2020-08-27 16:58 ` [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs Joe Perches 2020-08-27 19:42 ` Julia Lawall @ 2020-08-27 21:54 ` David Laight 1 sibling, 0 replies; 21+ messages in thread From: David Laight @ 2020-08-27 21:54 UTC (permalink / raw) To: 'Joe Perches', Alex Dewar, Rasmus Villemoes, cocci Cc: Kees Cook, Greg Kroah-Hartman, linux-usb, linux-kernel, Gustavo A. R. Silva, accessrunner-general 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-08-28 19:43 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20200824222322.22962-1-alex.dewar90@gmail.com> [not found] ` <48f2dc90-7852-eaf1-55d7-2c85cf954688@rasmusvillemoes.dk> [not found] ` <20200827071537.GA168593@kroah.com> [not found] ` <20200827131819.7rcl2f5js3hkoqj2@lenovo-laptop> [not found] ` <def24e9e-018c-9712-0d07-d4cbc84f07d9@rasmusvillemoes.dk> [not found] ` <20200827144846.yauuttjaqtxaldxg@lenovo-laptop> 2020-08-27 16:58 ` [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs Joe Perches 2020-08-27 19:42 ` Julia Lawall 2020-08-27 20:29 ` Joe Perches 2020-08-27 21:00 ` Joe Perches 2020-08-27 21:29 ` Julia Lawall 2020-08-27 22:03 ` David Laight 2020-08-27 22:11 ` Joe Perches 2020-08-27 22:16 ` Kees Cook 2020-08-27 21:01 ` Denis Efremov 2020-08-27 21:36 ` Julia Lawall 2020-08-27 21:44 ` Joe Perches 2020-08-27 22:38 ` Denis Efremov 2020-08-27 22:48 ` Joe Perches 2020-08-27 22:20 ` Kees Cook 2020-08-27 22:45 ` Joe Perches 2020-08-28 4:12 ` Joe Perches 2020-08-28 7:58 ` Kees Cook 2020-08-28 8:10 ` Joe Perches 2020-08-28 8:22 ` Joe Perches 2020-08-28 7:39 ` David Laight 2020-08-27 21:54 ` David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).