linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
       [not found] <CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com>
@ 2020-05-07  8:23 ` Greg Kroah-Hartman
  2020-05-07  9:50 ` Takashi Iwai
  1 sibling, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07  8:23 UTC (permalink / raw)
  To: butt3rflyh4ck; +Cc: security, syzkaller, tiwai, alsa-devel, linux-kernel

On Thu, May 07, 2020 at 03:54:34PM +0800, butt3rflyh4ck wrote:
> I report a bug (in linux-5.7-rc1) found by syzkaller.
> 
> kernel config:
> https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> reproducer:
> https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> 
> I test the reproducer in linux-5.7-rc4 and crash too.

Same as the other email, patches are best, thansk!

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
       [not found] <CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com>
  2020-05-07  8:23 ` KASAN: use-after-free Write in snd_rawmidi_kernel_write1 Greg Kroah-Hartman
@ 2020-05-07  9:50 ` Takashi Iwai
  2020-05-07 10:01   ` butt3rflyh4ck
  1 sibling, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2020-05-07  9:50 UTC (permalink / raw)
  To: butt3rflyh4ck
  Cc: security, syzkaller, tiwai, Greg Kroah-Hartman, alsa-devel, linux-kernel

On Thu, 07 May 2020 09:54:34 +0200,
butt3rflyh4ck wrote:
> 
> I report a bug (in linux-5.7-rc1) found by syzkaller.
> 
> kernel config: 
> https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> reproducer: 
> https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog

Is this report different from your another one?
  https://lore.kernel.org/r/CAFcO6XMGT42wFBxEa01Ee5Msuecm+WiXnn4rc-VWkC4vTzycPg@mail.gmail.com


thanks,

Takashi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07  9:50 ` Takashi Iwai
@ 2020-05-07 10:01   ` butt3rflyh4ck
  0 siblings, 0 replies; 13+ messages in thread
From: butt3rflyh4ck @ 2020-05-07 10:01 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: security, syzkaller, tiwai, Greg Kroah-Hartman, alsa-devel, linux-kernel

No, it's not.

Regards,
  butt3rflyh4ck.


On Thu, May 7, 2020 at 5:50 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Thu, 07 May 2020 09:54:34 +0200,
> butt3rflyh4ck wrote:
> >
> > I report a bug (in linux-5.7-rc1) found by syzkaller.
> >
> > kernel config:
> > https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> > reproducer:
> > https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
>
> Is this report different from your another one?
>   https://lore.kernel.org/r/CAFcO6XMGT42wFBxEa01Ee5Msuecm+WiXnn4rc-VWkC4vTzycPg@mail.gmail.com
>
>
> thanks,
>
> Takashi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07 10:42         ` Takashi Iwai
@ 2020-05-07 11:44           ` Takashi Iwai
  0 siblings, 0 replies; 13+ messages in thread
From: Takashi Iwai @ 2020-05-07 11:44 UTC (permalink / raw)
  To: butt3rflyh4ck
  Cc: Greg Kroah-Hartman, Amadeusz SX2awiX4ski, security, alsa-devel,
	tiwai, linux-kernel, syzkaller

On Thu, 07 May 2020 12:42:27 +0200,
Takashi Iwai wrote:
> 
> On Thu, 07 May 2020 12:36:14 +0200,
> Takashi Iwai wrote:
> > 
> > On Thu, 07 May 2020 12:27:41 +0200,
> > Amadeusz SX2awiX4ski wrote:
> > > 
> > > So if I follow this correctly, you call spin_unlock_irqrestore twice
> > > in case of error?
> > 
> > Erm no, this is obviously wrong.  The error path needs re-lock.
> > Will respin the fix.
> 
> ... and below is the revised patch.

and yet more brush up, with a slightly better description.
This seems working on my local machine, but let me know if something
still goes wrong.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v3] ALSA: rawmidi: Fix racy buffer resize under concurrent
 accesses

The rawmidi core allows user to resize the runtime buffer via ioctl,
and this may lead to UAF when performed during concurrent reads or
writes: the read/write functions unlock the runtime lock temporarily
during copying form/to user-space, and that's the race window.

This patch fixes the hole by introducing a reference counter for the
runtime buffer read/write access and returns -EBUSY error when the
resize is performed concurrently against read/write.

Note that the ref count field is a simple integer instead of
refcount_t here, since the all contexts accessing the buffer is
basically protected with a spinlock, hence we need no expensive atomic
ops.  Also, note that this busy check is needed only against read /
write functions, and not in receive/transmit callbacks; the race can
happen only at the spinlock hole mentioned in the above, while the
whole function is protected for receive / transmit callbacks.

Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Fix spinlock unbalance at error path of snd_rawmidi_kernel_read1()    
v2->v3: Remove superfluous ref in receive/transmit; more detailed
	patch description

 include/sound/rawmidi.h |  1 +
 sound/core/rawmidi.c    | 31 +++++++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
index a36b7227a15a..334842daa904 100644
--- a/include/sound/rawmidi.h
+++ b/include/sound/rawmidi.h
@@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
 	size_t avail_min;	/* min avail for wakeup */
 	size_t avail;		/* max used buffer for wakeup */
 	size_t xruns;		/* over/underruns counter */
+	int buffer_ref;		/* buffer reference count */
 	/* misc */
 	spinlock_t lock;
 	wait_queue_head_t sleep;
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 20dd08e1f675..2a688b711a9a 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
 		runtime->event(runtime->substream);
 }
 
+/* buffer refcount management: call with runtime->lock held */
+static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
+{
+	runtime->buffer_ref++;
+}
+
+static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
+{
+	runtime->buffer_ref--;
+}
+
 static int snd_rawmidi_runtime_create(struct snd_rawmidi_substream *substream)
 {
 	struct snd_rawmidi_runtime *runtime;
@@ -669,6 +680,11 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime,
 		if (!newbuf)
 			return -ENOMEM;
 		spin_lock_irq(&runtime->lock);
+		if (runtime->buffer_ref) {
+			spin_unlock_irq(&runtime->lock);
+			kvfree(newbuf);
+			return -EBUSY;
+		}
 		oldbuf = runtime->buffer;
 		runtime->buffer = newbuf;
 		runtime->buffer_size = params->buffer_size;
@@ -1019,8 +1035,10 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
 	long result = 0, count1;
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 	unsigned long appl_ptr;
+	int err = 0;
 
 	spin_lock_irqsave(&runtime->lock, flags);
+	snd_rawmidi_buffer_ref(runtime);
 	while (count > 0 && runtime->avail) {
 		count1 = runtime->buffer_size - runtime->appl_ptr;
 		if (count1 > count)
@@ -1039,16 +1057,19 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
 		if (userbuf) {
 			spin_unlock_irqrestore(&runtime->lock, flags);
 			if (copy_to_user(userbuf + result,
-					 runtime->buffer + appl_ptr, count1)) {
-				return result > 0 ? result : -EFAULT;
-			}
+					 runtime->buffer + appl_ptr, count1))
+				err = -EFAULT;
 			spin_lock_irqsave(&runtime->lock, flags);
+			if (err)
+				goto out;
 		}
 		result += count1;
 		count -= count1;
 	}
+ out:
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
-	return result;
+	return result > 0 ? result : err;
 }
 
 long snd_rawmidi_kernel_read(struct snd_rawmidi_substream *substream,
@@ -1342,6 +1363,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
 			return -EAGAIN;
 		}
 	}
+	snd_rawmidi_buffer_ref(runtime);
 	while (count > 0 && runtime->avail > 0) {
 		count1 = runtime->buffer_size - runtime->appl_ptr;
 		if (count1 > count)
@@ -1373,6 +1395,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
 	}
       __end:
 	count1 = runtime->avail < runtime->buffer_size;
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
 	if (count1)
 		snd_rawmidi_output_trigger(substream, 1);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07 10:19       ` Takashi Iwai
@ 2020-05-07 11:07         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07 11:07 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: butt3rflyh4ck, security, syzkaller, tiwai, alsa-devel, linux-kernel

On Thu, May 07, 2020 at 12:19:18PM +0200, Takashi Iwai wrote:
> On Thu, 07 May 2020 12:13:10 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Thu, May 07, 2020 at 11:56:22AM +0200, Takashi Iwai wrote:
> > > On Thu, 07 May 2020 10:23:02 +0200,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> > > > > I report a bug (in linux-5.7-rc1) found by syzkaller.
> > > > > 
> > > > > kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> > > > > reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> > > > > 
> > > > > I test the reproducer in linux-5.7-rc4 and crash too.
> > > > 
> > > > Great, care to create a fix for this and send it to the proper
> > > > maintainers?  That's the best way to get it fixed, otherwise it just
> > > > goes in the file with the rest of the syzbot reports we are burried
> > > > under.
> > > 
> > > Don't worry, I already prepared a fix patch below :)
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > -- 8< --
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
> > >  accesses
> > > 
> > > The rawmidi core allows user to resize the runtime buffer via ioctl,
> > > and this may lead to UAF when performed during concurrent reads or
> > > writes.
> > > 
> > > This patch fixes the race by introducing a reference counter for the
> > > runtime buffer access and returns -EBUSY error when the resize is
> > > performed concurrently.
> > > 
> > > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> > > Cc: <stable@vger.kernel.org>
> > > Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  include/sound/rawmidi.h |  1 +
> > >  sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
> > >  2 files changed, 29 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> > > index a36b7227a15a..334842daa904 100644
> > > --- a/include/sound/rawmidi.h
> > > +++ b/include/sound/rawmidi.h
> > > @@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
> > >  	size_t avail_min;	/* min avail for wakeup */
> > >  	size_t avail;		/* max used buffer for wakeup */
> > >  	size_t xruns;		/* over/underruns counter */
> > > +	int buffer_ref;		/* buffer reference count */
> > >  	/* misc */
> > >  	spinlock_t lock;
> > >  	wait_queue_head_t sleep;
> > > diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> > > index 20dd08e1f675..4185d9e81e3c 100644
> > > --- a/sound/core/rawmidi.c
> > > +++ b/sound/core/rawmidi.c
> > > @@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
> > >  		runtime->event(runtime->substream);
> > >  }
> > >  
> > > +/* buffer refcount management: call with runtime->lock held */
> > > +static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
> > > +{
> > > +	runtime->buffer_ref++;
> > > +}
> > > +
> > > +static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
> > > +{
> > > +	runtime->buffer_ref--;
> > > +}
> > 
> > Why not use the reference count structure?
> 
> The context accessing the buffer is always with the spinlock, so we
> don't need expensive atomic ops there.
> 
> Usually this kind of check can be a simple boolean flag, but in this
> case, there is one place that goes out of lock due to
> copy_from/to_user, so a refcount is used in this patch instead.

Ah, ok, thanks for the explanation.

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07 10:36       ` Takashi Iwai
@ 2020-05-07 10:42         ` Takashi Iwai
  2020-05-07 11:44           ` Takashi Iwai
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2020-05-07 10:42 UTC (permalink / raw)
  To: Amadeusz SX2awiX4ski
  Cc: Greg Kroah-Hartman, security, alsa-devel, butt3rflyh4ck, tiwai,
	linux-kernel, syzkaller

On Thu, 07 May 2020 12:36:14 +0200,
Takashi Iwai wrote:
> 
> On Thu, 07 May 2020 12:27:41 +0200,
> Amadeusz SX2awiX4ski wrote:
> > 
> > So if I follow this correctly, you call spin_unlock_irqrestore twice
> > in case of error?
> 
> Erm no, this is obviously wrong.  The error path needs re-lock.
> Will respin the fix.

... and below is the revised patch.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: rawmidi: Fix racy buffer resize under concurrent
 accesses

The rawmidi core allows user to resize the runtime buffer via ioctl,
and this may lead to UAF when performed during concurrent reads or
writes.

This patch fixes the race by introducing a reference counter for the
runtime buffer access and returns -EBUSY error when the resize is
performed concurrently.

Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Fix spinlock unbalance at error path of snd_rawmidi_kernel_read1()

 include/sound/rawmidi.h |  1 +
 sound/core/rawmidi.c    | 35 +++++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
index a36b7227a15a..334842daa904 100644
--- a/include/sound/rawmidi.h
+++ b/include/sound/rawmidi.h
@@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
 	size_t avail_min;	/* min avail for wakeup */
 	size_t avail;		/* max used buffer for wakeup */
 	size_t xruns;		/* over/underruns counter */
+	int buffer_ref;		/* buffer reference count */
 	/* misc */
 	spinlock_t lock;
 	wait_queue_head_t sleep;
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 20dd08e1f675..d9f20dd990db 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
 		runtime->event(runtime->substream);
 }
 
+/* buffer refcount management: call with runtime->lock held */
+static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
+{
+	runtime->buffer_ref++;
+}
+
+static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
+{
+	runtime->buffer_ref--;
+}
+
 static int snd_rawmidi_runtime_create(struct snd_rawmidi_substream *substream)
 {
 	struct snd_rawmidi_runtime *runtime;
@@ -669,6 +680,11 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime,
 		if (!newbuf)
 			return -ENOMEM;
 		spin_lock_irq(&runtime->lock);
+		if (runtime->buffer_ref) {
+			spin_unlock_irq(&runtime->lock);
+			kvfree(newbuf);
+			return -EBUSY;
+		}
 		oldbuf = runtime->buffer;
 		runtime->buffer = newbuf;
 		runtime->buffer_size = params->buffer_size;
@@ -962,6 +978,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 		return -EINVAL;
 	}
 	spin_lock_irqsave(&runtime->lock, flags);
+	snd_rawmidi_buffer_ref(runtime);
 	if (count == 1) {	/* special case, faster code */
 		substream->bytes++;
 		if (runtime->avail < runtime->buffer_size) {
@@ -1006,6 +1023,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 		else if (snd_rawmidi_ready(substream))
 			wake_up(&runtime->sleep);
 	}
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
 	return result;
 }
@@ -1019,8 +1037,10 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
 	long result = 0, count1;
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 	unsigned long appl_ptr;
+	int err = 0;
 
 	spin_lock_irqsave(&runtime->lock, flags);
+	snd_rawmidi_buffer_ref(runtime);
 	while (count > 0 && runtime->avail) {
 		count1 = runtime->buffer_size - runtime->appl_ptr;
 		if (count1 > count)
@@ -1039,16 +1059,19 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
 		if (userbuf) {
 			spin_unlock_irqrestore(&runtime->lock, flags);
 			if (copy_to_user(userbuf + result,
-					 runtime->buffer + appl_ptr, count1)) {
-				return result > 0 ? result : -EFAULT;
-			}
+					 runtime->buffer + appl_ptr, count1))
+				err = -EFAULT;
 			spin_lock_irqsave(&runtime->lock, flags);
+			if (err)
+				goto out;
 		}
 		result += count1;
 		count -= count1;
 	}
+ out:
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
-	return result;
+	return result > 0 ? result : err;
 }
 
 long snd_rawmidi_kernel_read(struct snd_rawmidi_substream *substream,
@@ -1156,6 +1179,7 @@ int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
 		return -EINVAL;
 	}
 	result = 0;
+	snd_rawmidi_buffer_ref(runtime);
 	if (runtime->avail >= runtime->buffer_size) {
 		/* warning: lowlevel layer MUST trigger down the hardware */
 		goto __skip;
@@ -1180,6 +1204,7 @@ int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
 		}
 	}
       __skip:
+	snd_rawmidi_buffer_unref(runtime);
 	return result;
 }
 EXPORT_SYMBOL(__snd_rawmidi_transmit_peek);
@@ -1342,6 +1367,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
 			return -EAGAIN;
 		}
 	}
+	snd_rawmidi_buffer_ref(runtime);
 	while (count > 0 && runtime->avail > 0) {
 		count1 = runtime->buffer_size - runtime->appl_ptr;
 		if (count1 > count)
@@ -1373,6 +1399,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
 	}
       __end:
 	count1 = runtime->avail < runtime->buffer_size;
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
 	if (count1)
 		snd_rawmidi_output_trigger(substream, 1);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07 10:27     ` Amadeusz Sławiński
@ 2020-05-07 10:36       ` Takashi Iwai
  2020-05-07 10:42         ` Takashi Iwai
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2020-05-07 10:36 UTC (permalink / raw)
  To: Amadeusz SX2awiX4ski
  Cc: Greg Kroah-Hartman, security, alsa-devel, butt3rflyh4ck, tiwai,
	linux-kernel, syzkaller

On Thu, 07 May 2020 12:27:41 +0200,
Amadeusz SX2awiX4ski wrote:
> 
> 
> 
> On 5/7/2020 11:56 AM, Takashi Iwai wrote:
> > On Thu, 07 May 2020 10:23:02 +0200,
> > Greg Kroah-Hartman wrote:
> >>
> >> On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> >>> I report a bug (in linux-5.7-rc1) found by syzkaller.
> >>>
> >>> kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> >>> reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> >>>
> >>> I test the reproducer in linux-5.7-rc4 and crash too.
> >>
> >> Great, care to create a fix for this and send it to the proper
> >> maintainers?  That's the best way to get it fixed, otherwise it just
> >> goes in the file with the rest of the syzbot reports we are burried
> >> under.
> >
> > Don't worry, I already prepared a fix patch below :)
> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
> >   accesses
> >
> > The rawmidi core allows user to resize the runtime buffer via ioctl,
> > and this may lead to UAF when performed during concurrent reads or
> > writes.
> >
> > This patch fixes the race by introducing a reference counter for the
> > runtime buffer access and returns -EBUSY error when the resize is
> > performed concurrently.
> >
> > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> > Cc: <stable@vger.kernel.org>
> > Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   include/sound/rawmidi.h |  1 +
> >   sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
> >   2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> > index a36b7227a15a..334842daa904 100644
> > --- a/include/sound/rawmidi.h
> 
> (...)
> 
> > @@ -1021,6 +1039,7 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
> >   	unsigned long appl_ptr;
> >     	spin_lock_irqsave(&runtime->lock, flags);
> > +	snd_rawmidi_buffer_ref(runtime);
> >   	while (count > 0 && runtime->avail) {
> >   		count1 = runtime->buffer_size - runtime->appl_ptr;
> >   		if (count1 > count)
> > @@ -1040,13 +1059,17 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
> >   			spin_unlock_irqrestore(&runtime->lock, flags);
> First unlock
> >   			if (copy_to_user(userbuf + result,
> >   					 runtime->buffer + appl_ptr, count1)) {
> > -				return result > 0 ? result : -EFAULT;
> > +				if (!result)
> > +					result = -EFAULT;
> > +				goto out;
> 
> goto -> Second unlock
> >   			}
> >   			spin_lock_irqsave(&runtime->lock, flags);
> >   		}
> >   		result += count1;
> >   		count -= count1;
> >   	}
> > + out:
> > +	snd_rawmidi_buffer_unref(runtime);
> >   	spin_unlock_irqrestore(&runtime->lock, flags);
> Second unlock
> >   	return result;
> >   }
> 
> So if I follow this correctly, you call spin_unlock_irqrestore twice
> in case of error?

Erm no, this is obviously wrong.  The error path needs re-lock.
Will respin the fix.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07  9:56   ` Takashi Iwai
  2020-05-07 10:13     ` Greg Kroah-Hartman
@ 2020-05-07 10:27     ` Amadeusz Sławiński
  2020-05-07 10:36       ` Takashi Iwai
  1 sibling, 1 reply; 13+ messages in thread
From: Amadeusz Sławiński @ 2020-05-07 10:27 UTC (permalink / raw)
  To: Takashi Iwai, Greg Kroah-Hartman
  Cc: security, alsa-devel, butt3rflyh4ck, tiwai, linux-kernel, syzkaller



On 5/7/2020 11:56 AM, Takashi Iwai wrote:
> On Thu, 07 May 2020 10:23:02 +0200,
> Greg Kroah-Hartman wrote:
>>
>> On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
>>> I report a bug (in linux-5.7-rc1) found by syzkaller.
>>>
>>> kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
>>> reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
>>>
>>> I test the reproducer in linux-5.7-rc4 and crash too.
>>
>> Great, care to create a fix for this and send it to the proper
>> maintainers?  That's the best way to get it fixed, otherwise it just
>> goes in the file with the rest of the syzbot reports we are burried
>> under.
> 
> Don't worry, I already prepared a fix patch below :)
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
>   accesses
> 
> The rawmidi core allows user to resize the runtime buffer via ioctl,
> and this may lead to UAF when performed during concurrent reads or
> writes.
> 
> This patch fixes the race by introducing a reference counter for the
> runtime buffer access and returns -EBUSY error when the resize is
> performed concurrently.
> 
> Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> Cc: <stable@vger.kernel.org>
> Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   include/sound/rawmidi.h |  1 +
>   sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
>   2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> index a36b7227a15a..334842daa904 100644
> --- a/include/sound/rawmidi.h

(...)

> @@ -1021,6 +1039,7 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
>   	unsigned long appl_ptr;
>   
>   	spin_lock_irqsave(&runtime->lock, flags);
> +	snd_rawmidi_buffer_ref(runtime);
>   	while (count > 0 && runtime->avail) {
>   		count1 = runtime->buffer_size - runtime->appl_ptr;
>   		if (count1 > count)
> @@ -1040,13 +1059,17 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
>   			spin_unlock_irqrestore(&runtime->lock, flags);
First unlock
>   			if (copy_to_user(userbuf + result,
>   					 runtime->buffer + appl_ptr, count1)) {
> -				return result > 0 ? result : -EFAULT;
> +				if (!result)
> +					result = -EFAULT;
> +				goto out;

goto -> Second unlock
>   			}
>   			spin_lock_irqsave(&runtime->lock, flags);
>   		}
>   		result += count1;
>   		count -= count1;
>   	}
> + out:
> +	snd_rawmidi_buffer_unref(runtime);
>   	spin_unlock_irqrestore(&runtime->lock, flags);
Second unlock
>   	return result;
>   }

So if I follow this correctly, you call spin_unlock_irqrestore twice in 
case of error?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07 10:13     ` Greg Kroah-Hartman
@ 2020-05-07 10:19       ` Takashi Iwai
  2020-05-07 11:07         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2020-05-07 10:19 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: butt3rflyh4ck, security, syzkaller, tiwai, alsa-devel, linux-kernel

On Thu, 07 May 2020 12:13:10 +0200,
Greg Kroah-Hartman wrote:
> 
> On Thu, May 07, 2020 at 11:56:22AM +0200, Takashi Iwai wrote:
> > On Thu, 07 May 2020 10:23:02 +0200,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> > > > I report a bug (in linux-5.7-rc1) found by syzkaller.
> > > > 
> > > > kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> > > > reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> > > > 
> > > > I test the reproducer in linux-5.7-rc4 and crash too.
> > > 
> > > Great, care to create a fix for this and send it to the proper
> > > maintainers?  That's the best way to get it fixed, otherwise it just
> > > goes in the file with the rest of the syzbot reports we are burried
> > > under.
> > 
> > Don't worry, I already prepared a fix patch below :)
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
> >  accesses
> > 
> > The rawmidi core allows user to resize the runtime buffer via ioctl,
> > and this may lead to UAF when performed during concurrent reads or
> > writes.
> > 
> > This patch fixes the race by introducing a reference counter for the
> > runtime buffer access and returns -EBUSY error when the resize is
> > performed concurrently.
> > 
> > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> > Cc: <stable@vger.kernel.org>
> > Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  include/sound/rawmidi.h |  1 +
> >  sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> > index a36b7227a15a..334842daa904 100644
> > --- a/include/sound/rawmidi.h
> > +++ b/include/sound/rawmidi.h
> > @@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
> >  	size_t avail_min;	/* min avail for wakeup */
> >  	size_t avail;		/* max used buffer for wakeup */
> >  	size_t xruns;		/* over/underruns counter */
> > +	int buffer_ref;		/* buffer reference count */
> >  	/* misc */
> >  	spinlock_t lock;
> >  	wait_queue_head_t sleep;
> > diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> > index 20dd08e1f675..4185d9e81e3c 100644
> > --- a/sound/core/rawmidi.c
> > +++ b/sound/core/rawmidi.c
> > @@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
> >  		runtime->event(runtime->substream);
> >  }
> >  
> > +/* buffer refcount management: call with runtime->lock held */
> > +static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
> > +{
> > +	runtime->buffer_ref++;
> > +}
> > +
> > +static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
> > +{
> > +	runtime->buffer_ref--;
> > +}
> 
> Why not use the reference count structure?

The context accessing the buffer is always with the spinlock, so we
don't need expensive atomic ops there.

Usually this kind of check can be a simple boolean flag, but in this
case, there is one place that goes out of lock due to
copy_from/to_user, so a refcount is used in this patch instead.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07  9:56   ` Takashi Iwai
@ 2020-05-07 10:13     ` Greg Kroah-Hartman
  2020-05-07 10:19       ` Takashi Iwai
  2020-05-07 10:27     ` Amadeusz Sławiński
  1 sibling, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07 10:13 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: butt3rflyh4ck, security, syzkaller, tiwai, alsa-devel, linux-kernel

On Thu, May 07, 2020 at 11:56:22AM +0200, Takashi Iwai wrote:
> On Thu, 07 May 2020 10:23:02 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> > > I report a bug (in linux-5.7-rc1) found by syzkaller.
> > > 
> > > kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> > > reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> > > 
> > > I test the reproducer in linux-5.7-rc4 and crash too.
> > 
> > Great, care to create a fix for this and send it to the proper
> > maintainers?  That's the best way to get it fixed, otherwise it just
> > goes in the file with the rest of the syzbot reports we are burried
> > under.
> 
> Don't worry, I already prepared a fix patch below :)
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
>  accesses
> 
> The rawmidi core allows user to resize the runtime buffer via ioctl,
> and this may lead to UAF when performed during concurrent reads or
> writes.
> 
> This patch fixes the race by introducing a reference counter for the
> runtime buffer access and returns -EBUSY error when the resize is
> performed concurrently.
> 
> Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> Cc: <stable@vger.kernel.org>
> Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  include/sound/rawmidi.h |  1 +
>  sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> index a36b7227a15a..334842daa904 100644
> --- a/include/sound/rawmidi.h
> +++ b/include/sound/rawmidi.h
> @@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
>  	size_t avail_min;	/* min avail for wakeup */
>  	size_t avail;		/* max used buffer for wakeup */
>  	size_t xruns;		/* over/underruns counter */
> +	int buffer_ref;		/* buffer reference count */
>  	/* misc */
>  	spinlock_t lock;
>  	wait_queue_head_t sleep;
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index 20dd08e1f675..4185d9e81e3c 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
>  		runtime->event(runtime->substream);
>  }
>  
> +/* buffer refcount management: call with runtime->lock held */
> +static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
> +{
> +	runtime->buffer_ref++;
> +}
> +
> +static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
> +{
> +	runtime->buffer_ref--;
> +}

Why not use the reference count structure?

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07  8:23 ` Greg Kroah-Hartman
@ 2020-05-07  9:56   ` Takashi Iwai
  2020-05-07 10:13     ` Greg Kroah-Hartman
  2020-05-07 10:27     ` Amadeusz Sławiński
  0 siblings, 2 replies; 13+ messages in thread
From: Takashi Iwai @ 2020-05-07  9:56 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: butt3rflyh4ck, security, syzkaller, tiwai, alsa-devel, linux-kernel

On Thu, 07 May 2020 10:23:02 +0200,
Greg Kroah-Hartman wrote:
> 
> On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> > I report a bug (in linux-5.7-rc1) found by syzkaller.
> > 
> > kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> > reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> > 
> > I test the reproducer in linux-5.7-rc4 and crash too.
> 
> Great, care to create a fix for this and send it to the proper
> maintainers?  That's the best way to get it fixed, otherwise it just
> goes in the file with the rest of the syzbot reports we are burried
> under.

Don't worry, I already prepared a fix patch below :)


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
 accesses

The rawmidi core allows user to resize the runtime buffer via ioctl,
and this may lead to UAF when performed during concurrent reads or
writes.

This patch fixes the race by introducing a reference counter for the
runtime buffer access and returns -EBUSY error when the resize is
performed concurrently.

Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/rawmidi.h |  1 +
 sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
index a36b7227a15a..334842daa904 100644
--- a/include/sound/rawmidi.h
+++ b/include/sound/rawmidi.h
@@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
 	size_t avail_min;	/* min avail for wakeup */
 	size_t avail;		/* max used buffer for wakeup */
 	size_t xruns;		/* over/underruns counter */
+	int buffer_ref;		/* buffer reference count */
 	/* misc */
 	spinlock_t lock;
 	wait_queue_head_t sleep;
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 20dd08e1f675..4185d9e81e3c 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
 		runtime->event(runtime->substream);
 }
 
+/* buffer refcount management: call with runtime->lock held */
+static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
+{
+	runtime->buffer_ref++;
+}
+
+static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
+{
+	runtime->buffer_ref--;
+}
+
 static int snd_rawmidi_runtime_create(struct snd_rawmidi_substream *substream)
 {
 	struct snd_rawmidi_runtime *runtime;
@@ -669,6 +680,11 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime,
 		if (!newbuf)
 			return -ENOMEM;
 		spin_lock_irq(&runtime->lock);
+		if (runtime->buffer_ref) {
+			spin_unlock_irq(&runtime->lock);
+			kvfree(newbuf);
+			return -EBUSY;
+		}
 		oldbuf = runtime->buffer;
 		runtime->buffer = newbuf;
 		runtime->buffer_size = params->buffer_size;
@@ -962,6 +978,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 		return -EINVAL;
 	}
 	spin_lock_irqsave(&runtime->lock, flags);
+	snd_rawmidi_buffer_ref(runtime);
 	if (count == 1) {	/* special case, faster code */
 		substream->bytes++;
 		if (runtime->avail < runtime->buffer_size) {
@@ -1006,6 +1023,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 		else if (snd_rawmidi_ready(substream))
 			wake_up(&runtime->sleep);
 	}
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
 	return result;
 }
@@ -1021,6 +1039,7 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
 	unsigned long appl_ptr;
 
 	spin_lock_irqsave(&runtime->lock, flags);
+	snd_rawmidi_buffer_ref(runtime);
 	while (count > 0 && runtime->avail) {
 		count1 = runtime->buffer_size - runtime->appl_ptr;
 		if (count1 > count)
@@ -1040,13 +1059,17 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
 			spin_unlock_irqrestore(&runtime->lock, flags);
 			if (copy_to_user(userbuf + result,
 					 runtime->buffer + appl_ptr, count1)) {
-				return result > 0 ? result : -EFAULT;
+				if (!result)
+					result = -EFAULT;
+				goto out;
 			}
 			spin_lock_irqsave(&runtime->lock, flags);
 		}
 		result += count1;
 		count -= count1;
 	}
+ out:
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
 	return result;
 }
@@ -1156,6 +1179,7 @@ int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
 		return -EINVAL;
 	}
 	result = 0;
+	snd_rawmidi_buffer_ref(runtime);
 	if (runtime->avail >= runtime->buffer_size) {
 		/* warning: lowlevel layer MUST trigger down the hardware */
 		goto __skip;
@@ -1180,6 +1204,7 @@ int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
 		}
 	}
       __skip:
+	snd_rawmidi_buffer_unref(runtime);
 	return result;
 }
 EXPORT_SYMBOL(__snd_rawmidi_transmit_peek);
@@ -1342,6 +1367,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
 			return -EAGAIN;
 		}
 	}
+	snd_rawmidi_buffer_ref(runtime);
 	while (count > 0 && runtime->avail > 0) {
 		count1 = runtime->buffer_size - runtime->appl_ptr;
 		if (count1 > count)
@@ -1373,6 +1399,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
 	}
       __end:
 	count1 = runtime->avail < runtime->buffer_size;
+	snd_rawmidi_buffer_unref(runtime);
 	spin_unlock_irqrestore(&runtime->lock, flags);
 	if (count1)
 		snd_rawmidi_output_trigger(substream, 1);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
  2020-05-07  8:04 butt3rflyh4ck
@ 2020-05-07  8:23 ` Greg Kroah-Hartman
  2020-05-07  9:56   ` Takashi Iwai
  0 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07  8:23 UTC (permalink / raw)
  To: butt3rflyh4ck; +Cc: security, syzkaller, tiwai, alsa-devel, linux-kernel

On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> I report a bug (in linux-5.7-rc1) found by syzkaller.
> 
> kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> 
> I test the reproducer in linux-5.7-rc4 and crash too.

Great, care to create a fix for this and send it to the proper
maintainers?  That's the best way to get it fixed, otherwise it just
goes in the file with the rest of the syzbot reports we are burried
under.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* KASAN: use-after-free Write in snd_rawmidi_kernel_write1
@ 2020-05-07  8:04 butt3rflyh4ck
  2020-05-07  8:23 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 13+ messages in thread
From: butt3rflyh4ck @ 2020-05-07  8:04 UTC (permalink / raw)
  To: security; +Cc: syzkaller, tiwai, Greg Kroah-Hartman, alsa-devel, linux-kernel

I report a bug (in linux-5.7-rc1) found by syzkaller.

kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog

I test the reproducer in linux-5.7-rc4 and crash too.

crash log:
==================================================================
BUG: KASAN: use-after-free in _copy_from_user+0x113/0x1a0 lib/usercopy.c:13
Write of size 1 at addr ffff888069da43ca by task syz-executor611/6797

CPU: 0 PID: 6797 Comm: syz-executor611 Not tainted 5.7.0-rc1 #3
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.10.2-1ubuntu1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x188/0x20d lib/dump_stack.c:118
 print_address_description.constprop.0.cold+0xd3/0x347 mm/kasan/report.c:382
 __kasan_report.cold+0x35/0x4d mm/kasan/report.c:511
 kasan_report+0x33/0x50 mm/kasan/common.c:625
 check_memory_region_inline mm/kasan/generic.c:187 [inline]
 check_memory_region+0x141/0x190 mm/kasan/generic.c:193
 _copy_from_user+0x113/0x1a0 lib/usercopy.c:13
 copy_from_user include/linux/uaccess.h:144 [inline]
 snd_rawmidi_kernel_write1+0x320/0x6f0 sound/core/rawmidi.c:1363
 snd_rawmidi_write+0x2e4/0xb80 sound/core/rawmidi.c:1429
 __vfs_write+0x76/0x100 fs/read_write.c:495
 vfs_write+0x268/0x5d0 fs/read_write.c:559
 ksys_write+0x1ee/0x250 fs/read_write.c:612
 do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
 entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x4553a9
Code: 00 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48
89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d
01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f556b7f5cc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00000000006d9588 RCX: 00000000004553a9
RDX: 00000000fffffc85 RSI: 0000000020000040 RDI: 0000000000000004
RBP: 00000000006d9580 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006d958c
R13: 0000000000000000 R14: 00000000006d9580 R15: 00007fff34bcb390

Allocated by task 6797:
 save_stack+0x1b/0x40 mm/kasan/common.c:49
 set_track mm/kasan/common.c:57 [inline]
 __kasan_kmalloc mm/kasan/common.c:495 [inline]
 __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:468
 kmalloc_node include/linux/slab.h:578 [inline]
 kvmalloc_node+0x61/0xf0 mm/util.c:574
 kvmalloc include/linux/mm.h:757 [inline]
 kvzalloc include/linux/mm.h:765 [inline]
 snd_rawmidi_runtime_create sound/core/rawmidi.c:141 [inline]
 open_substream+0x33d/0x880 sound/core/rawmidi.c:285
 rawmidi_open_priv+0x58e/0x6e0 sound/core/rawmidi.c:336
 snd_rawmidi_open+0x455/0xb20 sound/core/rawmidi.c:433
 snd_open+0x212/0x3d0 sound/core/sound.c:163
 chrdev_open+0x219/0x5c0 fs/char_dev.c:414
 do_dentry_open+0x4ba/0x1290 fs/open.c:797
 do_open fs/namei.c:3229 [inline]
 path_openat+0x1e59/0x27d0 fs/namei.c:3346
 do_filp_open+0x192/0x260 fs/namei.c:3373
 do_sys_openat2+0x585/0x7d0 fs/open.c:1148
 do_sys_open+0xc3/0x140 fs/open.c:1164
 do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
 entry_SYSCALL_64_after_hwframe+0x49/0xb3

Freed by task 6798:
 save_stack+0x1b/0x40 mm/kasan/common.c:49
 set_track mm/kasan/common.c:57 [inline]
 kasan_set_free_info mm/kasan/common.c:317 [inline]
 __kasan_slab_free+0x126/0x170 mm/kasan/common.c:456
 slab_free_hook mm/slub.c:1445 [inline]
 slab_free_freelist_hook+0xc4/0x1c0 mm/slub.c:1478
 slab_free mm/slub.c:3035 [inline]
 kfree+0xd8/0x330 mm/slub.c:4003
 kvfree+0x42/0x50 mm/util.c:603
 resize_runtime_buffer+0x29e/0x370 sound/core/rawmidi.c:677
 snd_rawmidi_output_params+0x128/0x170 sound/core/rawmidi.c:690
 snd_rawmidi_ioctl+0x544/0x5b0 sound/core/rawmidi.c:841
 vfs_ioctl fs/ioctl.c:47 [inline]
 ksys_ioctl+0x11a/0x180 fs/ioctl.c:763
 __do_sys_ioctl fs/ioctl.c:772 [inline]
 __se_sys_ioctl fs/ioctl.c:770 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:770
 do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
 entry_SYSCALL_64_after_hwframe+0x49/0xb3

The buggy address belongs to the object at ffff888069da4000
 which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 970 bytes inside of
 4096-byte region [ffff888069da4000, ffff888069da5000)
The buggy address belongs to the page:
page:ffffea0001a76800 refcount:1 mapcount:0 mapping:00000000c136e8e7
index:0x0 head:ffffea0001a76800 order:3 compound_mapcount:0
compound_pincount:0
flags: 0xfffe0000010200(slab|head)
raw: 00fffe0000010200 dead000000000100 dead000000000122 ffff88806bc0c280
raw: 0000000000000000 0000000080040004 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888069da4280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888069da4300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff888069da4380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                              ^
 ffff888069da4400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888069da4480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Regards,
  butt3rflyh4ck

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-05-07 11:44 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com>
2020-05-07  8:23 ` KASAN: use-after-free Write in snd_rawmidi_kernel_write1 Greg Kroah-Hartman
2020-05-07  9:50 ` Takashi Iwai
2020-05-07 10:01   ` butt3rflyh4ck
2020-05-07  8:04 butt3rflyh4ck
2020-05-07  8:23 ` Greg Kroah-Hartman
2020-05-07  9:56   ` Takashi Iwai
2020-05-07 10:13     ` Greg Kroah-Hartman
2020-05-07 10:19       ` Takashi Iwai
2020-05-07 11:07         ` Greg Kroah-Hartman
2020-05-07 10:27     ` Amadeusz Sławiński
2020-05-07 10:36       ` Takashi Iwai
2020-05-07 10:42         ` Takashi Iwai
2020-05-07 11:44           ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).