Re: [PATCH] drivers/md/md.c: use kzalloc() when bitmap is disabled

[NeilBrown <neilb@suse.com>]

On Sat, 25 Jul 2015 16:36:50 +0200 Benjamin Randazzo
<benjamin@randazzo.fr> wrote:

> In drivers/md/md.c get_bitmap_file() uses kmalloc() for creating a
> mdu_bitmap_file_t called "file".
> 
> 5769         file = kmalloc(sizeof(*file), GFP_NOIO);
> 5770         if (!file)
> 5771                 return -ENOMEM;
> 
> This structure is copied to user space at the end of the function.
> 
> 5786         if (err == 0 &&
> 5787             copy_to_user(arg, file, sizeof(*file)))
> 5788                 err = -EFAULT
> 
> But if bitmap is disabled only the first byte of "file" is initialized
> with zero, so it's possible to read some bytes (up to 4095) of kernel
> space memory from user space. This is an information leak.
> 
> 5775         /* bitmap disabled, zero the first byte and copy out */
> 5776         if (!mddev->bitmap_info.file)
> 5777                 file->pathname[0] = '\0';
> 
> Signed-off-by: Benjamin Randazzo <benjamin@randazzo.fr>
> ---
>  drivers/md/md.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 80879dc..382bdbc 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5766,22 +5766,21 @@ static int get_bitmap_file(struct mddev *mddev, void __user * arg)
>  	char *ptr;
>  	int err;
>  
> -	file = kmalloc(sizeof(*file), GFP_NOIO);
> +	file = kzalloc(sizeof(*file), GFP_NOIO);
>  	if (!file)
>  		return -ENOMEM;
>  
>  	err = 0;
>  	spin_lock(&mddev->lock);
> -	/* bitmap disabled, zero the first byte and copy out */
> -	if (!mddev->bitmap_info.file)
> -		file->pathname[0] = '\0';
> -	else if ((ptr = file_path(mddev->bitmap_info.file,
> -			       file->pathname, sizeof(file->pathname))),
> -		 IS_ERR(ptr))
> -		err = PTR_ERR(ptr);
> -	else
> -		memmove(file->pathname, ptr,
> -			sizeof(file->pathname)-(ptr-file->pathname));
> +	/* bitmap enabled */
> +	if (mddev->bitmap_info.file) {
> +		if ((ptr = file_path(mddev->bitmap_info.file, file->pathname,
> +					sizeof(file->pathname))), IS_ERR(ptr))
> +			err = PTR_ERR(ptr);
> +		else
> +			memmove(file->pathname, ptr,
> +				sizeof(file->pathname)-(ptr-file->pathname));
> +	}
>  	spin_unlock(&mddev->lock);
>  
>  	if (err == 0 &&


Thanks.
I re-arranged the code a little bit more as there is no longer any
excuse for having the "ptr = file_path()" assignment inside the
condition of the 'if'.

Applied.  Thanks,
NeilBrown