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