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;
Subject: Re: [PATCH] usb: atm: don't use snprintf() for sysfs attrs
To:     Alex Dewar <alex.dewar90@gmail.com>
Cc:     Kees Cook <keescook@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        accessrunner-general@lists.sourceforge.net,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200824222322.22962-1-alex.dewar90@gmail.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <48f2dc90-7852-eaf1-55d7-2c85cf954688@rasmusvillemoes.dk>
Date:   Thu, 27 Aug 2020 08:42:06 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200824222322.22962-1-alex.dewar90@gmail.com>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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