* [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm
@ 2017-09-19 5:21 Meng Xu
2017-09-19 7:27 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Meng Xu @ 2017-09-19 5:21 UTC (permalink / raw)
To: perex, tiwai, vlad, alsa-devel, linux-kernel
Cc: meng.xu, sanidhya, taesoo, Meng Xu
The hm->h.size is intended to hold the actual size of the hm struct
that is copied from userspace and should always be <= sizeof(*hm).
However, after copy_from_user(hm, puhm, hm->h.size), since userspace
process has full control over the memory region pointed by puhm, it is
possible that the value of hm->h.size is different from what is fetched-in
previously (get_user(hm->h.size, (u16 __user *)puhm)). In other words,
hm->h.size is overriden and the relation between hm->h.size and the hm
struct is broken.
This patch proposes to use a seperate variable, msg_size, to hold
the value of the first fetch and override hm->h.size to msg_size
after the second fetch to maintain the relation.
Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
---
sound/pci/asihpi/hpioctl.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
index 7e3aa50..5badd08 100644
--- a/sound/pci/asihpi/hpioctl.c
+++ b/sound/pci/asihpi/hpioctl.c
@@ -103,6 +103,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
void __user *puhr;
union hpi_message_buffer_v1 *hm;
union hpi_response_buffer_v1 *hr;
+ u16 msg_size;
u16 res_max_size;
u32 uncopied_bytes;
int err = 0;
@@ -127,22 +128,25 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
}
/* Now read the message size and data from user space. */
- if (get_user(hm->h.size, (u16 __user *)puhm)) {
+ if (get_user(msg_size, (u16 __user *)puhm)) {
err = -EFAULT;
goto out;
}
- if (hm->h.size > sizeof(*hm))
- hm->h.size = sizeof(*hm);
+ if (msg_size > sizeof(*hm))
+ msg_size = sizeof(*hm);
/* printk(KERN_INFO "message size %d\n", hm->h.wSize); */
- uncopied_bytes = copy_from_user(hm, puhm, hm->h.size);
+ uncopied_bytes = copy_from_user(hm, puhm, msg_size);
if (uncopied_bytes) {
HPI_DEBUG_LOG(ERROR, "uncopied bytes %d\n", uncopied_bytes);
err = -EFAULT;
goto out;
}
+ /* Override h.size in case it is changed between two userspace fetches */
+ hm->h.size = msg_size;
+
if (get_user(res_max_size, (u16 __user *)puhr)) {
err = -EFAULT;
goto out;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm
2017-09-19 5:21 [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm Meng Xu
@ 2017-09-19 7:27 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2017-09-19 7:27 UTC (permalink / raw)
To: Meng Xu; +Cc: alsa-devel, perex, vlad, linux-kernel, meng.xu, sanidhya, taesoo
On Tue, 19 Sep 2017 07:21:56 +0200,
Meng Xu wrote:
>
> The hm->h.size is intended to hold the actual size of the hm struct
> that is copied from userspace and should always be <= sizeof(*hm).
>
> However, after copy_from_user(hm, puhm, hm->h.size), since userspace
> process has full control over the memory region pointed by puhm, it is
> possible that the value of hm->h.size is different from what is fetched-in
> previously (get_user(hm->h.size, (u16 __user *)puhm)). In other words,
> hm->h.size is overriden and the relation between hm->h.size and the hm
> struct is broken.
>
> This patch proposes to use a seperate variable, msg_size, to hold
> the value of the first fetch and override hm->h.size to msg_size
> after the second fetch to maintain the relation.
>
> Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
But when user-space already changes the data, the data being read is
more or less broken in anyway no matter whether we keep the original
h.size or not, because it doesn't match with h.size, no?
I'd take a fix patch if it would fix some out-of-bounds access or such
severe issues. But this sounds like covering a corner-case that is
broken in anyway. Or am I missing something else?
thanks,
Takashi
> ---
> sound/pci/asihpi/hpioctl.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 7e3aa50..5badd08 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -103,6 +103,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> void __user *puhr;
> union hpi_message_buffer_v1 *hm;
> union hpi_response_buffer_v1 *hr;
> + u16 msg_size;
> u16 res_max_size;
> u32 uncopied_bytes;
> int err = 0;
> @@ -127,22 +128,25 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> }
>
> /* Now read the message size and data from user space. */
> - if (get_user(hm->h.size, (u16 __user *)puhm)) {
> + if (get_user(msg_size, (u16 __user *)puhm)) {
> err = -EFAULT;
> goto out;
> }
> - if (hm->h.size > sizeof(*hm))
> - hm->h.size = sizeof(*hm);
> + if (msg_size > sizeof(*hm))
> + msg_size = sizeof(*hm);
>
> /* printk(KERN_INFO "message size %d\n", hm->h.wSize); */
>
> - uncopied_bytes = copy_from_user(hm, puhm, hm->h.size);
> + uncopied_bytes = copy_from_user(hm, puhm, msg_size);
> if (uncopied_bytes) {
> HPI_DEBUG_LOG(ERROR, "uncopied bytes %d\n", uncopied_bytes);
> err = -EFAULT;
> goto out;
> }
>
> + /* Override h.size in case it is changed between two userspace fetches */
> + hm->h.size = msg_size;
> +
> if (get_user(res_max_size, (u16 __user *)puhr)) {
> err = -EFAULT;
> goto out;
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm
@ 2017-09-19 7:27 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2017-09-19 7:27 UTC (permalink / raw)
To: Meng Xu; +Cc: alsa-devel, perex, vlad, linux-kernel, meng.xu, sanidhya, taesoo
On Tue, 19 Sep 2017 07:21:56 +0200,
Meng Xu wrote:
>
> The hm->h.size is intended to hold the actual size of the hm struct
> that is copied from userspace and should always be <= sizeof(*hm).
>
> However, after copy_from_user(hm, puhm, hm->h.size), since userspace
> process has full control over the memory region pointed by puhm, it is
> possible that the value of hm->h.size is different from what is fetched-in
> previously (get_user(hm->h.size, (u16 __user *)puhm)). In other words,
> hm->h.size is overriden and the relation between hm->h.size and the hm
> struct is broken.
>
> This patch proposes to use a seperate variable, msg_size, to hold
> the value of the first fetch and override hm->h.size to msg_size
> after the second fetch to maintain the relation.
>
> Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
But when user-space already changes the data, the data being read is
more or less broken in anyway no matter whether we keep the original
h.size or not, because it doesn't match with h.size, no?
I'd take a fix patch if it would fix some out-of-bounds access or such
severe issues. But this sounds like covering a corner-case that is
broken in anyway. Or am I missing something else?
thanks,
Takashi
> ---
> sound/pci/asihpi/hpioctl.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 7e3aa50..5badd08 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -103,6 +103,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> void __user *puhr;
> union hpi_message_buffer_v1 *hm;
> union hpi_response_buffer_v1 *hr;
> + u16 msg_size;
> u16 res_max_size;
> u32 uncopied_bytes;
> int err = 0;
> @@ -127,22 +128,25 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> }
>
> /* Now read the message size and data from user space. */
> - if (get_user(hm->h.size, (u16 __user *)puhm)) {
> + if (get_user(msg_size, (u16 __user *)puhm)) {
> err = -EFAULT;
> goto out;
> }
> - if (hm->h.size > sizeof(*hm))
> - hm->h.size = sizeof(*hm);
> + if (msg_size > sizeof(*hm))
> + msg_size = sizeof(*hm);
>
> /* printk(KERN_INFO "message size %d\n", hm->h.wSize); */
>
> - uncopied_bytes = copy_from_user(hm, puhm, hm->h.size);
> + uncopied_bytes = copy_from_user(hm, puhm, msg_size);
> if (uncopied_bytes) {
> HPI_DEBUG_LOG(ERROR, "uncopied bytes %d\n", uncopied_bytes);
> err = -EFAULT;
> goto out;
> }
>
> + /* Override h.size in case it is changed between two userspace fetches */
> + hm->h.size = msg_size;
> +
> if (get_user(res_max_size, (u16 __user *)puhr)) {
> err = -EFAULT;
> goto out;
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm
2017-09-19 7:27 ` Takashi Iwai
(?)
@ 2017-09-19 13:54 ` Meng Xu
2017-09-19 20:04 ` Takashi Iwai
-1 siblings, 1 reply; 6+ messages in thread
From: Meng Xu @ 2017-09-19 13:54 UTC (permalink / raw)
To: Takashi Iwai, Meng Xu
Cc: alsa-devel, perex, vlad, linux-kernel, sanidhya, taesoo
Hi Takashi,
Thanks for the reply. In my opinion, many security issues
are in fact unhandled corner cases and this could be one.
In the first fetch, get_user(hm->h.size, (u16 __user *)puhm),
only 2 bytes from puhm are copied in and later it is ensured
that hm->h.size (which is also hm->m0.size given hm is a union)
is no larger than sizeof(*hm). However, this relation is broken
after the second fetch, copy_from_user(hm, puhm, hm->h.size).
As a concrete example, a user could put 0x000A when the first
fetch happens which make hm->h.size <= sizeof(*hm) and later
races to change it to 0xFFFF in the second fetch. What makes it
even worse is this call: hpi_send_recv_f(&hm->m0, &hr->r0, file),
which sends &hm->m0 to a lot of destinations. If any of the
downstream functions assumes that hm->m0.size <= sizeof(*hm)
which is actually not, an exploit can be constructed.
In fact, similar issues have caused vulnerabilities before as in
https://bugzilla.kernel.org/show_bug.cgi?id=116651
https://bugzilla.kernel.org/show_bug.cgi?id=120131,
and more recently the fix in sched/perf
https://marc.info/?l=linux-kernel&m=150401225812533&w=2
Feel free to let us know your opinion.
Best Regards,
Meng
On 09/19/2017 03:27 AM, Takashi Iwai wrote:
> On Tue, 19 Sep 2017 07:21:56 +0200,
> Meng Xu wrote:
>> The hm->h.size is intended to hold the actual size of the hm struct
>> that is copied from userspace and should always be <= sizeof(*hm).
>>
>> However, after copy_from_user(hm, puhm, hm->h.size), since userspace
>> process has full control over the memory region pointed by puhm, it is
>> possible that the value of hm->h.size is different from what is fetched-in
>> previously (get_user(hm->h.size, (u16 __user *)puhm)). In other words,
>> hm->h.size is overriden and the relation between hm->h.size and the hm
>> struct is broken.
>>
>> This patch proposes to use a seperate variable, msg_size, to hold
>> the value of the first fetch and override hm->h.size to msg_size
>> after the second fetch to maintain the relation.
>>
>> Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
> But when user-space already changes the data, the data being read is
> more or less broken in anyway no matter whether we keep the original
> h.size or not, because it doesn't match with h.size, no?
>
> I'd take a fix patch if it would fix some out-of-bounds access or such
> severe issues. But this sounds like covering a corner-case that is
> broken in anyway. Or am I missing something else?
>
>
> thanks,
>
> Takashi
>
>> ---
>> sound/pci/asihpi/hpioctl.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
>> index 7e3aa50..5badd08 100644
>> --- a/sound/pci/asihpi/hpioctl.c
>> +++ b/sound/pci/asihpi/hpioctl.c
>> @@ -103,6 +103,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> void __user *puhr;
>> union hpi_message_buffer_v1 *hm;
>> union hpi_response_buffer_v1 *hr;
>> + u16 msg_size;
>> u16 res_max_size;
>> u32 uncopied_bytes;
>> int err = 0;
>> @@ -127,22 +128,25 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> }
>>
>> /* Now read the message size and data from user space. */
>> - if (get_user(hm->h.size, (u16 __user *)puhm)) {
>> + if (get_user(msg_size, (u16 __user *)puhm)) {
>> err = -EFAULT;
>> goto out;
>> }
>> - if (hm->h.size > sizeof(*hm))
>> - hm->h.size = sizeof(*hm);
>> + if (msg_size > sizeof(*hm))
>> + msg_size = sizeof(*hm);
>>
>> /* printk(KERN_INFO "message size %d\n", hm->h.wSize); */
>>
>> - uncopied_bytes = copy_from_user(hm, puhm, hm->h.size);
>> + uncopied_bytes = copy_from_user(hm, puhm, msg_size);
>> if (uncopied_bytes) {
>> HPI_DEBUG_LOG(ERROR, "uncopied bytes %d\n", uncopied_bytes);
>> err = -EFAULT;
>> goto out;
>> }
>>
>> + /* Override h.size in case it is changed between two userspace fetches */
>> + hm->h.size = msg_size;
>> +
>> if (get_user(res_max_size, (u16 __user *)puhr)) {
>> err = -EFAULT;
>> goto out;
>> --
>> 2.7.4
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm
2017-09-19 13:54 ` Meng Xu
@ 2017-09-19 20:04 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2017-09-19 20:04 UTC (permalink / raw)
To: Meng Xu; +Cc: Meng Xu, alsa-devel, perex, vlad, linux-kernel, sanidhya, taesoo
On Tue, 19 Sep 2017 15:54:22 +0200,
Meng Xu wrote:
>
> Hi Takashi,
>
> Thanks for the reply. In my opinion, many security issues
> are in fact unhandled corner cases and this could be one.
>
> In the first fetch, get_user(hm->h.size, (u16 __user *)puhm),
> only 2 bytes from puhm are copied in and later it is ensured
> that hm->h.size (which is also hm->m0.size given hm is a union)
> is no larger than sizeof(*hm). However, this relation is broken
> after the second fetch, copy_from_user(hm, puhm, hm->h.size).
>
> As a concrete example, a user could put 0x000A when the first
> fetch happens which make hm->h.size <= sizeof(*hm) and later
> races to change it to 0xFFFF in the second fetch. What makes it
> even worse is this call: hpi_send_recv_f(&hm->m0, &hr->r0, file),
> which sends &hm->m0 to a lot of destinations. If any of the
> downstream functions assumes that hm->m0.size <= sizeof(*hm)
> which is actually not, an exploit can be constructed.
>
> In fact, similar issues have caused vulnerabilities before as in
> https://bugzilla.kernel.org/show_bug.cgi?id=116651
> https://bugzilla.kernel.org/show_bug.cgi?id=120131,
> and more recently the fix in sched/perf
> https://marc.info/?l=linux-kernel&m=150401225812533&w=2
>
> Feel free to let us know your opinion.
OK, now it's clearer, it's about the overwrite by the second
copy_from_user() call. I took the patch as is now.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm
@ 2017-09-19 20:04 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2017-09-19 20:04 UTC (permalink / raw)
To: Meng Xu; +Cc: alsa-devel, taesoo, sanidhya, linux-kernel, Meng Xu, vlad
On Tue, 19 Sep 2017 15:54:22 +0200,
Meng Xu wrote:
>
> Hi Takashi,
>
> Thanks for the reply. In my opinion, many security issues
> are in fact unhandled corner cases and this could be one.
>
> In the first fetch, get_user(hm->h.size, (u16 __user *)puhm),
> only 2 bytes from puhm are copied in and later it is ensured
> that hm->h.size (which is also hm->m0.size given hm is a union)
> is no larger than sizeof(*hm). However, this relation is broken
> after the second fetch, copy_from_user(hm, puhm, hm->h.size).
>
> As a concrete example, a user could put 0x000A when the first
> fetch happens which make hm->h.size <= sizeof(*hm) and later
> races to change it to 0xFFFF in the second fetch. What makes it
> even worse is this call: hpi_send_recv_f(&hm->m0, &hr->r0, file),
> which sends &hm->m0 to a lot of destinations. If any of the
> downstream functions assumes that hm->m0.size <= sizeof(*hm)
> which is actually not, an exploit can be constructed.
>
> In fact, similar issues have caused vulnerabilities before as in
> https://bugzilla.kernel.org/show_bug.cgi?id=116651
> https://bugzilla.kernel.org/show_bug.cgi?id=120131,
> and more recently the fix in sched/perf
> https://marc.info/?l=linux-kernel&m=150401225812533&w=2
>
> Feel free to let us know your opinion.
OK, now it's clearer, it's about the overwrite by the second
copy_from_user() call. I took the patch as is now.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-19 20:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-19 5:21 [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm Meng Xu
2017-09-19 7:27 ` Takashi Iwai
2017-09-19 7:27 ` Takashi Iwai
2017-09-19 13:54 ` Meng Xu
2017-09-19 20:04 ` Takashi Iwai
2017-09-19 20:04 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.