Re: [PATCH linux-next] init/do_mounts: fix potential memory out of bounds access

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Sep 14, 2021 at 06:41:42PM -0400, Vivek Goyal wrote:
> On Tue, Sep 14, 2021 at 10:23:49PM +0200, Jan Kara wrote:
> > On Mon 13-09-21 11:43:36, cgel.zte@gmail.com wrote:
> > > From: xu xin <xu.xin16@zte.com.cn>
> > > 
> > > Initially the pointer "p" points to the start of "pages".
> > > In the loop "while(*p++) {...}", it ends when "*p" equals
> > > to zero. Just after that, the pointer "p" moves forward
> > > with "p++", so "p" may points ouf of "pages".
> > > 
> > > furthermore, it is no use to set *p = '\0', so we remove it.
> > 
> > Hum, I agree it is somewhat unclear that the assignment cannot go beyond
> > the end of the page although I suspect it cannot happen in practice as that
> > would mean parameter PAGE_SIZE long and I suspect parameter parsing code
> > would refuse that earlier (but don't really know kernel cmdline parsing
> > details).
> > 
> > But what I'm quite sure about is that the assignment is not useless. If you
> > look at the loop below this assignment, you'll notice it terminates on
> > 0-length string and the assignment creates exactly this string at the end
> > of the split parameter. So your patch certainly breaks things.
> 
> Yes, that '\0' at the end is intentional so that we terminate the
> loop right after this assignment and count number of strings and
> return to caller.
> 
> Even before recent changes, get_fs_names() was doing same thing.
> It was adding at '\0' at the end. So behavior has not changed.
> 
> Now question is, is it easily possible to pass root_fs_names big
> enough that it can overflow the page we have assigned. If yes,
> then we can think if putting some safeguards and truncate the
> passed string and not overflow into next page.

Or we could pass "size" to split_fs_names() and make sure it
does not cross page boundary. Something like this. Compile
tested only. Will test tomorrow.

---
 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-14 18:50:13.608554845 -0400
+++ redhat-linux/init/do_mounts.c	2021-09-14 19:08:58.349284067 -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) {