New subject: Patch "usb: gadget: u_audio: protect stream runtime fields with stream spinlock" has been added to the 4.14-stable tree

26 Aug 2018

Hi Greg,
On 08/26/2018 10:14 AM, gregkh@linuxfoundation.org wrote:
...
This is a note to let you know that I've just added the patch titled
 usb: gadget: u_audio: protect stream runtime fields with stream spinlock


to the 4.14-stable tree which can be found at:
     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is:
      usb-gadget-u_audio-protect-stream-runtime-fields-with-stream-spinlock.patch
and it can be found in the queue-4.14 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let stable@vger.kernel.org know about it.
From foo@baz Sun Aug 26 09:13:00 CEST 2018
From: Vladimir Zapolskiy vladimir_zapolskiy@mentor.com
Date: Thu, 21 Jun 2018 17:22:52 +0200
Subject: usb: gadget: u_audio: protect stream runtime fields with stream spinlock
From: Vladimir Zapolskiy vladimir_zapolskiy@mentor.com
[ Upstream commit 56bc61587daadef67712068f251c4ef2e3932d94 ]
The change protects almost the whole body of u_audio_iso_complete()
function by PCM stream lock, this is mainly sufficient to avoid a race
between USB request completion and stream termination, the change
prevents a possibility of invalid memory access in interrupt context
by memcpy():
 Unable to handle kernel paging request at virtual address 00004e80
 pgd = c0004000
 [00004e80] *pgd=00000000
 Internal error: Oops: 817 [#1] PREEMPT SMP ARM
 CPU: 0 PID: 3 Comm: ksoftirqd/0 Tainted: G         C   3.14.54+ #117
 task: da180b80 ti: da192000 task.ti: da192000
 PC is at memcpy+0x50/0x330
 LR is at 0xcdd92b0e
 pc : [<c029ef30>]    lr : [<cdd92b0e>]    psr: 20000193
 sp : da193ce4  ip : dd86ae26  fp : 0000b180
 r10: daf81680  r9 : 00000000  r8 : d58a01ea
 r7 : 2c0b43e4  r6 : acdfb08b  r5 : 01a271cf  r4 : 87389377
 r3 : 69469782  r2 : 00000020  r1 : daf82fe0  r0 : 00004e80
 Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
 Control: 10c5387d  Table: 2b70804a  DAC: 00000015
 Process ksoftirqd/0 (pid: 3, stack limit = 0xda192238)


Also added a check for potential !runtime condition, commonly it is
done by PCM_RUNTIME_CHECK(substream) in the beginning, however this
does not completely prevent from oopses in u_audio_iso_complete(),
because the proper protection scheme must be implemented in PCM
library functions.
An example of *not fixed* oops due to substream->runtime->*
dereference by snd_pcm_running(substream) from
snd_pcm_period_elapsed(), where substream->runtime is gone while
waiting the substream lock:
 Unable to handle kernel paging request at virtual address 6b6b6b6b
 pgd = db7e4000
 [6b6b6b6b] *pgd=00000000
 CPU: 0 PID: 193 Comm: klogd Tainted: G         C   3.14.54+ #118
 task: db5ac500 ti: db60c000 task.ti: db60c000
 PC is at snd_pcm_period_elapsed+0x48/0xd8 [snd_pcm]
 LR is at snd_pcm_period_elapsed+0x40/0xd8 [snd_pcm]
 pc : [<>]    lr : [<>]    psr: 60000193
 Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
 Control: 10c5387d  Table: 2b7e404a  DAC: 00000015
 Process klogd (pid: 193, stack limit = 0xdb60c238)
 [<>] (snd_pcm_period_elapsed [snd_pcm]) from [<>] (udc_irq+0x500/0xbbc)
 [<>] (udc_irq) from [<>] (ci_irq+0x280/0x304)
 [<>] (ci_irq) from [<>] (handle_irq_event_percpu+0xa4/0x40c)
 [<>] (handle_irq_event_percpu) from [<>] (handle_irq_event+0x3c/0x5c)
 [<>] (handle_irq_event) from [<>] (handle_fasteoi_irq+0xc4/0x110)
 [<>] (handle_fasteoi_irq) from [<>] (generic_handle_irq+0x20/0x30)
 [<>] (generic_handle_irq) from [<>] (handle_IRQ+0x80/0xc0)
 [<>] (handle_IRQ) from [<>] (gic_handle_irq+0x3c/0x60)
 [<>] (gic_handle_irq) from [<>] (__irq_svc+0x44/0x78)


Signed-off-by: Vladimir Zapolskiy vladimir_zapolskiy@mentor.com
[erosca: W/o this patch, with minimal instrumentation [1], I can
          consistently reproduce BUG: KASAN: use-after-free [2]]
[1] Instrumentation to reproduce issue [2]:
  diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
  index a72295c953bb..bd0b308024fe 100644
  --- a/drivers/usb/gadget/function/u_audio.c
  +++ b/drivers/usb/gadget/function/u_audio.c
  @@ -16,6 +16,7 @@
   #include <sound/core.h>
   #include <sound/pcm.h>
   #include <sound/pcm_params.h>
  +#include <linux/delay.h>
#include "u_audio.h"
@@ -147,6 +148,8 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
spin_unlock_irqrestore(&prm->lock, flags);

udelay(500); //delay here to increase probability of parallel activities


/* Pack USB load in ALSA ring buffer */
   pending = prm->dma_bytes - hw_ptr;
[2] After applying [1], below BUG occurs on Rcar-H3-Salvator-X board:
BUG: KASAN: use-after-free in u_audio_iso_complete+0x24c/0x520 [u_audio]
Read of size 8 at addr ffff8006cafcc248 by task swapper/0/0
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        WC      4.14.47+ #160
Hardware name: Renesas Salvator-X board based on r8a7795 ES2.0+ (DT)
Call trace:
[<ffff2000080925ac>] dump_backtrace+0x0/0x364
[<ffff200008092924>] show_stack+0x14/0x1c
[<ffff200008f8dbcc>] dump_stack+0x108/0x174
[<ffff2000083c71b8>] print_address_description+0x7c/0x32c
[<ffff2000083c78e8>] kasan_report+0x324/0x354
[<ffff2000083c6114>] __asan_load8+0x24/0x94
[<ffff2000021d1b34>] u_audio_iso_complete+0x24c/0x520 [u_audio]
[<ffff20000152fe50>] usb_gadget_giveback_request+0x480/0x4d0 [udc_core]
[<ffff200001860ab8>] usbhsg_queue_done+0x100/0x130 [renesas_usbhs]
[<ffff20000185f814>] usbhsf_pkt_handler+0x1a4/0x298 [renesas_usbhs]
[<ffff20000185fb38>] usbhsf_irq_ready+0x128/0x178 [renesas_usbhs]
[<ffff200001859cc8>] usbhs_interrupt+0x440/0x490 [renesas_usbhs]
[<ffff2000081a0288>] __handle_irq_event_percpu+0x594/0xa58
[<ffff2000081a07d0>] handle_irq_event_percpu+0x84/0x12c
[<ffff2000081a0928>] handle_irq_event+0xb0/0x10c
[<ffff2000081a8384>] handle_fasteoi_irq+0x1e0/0x2ec
[<ffff20000819e5f8>] generic_handle_irq+0x2c/0x44
[<ffff20000819f0d0>] __handle_domain_irq+0x190/0x194
[<ffff20000808177c>] gic_handle_irq+0x80/0xac
Exception stack(0xffff200009e97c80 to 0xffff200009e97dc0)
7c80: 0000000000000000 0000000000000000 0000000000000003 ffff200008179298
7ca0: ffff20000ae1c180 dfff200000000000 0000000000000000 ffff2000081f9a88
7cc0: ffff200009eb5960 ffff200009e97cf0 0000000000001600 ffff0400041b064b
7ce0: 0000000000000000 0000000000000002 0000000200000001 0000000000000001
7d00: ffff20000842197c 0000ffff958c4970 0000000000000000 ffff8006da0d5b80
7d20: ffff8006d4678498 0000000000000000 000000126bde0a8b ffff8006d4678480
7d40: 0000000000000000 000000126bdbea64 ffff200008fd0000 ffff8006fffff980
7d60: 00000000495f0018 ffff200009e97dc0 ffff200008b6c4ec ffff200009e97dc0
7d80: ffff200008b6c4f0 0000000020000145 ffff8006da0d5b80 ffff8006d4678498
7da0: ffffffffffffffff ffff8006d4678498 ffff200009e97dc0 ffff200008b6c4f0
[<ffff200008084034>] el1_irq+0xb4/0x12c
[<ffff200008b6c4f0>] cpuidle_enter_state+0x818/0x844
[<ffff200008b6c59c>] cpuidle_enter+0x18/0x20
[<ffff20000815f2e4>] call_cpuidle+0x98/0x9c
[<ffff20000815f674>] do_idle+0x214/0x264
[<ffff20000815facc>] cpu_startup_entry+0x20/0x24
[<ffff200008fb09d8>] rest_init+0x30c/0x320
[<ffff2000095f1338>] start_kernel+0x570/0x5b0
---<-snip->---
Fixes: 132fcb460839 ("usb: gadget: Add Audio Class 2.0 Driver")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Signed-off-by: Felipe Balbi felipe.balbi@linux.intel.com
Signed-off-by: Sasha Levin alexander.levin@microsoft.com
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org

drivers/usb/gadget/function/u_audio.c |   13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)

--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -25,6 +25,7 @@
  #include <sound/core.h>
  #include <sound/pcm.h>
  #include <sound/pcm_params.h>
+#include <linux/delay.h>
See a comment below...
...
#include "u_audio.h"
  
@@ -88,7 +89,7 @@ static const struct snd_pcm_hardware uac
  static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
  {
   unsigned pending;

unsigned long flags;


unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
struct uac_req *ur = req->context;

@@ -115,7 +116,14 @@ static void u_audio_iso_complete(struct
   if (!substream)
   	goto exit;

snd_pcm_stream_lock_irqsave(substream, flags2);
runtime = substream->runtime;
if (!runtime || !snd_pcm_running(substream)) {
snd_pcm_stream_unlock_irqrestore(substream, flags2);


goto exit;


}
spin_lock_irqsave(&prm->lock, flags);

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
@@ -146,6 +154,8 @@ static void u_audio_iso_complete(struct
  
   spin_unlock_irqrestore(&prm->lock, flags);

udelay(500); //delay here to increase probability of parallel activities


^^^^^
That part of the change came from the commit message, it just describes 
instrumentation, and adding it to the change itself is wrong.
Note that the source commit 56bc61587daad does not have such issue.
--
Best wishes,
Vladimir

    

Re: Patch "usb: gadget: u_audio: protect stream runtime fields with stream spinlock" has been added to the 4.14-stable tree

[2] After applying [1], below BUG occurs on Rcar-H3-Salvator-X board:

[2] After applying [1], below BUG occurs on Rcar-H3-Salvator-X board: