Re: [PATCH v2] init/do_mounts.c: Harden split_fs_names() against buffer overflow

[Jan Kara]

On Thu 16-09-21 11:50:58, Vivek Goyal wrote:
> split_fs_names() currently takes comma separate 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.
> 
> Pass size of input buffer to split_fs_names() and put enough checks
> in place so such buffer overrun possibilities do not occur.
> 
> This patch does few things.
> 
> - Add a parameter "size" to split_fs_names(). This specifies size
>   of input buffer.
> 
> - Use strlcpy() (instead of strcpy()) so that we can't go beyond
>   buffer size. If input string "names" is larger than passed in
>   buffer, input string will be truncated to fit in buffer.
> 
> - Stop appending extra '\0' character at the end and avoid one
>   possibility of going beyond the input buffer size.
> 
> - Do not use extra loop to count number of strings.
> 
> - Previously if one passed "rootfstype=foo,,bar", split_fs_names()
>   will return only 1 string "foo" (and "bar" will be truncated
>   due to extra ,). After this patch, now split_fs_names() will
>   return 3 strings ("foo", zero-sized-string, and "bar").
> 
>   Callers of split_fs_names() have been modified to check for
>   zero sized string and skip to next one.
> 
> Reported-by: xu xin <xu.xin16@xxxxxxxxxx>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
>  init/do_mounts.c |   28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)

Just one nit 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-16 11:28:36.753625037 -0400
> @@ -338,19 +338,25 @@ __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;
> +	bool str_start = false;
>  
> -	strcpy(p, root_fs_names);
> +	strlcpy(p, root_fs_names, size);
>  	while (*p++) {
> -		if (p[-1] == ',')
> +		if (p[-1] == ',') {
>  			p[-1] = '\0';
> +			count++;
> +			str_start = false;
> +		} else {
> +			str_start = true;
> +		}
>  	}
> -	*p = '\0';
>  
> -	for (p = page; *p; p += strlen(p)+1)
> +	/* Last string which might not be comma terminated */
> +	if (str_start)
>  		count++;

You could avoid the whole str_start logic if you just initialize 'count' to
1 - in the worst case you'll have 0-length string at the end (for case like
xfs,) but you deal with 0-length strings in the callers anyway. Otherwise
the patch looks good so feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR