linux-kernel - Re: [PATCH v2] init/do_mounts.c: Harden split_fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210917080730.GA5284@quack2.suse.cz>
Date:   Fri, 17 Sep 2021 10:07:30 +0200
From:   Jan Kara <jack@...e.cz>
To:     Vivek Goyal <vgoyal@...hat.com>
Cc:     Christoph Hellwig <hch@...radead.org>, Jan Kara <jack@...e.cz>,
        Christian Brauner <christian.brauner@...ntu.com>,
        viro@...iv.linux.org.uk,
        Linux fsdevel mailing list <linux-fsdevel@...r.kernel.org>,
        linux kernel mailing list <linux-kernel@...r.kernel.org>,
        xu.xin16@....com.cn
Subject: Re: [PATCH v2] init/do_mounts.c: Harden split_fs_names() against
 buffer overflow

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@....com.cn>
> Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
> ---
>  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@...e.cz>

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