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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YUElJqNI9VVL/SI/@redhat.com>
Date:   Tue, 14 Sep 2021 18:41:42 -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 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.

Thanks
Vivek



> 
> 								Honza
> 
> > 
> > Reported-by: Zeal Robot <zealci@....com.cn>
> > Acked-by: zhang yunkai<zhang.yunkai@....com.cn>
> > Signed-off-by: xu xin <xu.xin16@....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@...e.com>
> SUSE Labs, CR
>