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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Aug 2020 09:15:37 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Alex Dewar <alex.dewar90@...il.com>,
        Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        accessrunner-general@...ts.sourceforge.net,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: atm: don't use snprintf() for sysfs attrs

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.
> >
> 
> 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.

Code churn to keep code checkers quiet for pointless reasons?  What
could go wrong with that...

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.  Yes, it
doesn't help with those that make blanket statements like "sprintf is
evil", but I think that kind of just shows them that they shouldn't be
making foolish blanket statements like that :)

Anyway, we've had this for 20 years, if sysfs calls are the only left
remaining user of sprintf(), then I'll be glad to consider using a
"wrapper" function or macro.

thanks,

greg k-h