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, 16 Sep 2021 13:00:16 +0200
From:   Jan Kara <jack@suse.cz>
To:     Vivek Goyal <vgoyal@redhat.com>
Cc:     viro@zeniv.linux.org.uk,
        Linux fsdevel mailing list <linux-fsdevel@vger.kernel.org>,
        linux kernel mailing list <linux-kernel@vger.kernel.org>,
        Jan Kara <jack@suse.cz>, xu.xin16@zte.com.cn,
        Christoph Hellwig <hch@infradead.org>, zhang.yunkai@zte.com.cn
Subject: Re: [PATCH] init/do_mounts.c: Harden split_fs_names() against buffer
 overflow
Message-ID: <20210916110016.GG10610@quack2.suse.cz>
References: <YUIPnPV2ttOHNIcX@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YUIPnPV2ttOHNIcX@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Wed 15-09-21 11:22:04, Vivek Goyal wrote:
> split_fs_names() currently takes comma separated list of filesystems
> and converts it into individual filesystem strings. Pleaces these
> strings in the input buffer passed by caller and returns number of
> strings.
> 
> If caller manages to pass input string bigger than buffer, then we
> can write beyond the buffer. Or if string just fits buffer, we will
> still write beyond the buffer as we append a '\0' byte at the end.
> 
> Will be nice to pass size of input buffer to split_fs_names() and
> put enough checks in place so such buffer overrun possibilities
> do not occur.
> 
> Hence this patch adds "size" parameter to split_fs_names() and makes
> sure we do not access memory beyond size. If input string "names"
> is larger than passed in buffer, input string will be truncated to
> fit in buffer.
> 
> Reported-by: xu xin <xu.xin16@zte.com.cn>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

The patch looks correct but IMO is more complicated than it needs to be...
See below.

> Index: redhat-linux/init/do_mounts.c
> ===================================================================
> --- redhat-linux.orig/init/do_mounts.c	2021-09-15 08:46:33.801689806 -0400
> +++ redhat-linux/init/do_mounts.c	2021-09-15 09:52:09.884449718 -0400
> @@ -338,19 +338,20 @@ __setup("rootflags=", root_data_setup);
>  __setup("rootfstype=", fs_names_setup);
>  __setup("rootdelay=", root_delay_setup);
>  
> -static int __init split_fs_names(char *page, char *names)
> +static int __init split_fs_names(char *page, size_t size, char *names)
>  {
>  	int count = 0;
> -	char *p = page;
> +	char *p = page, *end = page + size - 1;
> +
> +	strncpy(p, root_fs_names, size);

Why not strlcpy()? That way you don't have to explicitely terminate the
string...

> +	*end = '\0';
>  
> -	strcpy(p, root_fs_names);
>  	while (*p++) {
>  		if (p[-1] == ',')
>  			p[-1] = '\0';
>  	}
> -	*p = '\0';
>  
> -	for (p = page; *p; p += strlen(p)+1)
> +	for (p = page; p < end && *p; p += strlen(p)+1)
>  		count++;

And I kind of fail to see why you have a separate loop for counting number
of elements when you could count them directly when changing ',' to '\0'.
There's this small subtlety that e.g. string 'foo,,bar' will report to have
only 1 element with the above code while direct computation would return 3
but that's hardly problem IMHO.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR