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. > Sure. But then the security guys come along and send a patch saying "sprintf is evil, always pass a buffer bound". Perhaps with a side of "this code could get copy-pasted, better not promote the use of sprintf more than strictly necessary". 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. > static ssize_t cxacru_sysfs_showattr_LINE(u32 value, char *buf) > @@ -275,8 +275,8 @@ static ssize_t cxacru_sysfs_showattr_LINE(u32 value, char *buf) > "waiting", "initialising" > }; > if (unlikely(value >= ARRAY_SIZE(str))) > - return snprintf(buf, PAGE_SIZE, "%u\n", value); > - return snprintf(buf, PAGE_SIZE, "%s\n", str[value]); > + return sprintf(buf, "%u\n", value); > + return sprintf(buf, "%s\n", str[value]); > } Not this patch in particular, but in some cases the string being printed is not immediately adjacent to the sprintf call, making it rather hard to subsequently verify that yes, that string is certainly reasonably bounded. If you really want to save the few bytes of code that is the practical effect of eliding the PAGE_SIZE argument, how about a sysfs_print_string(buf, str) helper that prints the string and appends a newline; that's another argument saved. And again it would make it obvious to a reader that that particular helper is only to be used in sysfs show methods. Rasmus
