Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] staging: lustre: Remove VLA usage
To:     Kees Cook <keescook@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, "Tobin C. Harding" <me@tobin.cc>,
        Tycho Andersen <tycho@tycho.ws>,
        Oleg Drokin <oleg.drokin@intel.com>,
        Andreas Dilger <andreas.dilger@intel.com>,
        James Simmons <jsimmons@infradead.org>,
        Dmitry Eremin <dmitry.eremin@intel.com>,
        Gargi Sharma <gs051095@gmail.com>,
        lustre-devel@lists.lustre.org, devel@driverdev.osuosl.org,
        Kernel Hardening <kernel-hardening@lists.openwall.com>
References: <20180307054608.GA9300@beast>
From:   Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Message-ID: <d5de0034-6f74-5b73-2df8-dbe5b366c2fc@prevas.dk>
Date:   Wed, 7 Mar 2018 14:10:41 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <20180307054608.GA9300@beast>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: prevas.se does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2018 13:10:45.1976 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 09b1b293-37e6-43f4-edd2-08d5842cd700
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: d350cf71-778d-4780-88f5-071a4cb1ed61
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR10MB0435
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2018-03-07 06:46, Kees Cook wrote:
> The kernel would like to remove all VLA usage. This switches to a
> simple kasprintf() instead.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..aab4eab64289 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>  		    const char *name, const void *value, size_t size,
>  		    int flags)
>  {
> -	char fullname[strlen(handler->prefix) + strlen(name) + 1];
> +	char *fullname;
>  	struct ll_sb_info *sbi = ll_i2sbi(inode);
>  	struct ptlrpc_request *req = NULL;
>  	const char *pv = value;
> @@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>  			return -EPERM;
>  	}
>  
> -	sprintf(fullname, "%s%s\n", handler->prefix, name);

It's probably worth pointing out that this actually fixes an
unconditional buffer overflow: fullname only has room for the two
strings and the '\n', but vsnprintf() is told that the buffer has
infinite size (well, INT_MAX), so there should be plenty of room to
append the '\0' after the '\n'.

> +	fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
> +	if (!fullname)
> +		return -ENOMEM;
>  	rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
>  			 valid, fullname, pv, size, 0, flags,
>  			 ll_i2suppgid(inode), &req);
> +	kfree(fullname);
>  	if (rc) {
>  		if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
>  			LCONSOLE_INFO("Disabling user_xattr feature because it is not supported on the server\n");
> @@ -364,7 +367,7 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
>  			       struct dentry *dentry, struct inode *inode,
>  			       const char *name, void *buffer, size_t size)
>  {
> -	char fullname[strlen(handler->prefix) + strlen(name) + 1];
> +	char *fullname;
>  	struct ll_sb_info *sbi = ll_i2sbi(inode);
>  #ifdef CONFIG_FS_POSIX_ACL
>  	struct ll_inode_info *lli = ll_i2info(inode);
> @@ -411,9 +414,13 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
>  	if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
>  		return -ENODATA;
>  #endif
> -	sprintf(fullname, "%s%s\n", handler->prefix, name);

Same here.

I'm a little surprised this hasn't been caugt by static analysis, I
thought gcc/coverity/smatch/whatnot had gotten pretty good at computing
the size of the output generated by a given format string with "known"
arguments and comparing to the size of the output buffer. Though of
course it does require the tool to be able to do symbolic manipulations,
in this case realizing that

outsize == strlen(x)+strlen(y)+1+1 > bufsize == strlen(x)+strlen(y)+1

Rasmus