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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 14 Sep 2021 19:14:20 -0400
From:   Vivek Goyal <vgoyal@...hat.com>
To:     Jan Kara <jack@...e.cz>
Cc:     cgel.zte@...il.com, hare@...e.de, axboe@...nel.dk, tj@...nel.org,
        viro@...iv.linux.org.uk, xu.xin16@....com.cn,
        linux-kernel@...r.kernel.org, Zeal Robot <zealci@....com.cn>,
        zhang yunkai <zhang.yunkai@....com.cn>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH linux-next] init/do_mounts: fix potential memory out of
 bounds access

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@...il.com wrote:
> > > From: xu xin <xu.xin16@....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) {