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

[Jan Kara]

On Mon 13-09-21 11:43:36, cgel.zte@xxxxxxxxx wrote:
> From: xu xin <xu.xin16@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Acked-by: zhang yunkai<zhang.yunkai@xxxxxxxxxx>
> Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> ---
>  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@xxxxxxxx>
SUSE Labs, CR