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

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

[Vivek Goyal]

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>
---
 init/do_mounts.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

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);
+	*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++;
 
 	return count;
@@ -404,7 +405,7 @@ void __init mount_block_root(char *name,
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
 	if (root_fs_names)
-		num_fs = split_fs_names(fs_names, root_fs_names);
+		num_fs = split_fs_names(fs_names, PAGE_SIZE, root_fs_names);
 	else
 		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
@@ -543,7 +544,7 @@ static int __init mount_nodev_root(void)
 	fs_names = (void *)__get_free_page(GFP_KERNEL);
 	if (!fs_names)
 		return -EINVAL;
-	num_fs = split_fs_names(fs_names, root_fs_names);
+	num_fs = split_fs_names(fs_names, PAGE_SIZE, root_fs_names);
 
 	for (i = 0, fstype = fs_names; i < num_fs;
 	     i++, fstype += strlen(fstype) + 1) {

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

[Christoph Hellwig]

On Wed, Sep 15, 2021 at 11:22:04AM -0400, Vivek Goyal wrote:
> 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.

Will be nice sounds weird.

> 
> 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.

There's really two aspects here:  checking for a max size and explicitly
passing one.  I'm fine with passing the argument even if it always is
PAGE_SIZE, but this should probably be documented a little better.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christian Brauner]

On Wed, Sep 15, 2021 at 11:22:04AM -0400, 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>
> ---

Strange but probably reasonable,
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

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

[Jan Kara]

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...

> +	*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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Vivek Goyal]

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

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@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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Vivek Goyal]

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