Re: [PATCH] ALSA: asihpi: fix a potential double-fetch bug when copying puhm

[Meng Xu]

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@xxxxxxxxx>
> 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