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

[Jan Kara]

On Thu 16-09-21 11:41:53, Vivek Goyal wrote:
> On Thu, Sep 16, 2021 at 01:00:16PM +0200, Jan Kara wrote:
> > 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@xxxxxxxxxx>
> > > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > 
> > 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...
> 
> Sure, will use strlcpy().
> 
> > 
> > > +	*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.
> 
> Ok, will make this change. One side affect of this change will be that now
> split_fs_names() can return zero sized strings and caller will have
> to check for those and skip to next string.

Or we can just abort the loop early and don't bother with converting
further ',' if 0-length strings are indeed any problem.

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