Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 27 Aug 2020 09:15:37 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Alex Dewar <alex.dewar90@gmail.com>,
        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
Message-ID: <20200827071537.GA168593@kroah.com>
References: <20200824222322.22962-1-alex.dewar90@gmail.com>
 <48f2dc90-7852-eaf1-55d7-2c85cf954688@rasmusvillemoes.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <48f2dc90-7852-eaf1-55d7-2c85cf954688@rasmusvillemoes.dk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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