Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 16 Sep 2021 13:08:59 -0400
From:   Vivek Goyal <vgoyal@redhat.com>
To:     Jan Kara <jack@suse.cz>
Cc:     viro@zeniv.linux.org.uk,
        Linux fsdevel mailing list <linux-fsdevel@vger.kernel.org>,
        linux kernel mailing list <linux-kernel@vger.kernel.org>,
        xu.xin16@zte.com.cn, Christoph Hellwig <hch@infradead.org>,
        zhang.yunkai@zte.com.cn
Subject: Re: [PATCH] init/do_mounts.c: Harden split_fs_names() against buffer
 overflow
Message-ID: <YUN6K6qd2H6hsFZL@redhat.com>
References: <YUIPnPV2ttOHNIcX@redhat.com>
 <20210916110016.GG10610@quack2.suse.cz>
 <YUNlwdCf53HqRhKd@redhat.com>
 <20210916165446.GK10610@quack2.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210916165446.GK10610@quack2.suse.cz>
Precedence: bulk

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