LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] init/do_mounts.c: Harden split_fs_names() against buffer overflow
@ 2021-09-16 15:50 Vivek Goyal
  2021-09-17  8:07 ` Jan Kara
  0 siblings, 1 reply; 3+ messages in thread
From: Vivek Goyal @ 2021-09-16 15:50 UTC (permalink / raw)
  To: Christoph Hellwig, Jan Kara, Christian Brauner, viro
  Cc: Linux fsdevel mailing list, linux kernel mailing list, xu.xin16

split_fs_names() currently takes comma separate 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.

Pass size of input buffer to split_fs_names() and put enough checks
in place so such buffer overrun possibilities do not occur.

This patch does few things.

- Add a parameter "size" to split_fs_names(). This specifies size
  of input buffer.

- Use strlcpy() (instead of strcpy()) so that we can't go beyond
  buffer size. If input string "names" is larger than passed in
  buffer, input string will be truncated to fit in buffer.

- Stop appending extra '\0' character at the end and avoid one
  possibility of going beyond the input buffer size.

- Do not use extra loop to count number of strings.

- Previously if one passed "rootfstype=foo,,bar", split_fs_names()
  will return only 1 string "foo" (and "bar" will be truncated
  due to extra ,). After this patch, now split_fs_names() will
  return 3 strings ("foo", zero-sized-string, and "bar").

  Callers of split_fs_names() have been modified to check for
  zero sized string and skip to next one.

Reported-by: xu xin <xu.xin16@zte.com.cn>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 init/do_mounts.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 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-16 11:28:36.753625037 -0400
@@ -338,19 +338,25 @@ __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;
+	bool str_start = false;
 
-	strcpy(p, root_fs_names);
+	strlcpy(p, root_fs_names, size);
 	while (*p++) {
-		if (p[-1] == ',')
+		if (p[-1] == ',') {
 			p[-1] = '\0';
+			count++;
+			str_start = false;
+		} else {
+			str_start = true;
+		}
 	}
-	*p = '\0';
 
-	for (p = page; *p; p += strlen(p)+1)
+	/* Last string which might not be comma terminated */
+	if (str_start)
 		count++;
 
 	return count;
@@ -404,12 +410,16 @@ 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:
 	for (i = 0, p = fs_names; i < num_fs; i++, p += strlen(p)+1) {
-		int err = do_mount_root(name, p, flags, root_mount_data);
+		int err;
+
+		if (!*p)
+			continue;
+		err = do_mount_root(name, p, flags, root_mount_data);
 		switch (err) {
 			case 0:
 				goto out;
@@ -543,10 +553,12 @@ 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) {
+		if (!*fstype)
+			continue;
 		if (!fs_is_nodev(fstype))
 			continue;
 		err = do_mount_root(root_device_name, fstype, root_mountflags,


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] init/do_mounts.c: Harden split_fs_names() against buffer overflow
  2021-09-16 15:50 [PATCH v2] init/do_mounts.c: Harden split_fs_names() against buffer overflow Vivek Goyal
@ 2021-09-17  8:07 ` Jan Kara
  2021-09-17 12:52   ` Vivek Goyal
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2021-09-17  8:07 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Christoph Hellwig, Jan Kara, Christian Brauner, viro,
	Linux fsdevel mailing list, linux kernel mailing list, xu.xin16

On Thu 16-09-21 11:50:58, Vivek Goyal wrote:
> split_fs_names() currently takes comma separate 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.
> 
> Pass size of input buffer to split_fs_names() and put enough checks
> in place so such buffer overrun possibilities do not occur.
> 
> This patch does few things.
> 
> - Add a parameter "size" to split_fs_names(). This specifies size
>   of input buffer.
> 
> - Use strlcpy() (instead of strcpy()) so that we can't go beyond
>   buffer size. If input string "names" is larger than passed in
>   buffer, input string will be truncated to fit in buffer.
> 
> - Stop appending extra '\0' character at the end and avoid one
>   possibility of going beyond the input buffer size.
> 
> - Do not use extra loop to count number of strings.
> 
> - Previously if one passed "rootfstype=foo,,bar", split_fs_names()
>   will return only 1 string "foo" (and "bar" will be truncated
>   due to extra ,). After this patch, now split_fs_names() will
>   return 3 strings ("foo", zero-sized-string, and "bar").
> 
>   Callers of split_fs_names() have been modified to check for
>   zero sized string and skip to next one.
> 
> Reported-by: xu xin <xu.xin16@zte.com.cn>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  init/do_mounts.c |   28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)

Just one nit 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-16 11:28:36.753625037 -0400
> @@ -338,19 +338,25 @@ __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;
> +	bool str_start = false;
>  
> -	strcpy(p, root_fs_names);
> +	strlcpy(p, root_fs_names, size);
>  	while (*p++) {
> -		if (p[-1] == ',')
> +		if (p[-1] == ',') {
>  			p[-1] = '\0';
> +			count++;
> +			str_start = false;
> +		} else {
> +			str_start = true;
> +		}
>  	}
> -	*p = '\0';
>  
> -	for (p = page; *p; p += strlen(p)+1)
> +	/* Last string which might not be comma terminated */
> +	if (str_start)
>  		count++;

You could avoid the whole str_start logic if you just initialize 'count' to
1 - in the worst case you'll have 0-length string at the end (for case like
xfs,) but you deal with 0-length strings in the callers anyway. Otherwise
the patch looks good so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] init/do_mounts.c: Harden split_fs_names() against buffer overflow
  2021-09-17  8:07 ` Jan Kara
@ 2021-09-17 12:52   ` Vivek Goyal
  0 siblings, 0 replies; 3+ messages in thread
From: Vivek Goyal @ 2021-09-17 12:52 UTC (permalink / raw)
  To: Jan Kara
  Cc: Christoph Hellwig, Christian Brauner, viro,
	Linux fsdevel mailing list, linux kernel mailing list, xu.xin16

On Fri, Sep 17, 2021 at 10:07:30AM +0200, Jan Kara wrote:
> On Thu 16-09-21 11:50:58, Vivek Goyal wrote:
> > split_fs_names() currently takes comma separate 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.
> > 
> > Pass size of input buffer to split_fs_names() and put enough checks
> > in place so such buffer overrun possibilities do not occur.
> > 
> > This patch does few things.
> > 
> > - Add a parameter "size" to split_fs_names(). This specifies size
> >   of input buffer.
> > 
> > - Use strlcpy() (instead of strcpy()) so that we can't go beyond
> >   buffer size. If input string "names" is larger than passed in
> >   buffer, input string will be truncated to fit in buffer.
> > 
> > - Stop appending extra '\0' character at the end and avoid one
> >   possibility of going beyond the input buffer size.
> > 
> > - Do not use extra loop to count number of strings.
> > 
> > - Previously if one passed "rootfstype=foo,,bar", split_fs_names()
> >   will return only 1 string "foo" (and "bar" will be truncated
> >   due to extra ,). After this patch, now split_fs_names() will
> >   return 3 strings ("foo", zero-sized-string, and "bar").
> > 
> >   Callers of split_fs_names() have been modified to check for
> >   zero sized string and skip to next one.
> > 
> > Reported-by: xu xin <xu.xin16@zte.com.cn>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  init/do_mounts.c |   28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> Just one nit 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-16 11:28:36.753625037 -0400
> > @@ -338,19 +338,25 @@ __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;
> > +	bool str_start = false;
> >  
> > -	strcpy(p, root_fs_names);
> > +	strlcpy(p, root_fs_names, size);
> >  	while (*p++) {
> > -		if (p[-1] == ',')
> > +		if (p[-1] == ',') {
> >  			p[-1] = '\0';
> > +			count++;
> > +			str_start = false;
> > +		} else {
> > +			str_start = true;
> > +		}
> >  	}
> > -	*p = '\0';
> >  
> > -	for (p = page; *p; p += strlen(p)+1)
> > +	/* Last string which might not be comma terminated */
> > +	if (str_start)
> >  		count++;
> 
> You could avoid the whole str_start logic if you just initialize 'count' to
> 1 - in the worst case you'll have 0-length string at the end (for case like
> xfs,) but you deal with 0-length strings in the callers anyway. Otherwise
> the patch looks good so feel free to add:

Hi Jan,

This sounds good. I will get rid of str_start. V3 of the patch is on
the way.

Vivek
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-17 12:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 15:50 [PATCH v2] init/do_mounts.c: Harden split_fs_names() against buffer overflow Vivek Goyal
2021-09-17  8:07 ` Jan Kara
2021-09-17 12:52   ` Vivek Goyal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).