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;
From:   Alex Dewar <alex.dewar90@gmail.com>
Date:   Thu, 27 Aug 2020 14:18:19 +0100
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        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: <20200827131819.7rcl2f5js3hkoqj2@lenovo-laptop>
References: <20200824222322.22962-1-alex.dewar90@gmail.com>
 <48f2dc90-7852-eaf1-55d7-2c85cf954688@rasmusvillemoes.dk>
 <20200827071537.GA168593@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200827071537.GA168593@kroah.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

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

[1] https://github.com/KSPP/linux/issues/105

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