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: <20210914202349.GB9406@quack2.suse.cz>
Date:   Tue, 14 Sep 2021 22:23:49 +0200
From:   Jan Kara <jack@...e.cz>
To:     cgel.zte@...il.com
Cc:     hare@...e.de, axboe@...nel.dk, jack@...e.cz, 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>,
        Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH linux-next] init/do_mounts: fix potential memory out of
 bounds access

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.

								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