Skip to contentSkip to conversation

ALSA: asihpi: fix an information leak in asihpi_hpi_ioctl()

PATCH In review Dan Carpenter 3 participants 8 messages Dec 22, 2014 – Jan 5, 2015 1 message not in the archive

Subsystems (2)
Sound Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ALSA: asihpi: fix an information leak in asihpi_hpi_ioctl()

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 <[email protected]>
sound/pci/asihpi/hpioctl.c +4 -0
index 6aa677e..f88109a 100644
@@ -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) {
#1 lore raw

At Mon, 22 Dec 2014 10:49:46 +0300,

Quoted 25 lines · 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 <[email protected]>
sound/pci/asihpi/hpioctl.c +0 -0
index 6aa677e..f88109a 100644

sound/pci/asihpi/hpioctl.c long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 286

Quoted 4 lines
sound/pci/asihpi/hpioctl.c +1 -0
@@ -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.

sound/pci/asihpi/hpioctl.c long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 296

Quoted 10 lines
sound/pci/asihpi/hpioctl.c +3 -0
 	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

Quoted 4 lines
sound/pci/asihpi/hpioctl.c +0 -0
 	uncopied_bytes = copy_to_user(puhr, hr, hr->h.size);
 	if (uncopied_bytes) {
#2 lore raw
Quoted 34 lines · Takashi Iwai

At Mon, 22 Dec 2014 10:49:46 +0300,

Quoted 25 lines · 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 <[email protected]>
sound/pci/asihpi/hpioctl.c +0 -0
index 6aa677e..f88109a 100644

sound/pci/asihpi/hpioctl.c long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 286

Quoted 4 lines
sound/pci/asihpi/hpioctl.c +1 -0
@@ -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.

Quoted 16 lines

sound/pci/asihpi/hpioctl.c long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 296

Quoted 10 lines
sound/pci/asihpi/hpioctl.c +3 -0
 	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.

Quoted 10 lines

thanks,

Takashi

Quoted 4 lines
sound/pci/asihpi/hpioctl.c +0 -0
 	uncopied_bytes = copy_to_user(puhr, hr, hr->h.size);
 	if (uncopied_bytes) {
Signature · 1 line
Eliot
#3 lore raw

Add missing limits to keep copied data within allocated buffer.

Signed-off-by Eliot Blennerhassett <[email protected]>
---
 sound/pci/asihpi/hpi6000.c | 6 +++++-
 sound/pci/asihpi/hpioctl.c | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)
sound/pci/asihpi/hpi6000.c +5 -1
index e0c6715..794df30 100644
@@ -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;
sound/pci/asihpi/hpioctl.c +2 -0
index 6aa677e..72af66b 100644
@@ -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:
Signature · 1 line
1.9.1
#4 lore raw

At Wed, 31 Dec 2014 19:26:51 +1300,

Quoted 4 lines · Eliot Blennerhassett

Add missing limits to keep copied data within allocated buffer.

Signed-off-by Eliot Blennerhassett <[email protected]>

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

Quoted 46 lines
---
 sound/pci/asihpi/hpi6000.c | 6 +++++-
 sound/pci/asihpi/hpioctl.c | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)
sound/pci/asihpi/hpi6000.c +5 -1
index e0c6715..794df30 100644
@@ -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;
sound/pci/asihpi/hpioctl.c +2 -0
index 6aa677e..72af66b 100644
@@ -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:
Signature · 1 line
1.9.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw

At Wed, 31 Dec 2014 23:48:32 +1300,

Quoted 6 lines · Eliot Blennerhassett

Add missing limits to keep copied data within allocated buffer.

Signed-off-by Eliot Blennerhassett <[email protected]>

---
Regenerated, this should apply cleanly to for-next

OK, applied now. Thanks.

Takashi

Quoted 47 lines
 sound/pci/asihpi/hpi6000.c | 7 +++++--
 sound/pci/asihpi/hpioctl.c | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)
sound/pci/asihpi/hpi6000.c +5 -2
index 2414d7a..2d63648 100644
@@ -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;
sound/pci/asihpi/hpioctl.c +2 -0
index 6aa677e..72af66b 100644
@@ -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:
Signature · 1 line
1.9.1
#8 lore raw

At Mon, 5 Jan 2015 12:33:22 +0300,

Quoted 6 lines · Dan Carpenter
Quoted 2 lines · Eliot Blennerhassett

Add missing limits to keep copied data within allocated buffer.

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

Already done.

Takashi