Re: [patch] ALSA: asihpi: fix an information leak in asihpi_hpi_ioctl()

[Takashi Iwai <tiwai@suse.de>]

At Mon, 22 Dec 2014 10:49:46 +0300,
Dan Carpenter wrote:
> 
> So far as I can see "hr->h.size" is set to "res_max_size" which is a
> user controlled value between 12 and USHRT_MAX.  If it's larger than
> sizeof(*hr), then that leads to an information leak.
> 
> I am not very familiar with this code, my other question here is that
> on lines before we set "hr->h.size = sizeof(hr->h)".  It think this is
> a bug.  I also think this particular code is never executed and I added
> a comment to that effect.  But we do it in earlier in the function as
> well:
> 
> 	copy_to_user(puhr, hr, sizeof(hr->h));
> 
> It doesn't make sense to me.
> 
> Anyway, I think my patch is safe and it seems to fix a real information
> leak.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 6aa677e..f88109a 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -283,6 +283,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		goto out;
>  	}
>  
> +	/* FIXME:  isn't this a no-op? */

The whole union member may be overwritten by a response.

>  	if (hr->h.size > res_max_size) {
>  		HPI_DEBUG_LOG(ERROR, "response too big %d %d\n", hr->h.size,
>  			res_max_size);
> @@ -290,6 +291,9 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		hr->h.specific_error = hr->h.size;
>  		hr->h.size = sizeof(hr->h);
>  	}
> +	/* prevent an information leak */
> +	if (hr->h.size > sizeof(*hr))
> +		hr->h.size = sizeof(*hr);

Checking the size is good, but there is already a check against
res_max_size.  So, we'd rather need to check res_max_size itself
whether it's in a sane range.  The more fitting place would be at the
beginning of the function where it checks already res_max_size <
sizeof(struct hpi_response_header)).


thanks,

Takashi

>  
>  	uncopied_bytes = copy_to_user(puhr, hr, hr->h.size);
>  	if (uncopied_bytes) {
>