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

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

[Dan Carpenter]

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? */
 	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);
 
 	uncopied_bytes = copy_to_user(puhr, hr, hr->h.size);
 	if (uncopied_bytes) {

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

[Dan Carpenter]

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? */
 	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);
 
 	uncopied_bytes = copy_to_user(puhr, hr, hr->h.size);
 	if (uncopied_bytes) {

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

[Takashi Iwai]

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) {
> 

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

[Takashi Iwai]

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) {
> 

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

[Eliot Blennerhassett]

On 27/12/14 00:07, Takashi Iwai wrote:
> 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)).

Yes.

At that point res_max_size is size of userspace buffer, and *hr is
effectively kernelspace buffer,  so take lowest of the two.

i.e.
    res_max_size = min(res_max_size, sizeof(*hr));

On the way "down" from here hr->h.size is available buffer size for a
response from card DSP, then on the way "up", hr->h.size is the actual
size of the response.  This value comes from DSP by DMA or IO read.

Which leads me to notice another problem in hpi6000.c, where the
response read from DSP is not limited to the given buffer size.

I will follow up with a patch in the next few days.

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


-- 
Eliot

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

[Eliot Blennerhassett]

On 27/12/14 00:07, Takashi Iwai wrote:
> 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)).

Yes.

At that point res_max_size is size of userspace buffer, and *hr is
effectively kernelspace buffer,  so take lowest of the two.

i.e.
    res_max_size = min(res_max_size, sizeof(*hr));

On the way "down" from here hr->h.size is available buffer size for a
response from card DSP, then on the way "up", hr->h.size is the actual
size of the response.  This value comes from DSP by DMA or IO read.

Which leads me to notice another problem in hpi6000.c, where the
response read from DSP is not limited to the given buffer size.

I will follow up with a patch in the next few days.

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


-- 
Eliot

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

[Eliot Blennerhassett]

Add missing limits to keep copied data within allocated buffer.

Signed-off-by: Eliot Blennerhassett <eliot@blennerhassett.gen.nz>
---
 sound/pci/asihpi/hpi6000.c | 6 +++++-
 sound/pci/asihpi/hpioctl.c | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/pci/asihpi/hpi6000.c b/sound/pci/asihpi/hpi6000.c
index e0c6715..794df30 100644
--- a/sound/pci/asihpi/hpi6000.c
+++ b/sound/pci/asihpi/hpi6000.c
@@ -46,6 +46,7 @@
 
 /* operational/messaging errors */
 #define HPI6000_ERROR_MSG_RESP_IDLE_TIMEOUT		901
+#define HPI6000_ERROR_RESP_GET_LEN			902
 #define HPI6000_ERROR_MSG_RESP_GET_RESP_ACK		903
 #define HPI6000_ERROR_MSG_GET_ADR			904
 #define HPI6000_ERROR_RESP_GET_ADR			905
@@ -1363,7 +1364,10 @@ static short hpi6000_message_response_sequence(struct hpi_adapter_obj *pao,
 		length = hpi_read_word(pdo, HPI_HIF_ADDR(length));
 	} while (hpi6000_check_PCI2040_error_flag(pao, H6READ) && --timeout);
 	if (!timeout)
-		length = sizeof(struct hpi_response);
+		return HPI6000_ERROR_RESP_GET_LEN;
+
+	if (length > phr->size)
+		return HPI_ERROR_RESPONSE_BUFFER_TOO_SMALL;
 
 	/* get the response */
 	p_data = (u32 *)phr;
diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
index 6aa677e..72af66b 100644
--- a/sound/pci/asihpi/hpioctl.c
+++ b/sound/pci/asihpi/hpioctl.c
@@ -153,6 +153,8 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		goto out;
 	}
 
+	res_max_size = min_t(size_t, res_max_size, sizeof(*hr));
+
 	switch (hm->h.function) {
 	case HPI_SUBSYS_CREATE_ADAPTER:
 	case HPI_ADAPTER_DELETE:
-- 
1.9.1

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

[Eliot Blennerhassett]

Add missing limits to keep copied data within allocated buffer.

Signed-off-by: Eliot Blennerhassett <eliot@blennerhassett.gen.nz>
---
 sound/pci/asihpi/hpi6000.c | 6 +++++-
 sound/pci/asihpi/hpioctl.c | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/pci/asihpi/hpi6000.c b/sound/pci/asihpi/hpi6000.c
index e0c6715..794df30 100644
--- a/sound/pci/asihpi/hpi6000.c
+++ b/sound/pci/asihpi/hpi6000.c
@@ -46,6 +46,7 @@
 
 /* operational/messaging errors */
 #define HPI6000_ERROR_MSG_RESP_IDLE_TIMEOUT		901
+#define HPI6000_ERROR_RESP_GET_LEN			902
 #define HPI6000_ERROR_MSG_RESP_GET_RESP_ACK		903
 #define HPI6000_ERROR_MSG_GET_ADR			904
 #define HPI6000_ERROR_RESP_GET_ADR			905
@@ -1363,7 +1364,10 @@ static short hpi6000_message_response_sequence(struct hpi_adapter_obj *pao,
 		length = hpi_read_word(pdo, HPI_HIF_ADDR(length));
 	} while (hpi6000_check_PCI2040_error_flag(pao, H6READ) && --timeout);
 	if (!timeout)
-		length = sizeof(struct hpi_response);
+		return HPI6000_ERROR_RESP_GET_LEN;
+
+	if (length > phr->size)
+		return HPI_ERROR_RESPONSE_BUFFER_TOO_SMALL;
 
 	/* get the response */
 	p_data = (u32 *)phr;
diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
index 6aa677e..72af66b 100644
--- a/sound/pci/asihpi/hpioctl.c
+++ b/sound/pci/asihpi/hpioctl.c
@@ -153,6 +153,8 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		goto out;
 	}
 
+	res_max_size = min_t(size_t, res_max_size, sizeof(*hr));
+
 	switch (hm->h.function) {
 	case HPI_SUBSYS_CREATE_ADAPTER:
 	case HPI_ADAPTER_DELETE:
-- 
1.9.1

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

[Takashi Iwai]

At Wed, 31 Dec 2014 19:26:51 +1300,
Eliot Blennerhassett wrote:
> 
> Add missing limits to keep copied data within allocated buffer.
> 
> Signed-off-by: Eliot Blennerhassett <eliot@blennerhassett.gen.nz>

hpi6000.c changes can't be applied.  I guess it's for your development
branch?

Please split and send the currently applicable one (for hpioctl.c) for
merging to 3.19-rc kernel, and include the rest to the next update
batch.


thanks,

Takashi

> ---
>  sound/pci/asihpi/hpi6000.c | 6 +++++-
>  sound/pci/asihpi/hpioctl.c | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/asihpi/hpi6000.c b/sound/pci/asihpi/hpi6000.c
> index e0c6715..794df30 100644
> --- a/sound/pci/asihpi/hpi6000.c
> +++ b/sound/pci/asihpi/hpi6000.c
> @@ -46,6 +46,7 @@
>  
>  /* operational/messaging errors */
>  #define HPI6000_ERROR_MSG_RESP_IDLE_TIMEOUT		901
> +#define HPI6000_ERROR_RESP_GET_LEN			902
>  #define HPI6000_ERROR_MSG_RESP_GET_RESP_ACK		903
>  #define HPI6000_ERROR_MSG_GET_ADR			904
>  #define HPI6000_ERROR_RESP_GET_ADR			905
> @@ -1363,7 +1364,10 @@ static short hpi6000_message_response_sequence(struct hpi_adapter_obj *pao,
>  		length = hpi_read_word(pdo, HPI_HIF_ADDR(length));
>  	} while (hpi6000_check_PCI2040_error_flag(pao, H6READ) && --timeout);
>  	if (!timeout)
> -		length = sizeof(struct hpi_response);
> +		return HPI6000_ERROR_RESP_GET_LEN;
> +
> +	if (length > phr->size)
> +		return HPI_ERROR_RESPONSE_BUFFER_TOO_SMALL;
>  
>  	/* get the response */
>  	p_data = (u32 *)phr;
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 6aa677e..72af66b 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -153,6 +153,8 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		goto out;
>  	}
>  
> +	res_max_size = min_t(size_t, res_max_size, sizeof(*hr));
> +
>  	switch (hm->h.function) {
>  	case HPI_SUBSYS_CREATE_ADAPTER:
>  	case HPI_ADAPTER_DELETE:
> -- 
> 1.9.1
> 
> 

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

[Takashi Iwai]

At Wed, 31 Dec 2014 19:26:51 +1300,
Eliot Blennerhassett wrote:
> 
> Add missing limits to keep copied data within allocated buffer.
> 
> Signed-off-by: Eliot Blennerhassett <eliot@blennerhassett.gen.nz>

hpi6000.c changes can't be applied.  I guess it's for your development
branch?

Please split and send the currently applicable one (for hpioctl.c) for
merging to 3.19-rc kernel, and include the rest to the next update
batch.


thanks,

Takashi

> ---
>  sound/pci/asihpi/hpi6000.c | 6 +++++-
>  sound/pci/asihpi/hpioctl.c | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/asihpi/hpi6000.c b/sound/pci/asihpi/hpi6000.c
> index e0c6715..794df30 100644
> --- a/sound/pci/asihpi/hpi6000.c
> +++ b/sound/pci/asihpi/hpi6000.c
> @@ -46,6 +46,7 @@
>  
>  /* operational/messaging errors */
>  #define HPI6000_ERROR_MSG_RESP_IDLE_TIMEOUT		901
> +#define HPI6000_ERROR_RESP_GET_LEN			902
>  #define HPI6000_ERROR_MSG_RESP_GET_RESP_ACK		903
>  #define HPI6000_ERROR_MSG_GET_ADR			904
>  #define HPI6000_ERROR_RESP_GET_ADR			905
> @@ -1363,7 +1364,10 @@ static short hpi6000_message_response_sequence(struct hpi_adapter_obj *pao,
>  		length = hpi_read_word(pdo, HPI_HIF_ADDR(length));
>  	} while (hpi6000_check_PCI2040_error_flag(pao, H6READ) && --timeout);
>  	if (!timeout)
> -		length = sizeof(struct hpi_response);
> +		return HPI6000_ERROR_RESP_GET_LEN;
> +
> +	if (length > phr->size)
> +		return HPI_ERROR_RESPONSE_BUFFER_TOO_SMALL;
>  
>  	/* get the response */
>  	p_data = (u32 *)phr;
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 6aa677e..72af66b 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -153,6 +153,8 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		goto out;
>  	}
>  
> +	res_max_size = min_t(size_t, res_max_size, sizeof(*hr));
> +
>  	switch (hm->h.function) {
>  	case HPI_SUBSYS_CREATE_ADAPTER:
>  	case HPI_ADAPTER_DELETE:
> -- 
> 1.9.1
> 
> 

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

[Takashi Iwai]

At Wed, 31 Dec 2014 23:48:32 +1300,
Eliot Blennerhassett wrote:
> 
> Add missing limits to keep copied data within allocated buffer.
> 
> Signed-off-by: Eliot Blennerhassett <eliot@blennerhassett.gen.nz>
> ---
> Regenerated, this should apply cleanly to for-next

OK, applied now.  Thanks.


Takashi

> 
>  sound/pci/asihpi/hpi6000.c | 7 +++++--
>  sound/pci/asihpi/hpioctl.c | 2 ++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/asihpi/hpi6000.c b/sound/pci/asihpi/hpi6000.c
> index 2414d7a..2d63648 100644
> --- a/sound/pci/asihpi/hpi6000.c
> +++ b/sound/pci/asihpi/hpi6000.c
> @@ -47,7 +47,7 @@
>  
>  /* operational/messaging errors */
>  #define HPI6000_ERROR_MSG_RESP_IDLE_TIMEOUT             901
> -
> +#define HPI6000_ERROR_RESP_GET_LEN                      902
>  #define HPI6000_ERROR_MSG_RESP_GET_RESP_ACK             903
>  #define HPI6000_ERROR_MSG_GET_ADR                       904
>  #define HPI6000_ERROR_RESP_GET_ADR                      905
> @@ -1365,7 +1365,10 @@ static short hpi6000_message_response_sequence(struct hpi_adapter_obj *pao,
>  		length = hpi_read_word(pdo, HPI_HIF_ADDR(length));
>  	} while (hpi6000_check_PCI2040_error_flag(pao, H6READ) && --timeout);
>  	if (!timeout)
> -		length = sizeof(struct hpi_response);
> +		return HPI6000_ERROR_RESP_GET_LEN;
> +
> +	if (length > phr->size)
> +		return HPI_ERROR_RESPONSE_BUFFER_TOO_SMALL;
>  
>  	/* get the response */
>  	p_data = (u32 *)phr;
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 6aa677e..72af66b 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -153,6 +153,8 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		goto out;
>  	}
>  
> +	res_max_size = min_t(size_t, res_max_size, sizeof(*hr));
> +
>  	switch (hm->h.function) {
>  	case HPI_SUBSYS_CREATE_ADAPTER:
>  	case HPI_ADAPTER_DELETE:
> -- 
> 1.9.1
> 
> 

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

[Dan Carpenter]

On Wed, Dec 31, 2014 at 07:26:51PM +1300, Eliot Blennerhassett wrote:
> Add missing limits to keep copied data within allocated buffer.
> 

Could you give me a Reported-by tag for this?

regards,
dan carpenter

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

[Dan Carpenter]

On Wed, Dec 31, 2014 at 07:26:51PM +1300, Eliot Blennerhassett wrote:
> Add missing limits to keep copied data within allocated buffer.
> 

Could you give me a Reported-by tag for this?

regards,
dan carpenter

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

[Takashi Iwai]

At Mon, 5 Jan 2015 12:33:22 +0300,
Dan Carpenter wrote:
> 
> On Wed, Dec 31, 2014 at 07:26:51PM +1300, Eliot Blennerhassett wrote:
> > Add missing limits to keep copied data within allocated buffer.
> > 
> 
> Could you give me a Reported-by tag for this?

Already done.


Takashi

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

[Takashi Iwai]

At Mon, 5 Jan 2015 12:33:22 +0300,
Dan Carpenter wrote:
> 
> On Wed, Dec 31, 2014 at 07:26:51PM +1300, Eliot Blennerhassett wrote:
> > Add missing limits to keep copied data within allocated buffer.
> > 
> 
> Could you give me a Reported-by tag for this?

Already done.


Takashi