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

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