Messages in this thread Patch in this message | | | Date | Tue, 14 Sep 2021 19:14:20 -0400 | From | Vivek Goyal <> | 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@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) {
| |