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

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

[cgel.zte]

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.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Acked-by: zhang yunkai<zhang.yunkai@zte.com.cn>
Signed-off-by: xu xin <xu.xin16@zte.com.cn>
---
 init/do_mounts.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index 2ed30ff6c906..ee1172599249 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -348,7 +348,6 @@ static int __init split_fs_names(char *page, char *names)
 		if (p[-1] == ',')
 			p[-1] = '\0';
 	}
-	*p = '\0';
 
 	for (p = page; *p; p += strlen(p)+1)
 		count++;
-- 
2.25.1

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

[Jan Kara]

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.

								Honza

> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Acked-by: zhang yunkai<zhang.yunkai@zte.com.cn>
> Signed-off-by: xu xin <xu.xin16@zte.com.cn>
> ---
>  init/do_mounts.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/init/do_mounts.c b/init/do_mounts.c
> index 2ed30ff6c906..ee1172599249 100644
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -348,7 +348,6 @@ static int __init split_fs_names(char *page, char *names)
>  		if (p[-1] == ',')
>  			p[-1] = '\0';
>  	}
> -	*p = '\0';
>  
>  	for (p = page; *p; p += strlen(p)+1)
>  		count++;
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Vivek Goyal]

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.

Thanks
Vivek



> 
> 								Honza
> 
> > 
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Acked-by: zhang yunkai<zhang.yunkai@zte.com.cn>
> > Signed-off-by: xu xin <xu.xin16@zte.com.cn>
> > ---
> >  init/do_mounts.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/init/do_mounts.c b/init/do_mounts.c
> > index 2ed30ff6c906..ee1172599249 100644
> > --- a/init/do_mounts.c
> > +++ b/init/do_mounts.c
> > @@ -348,7 +348,6 @@ static int __init split_fs_names(char *page, char *names)
> >  		if (p[-1] == ',')
> >  			p[-1] = '\0';
> >  	}
> > -	*p = '\0';
> >  
> >  	for (p = page; *p; p += strlen(p)+1)
> >  		count++;
> > -- 
> > 2.25.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> 

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

[Vivek Goyal]

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) {