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

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Sep 17, 2021 at 10:07:30AM +0200, Jan Kara wrote:
> 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@zte.com.cn>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.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:

Hi Jan,

This sounds good. I will get rid of str_start. V3 of the patch is on
the way.

Vivek
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>