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

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Sep 16, 2021 at 06:54:46PM +0200, Jan Kara wrote:
> 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@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...
> > 
> > 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.

There are only two callers of split_fs_names(). So changing them for
zero sized strings was trivial (patch v2).

So I peronally don't mind supporting "rootfstype=xfs,,ext4" if there
is an accidental extra ',' in there.

Vivek