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 11:41:53 -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: <YUNlwdCf53HqRhKd@redhat.com>
References: <YUIPnPV2ttOHNIcX@redhat.com>
 <20210916110016.GG10610@quack2.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210916110016.GG10610@quack2.suse.cz>
Precedence: bulk

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.

Vivek