Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1341115
| From | Ben Hutchings <ben@decadent.org.uk> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | [PATCH 3.2 38/67] ALSA: rawmidi: Fix race at copying & updating the position |
| Date | 2016-02-23 23:10 +0100 |
| Message-ID | <r5tjB-3kK-51@gated-at.bofh.it> (permalink) |
| References | <r5t0e-2Ub-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
3.2.78-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Takashi Iwai <tiwai@suse.de>
commit 81f577542af15640cbcb6ef68baa4caa610cbbfc upstream.
The rawmidi read and write functions manage runtime stream status
such as runtime->appl_ptr and runtime->avail. These point where to
copy the new data and how many bytes have been copied (or to be
read). The problem is that rawmidi read/write call copy_from_user()
or copy_to_user(), and the runtime spinlock is temporarily unlocked
and relocked while copying user-space. Since the current code
advances and updates the runtime status after the spin unlock/relock,
the copy and the update may be asynchronous, and eventually
runtime->avail might go to a negative value when many concurrent
accesses are done. This may lead to memory corruption in the end.
For fixing this race, in this patch, the status update code is
performed in the same lock before the temporary unlock. Also, the
spinlock is now taken more widely in snd_rawmidi_kernel_read1() for
protecting more properly during the whole operation.
BugLink: http://lkml.kernel.org/r/CACT4Y+b-dCmNf1GpgPKfDO0ih+uZCL2JV4__j-r1kdhPLSgQCQ@mail.gmail.com
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Tested-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
sound/core/rawmidi.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -934,31 +934,36 @@ static long snd_rawmidi_kernel_read1(str
unsigned long flags;
long result = 0, count1;
struct snd_rawmidi_runtime *runtime = substream->runtime;
+ unsigned long appl_ptr;
+ spin_lock_irqsave(&runtime->lock, flags);
while (count > 0 && runtime->avail) {
count1 = runtime->buffer_size - runtime->appl_ptr;
if (count1 > count)
count1 = count;
- spin_lock_irqsave(&runtime->lock, flags);
if (count1 > (int)runtime->avail)
count1 = runtime->avail;
+
+ /* update runtime->appl_ptr before unlocking for userbuf */
+ appl_ptr = runtime->appl_ptr;
+ runtime->appl_ptr += count1;
+ runtime->appl_ptr %= runtime->buffer_size;
+ runtime->avail -= count1;
+
if (kernelbuf)
- memcpy(kernelbuf + result, runtime->buffer + runtime->appl_ptr, count1);
+ memcpy(kernelbuf + result, runtime->buffer + appl_ptr, count1);
if (userbuf) {
spin_unlock_irqrestore(&runtime->lock, flags);
if (copy_to_user(userbuf + result,
- runtime->buffer + runtime->appl_ptr, count1)) {
+ runtime->buffer + appl_ptr, count1)) {
return result > 0 ? result : -EFAULT;
}
spin_lock_irqsave(&runtime->lock, flags);
}
- runtime->appl_ptr += count1;
- runtime->appl_ptr %= runtime->buffer_size;
- runtime->avail -= count1;
- spin_unlock_irqrestore(&runtime->lock, flags);
result += count1;
count -= count1;
}
+ spin_unlock_irqrestore(&runtime->lock, flags);
return result;
}
@@ -1207,6 +1212,7 @@ static long snd_rawmidi_kernel_write1(st
unsigned long flags;
long count1, result;
struct snd_rawmidi_runtime *runtime = substream->runtime;
+ unsigned long appl_ptr;
if (!kernelbuf && !userbuf)
return -EINVAL;
@@ -1227,12 +1233,19 @@ static long snd_rawmidi_kernel_write1(st
count1 = count;
if (count1 > (long)runtime->avail)
count1 = runtime->avail;
+
+ /* update runtime->appl_ptr before unlocking for userbuf */
+ appl_ptr = runtime->appl_ptr;
+ runtime->appl_ptr += count1;
+ runtime->appl_ptr %= runtime->buffer_size;
+ runtime->avail -= count1;
+
if (kernelbuf)
- memcpy(runtime->buffer + runtime->appl_ptr,
+ memcpy(runtime->buffer + appl_ptr,
kernelbuf + result, count1);
else if (userbuf) {
spin_unlock_irqrestore(&runtime->lock, flags);
- if (copy_from_user(runtime->buffer + runtime->appl_ptr,
+ if (copy_from_user(runtime->buffer + appl_ptr,
userbuf + result, count1)) {
spin_lock_irqsave(&runtime->lock, flags);
result = result > 0 ? result : -EFAULT;
@@ -1240,9 +1253,6 @@ static long snd_rawmidi_kernel_write1(st
}
spin_lock_irqsave(&runtime->lock, flags);
}
- runtime->appl_ptr += count1;
- runtime->appl_ptr %= runtime->buffer_size;
- runtime->avail -= count1;
result += count1;
count -= count1;
}
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 3.2 00/67] 3.2.78-rc1 review Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 10/67] sctp: allow setting SCTP_SACK_IMMEDIATELY by the application Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 14/67] USB: serial: option: Adding support for Telit LE922 Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 06/67] usb: cdc-acm: send zero packet for intel 7260 modem Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 02/67] hrtimer: Handle remaining time proper for TIME_LOW_RES Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 52/67] ALSA: dummy: Implement timer backend switching more safely Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 37/67] ALSA: rawmidi: Make snd_rawmidi_transmit() race-free Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 01/67] KVM: vmx: fix MPX detection Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 40/67] Revert "xhci: don't finish a TD if we get a short-transfer event mid TD" Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 38/67] ALSA: rawmidi: Fix race at copying & updating the position Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 41/67] usb: xhci: apply XHCI_PME_STUCK_QUIRK to Intel Broxton-M platforms Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 08/67] af_unix: fix struct pid memory leak Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 04/67] posix-timers: Handle relative timers with CONFIG_TIME_LOW_RES proper Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 45/67] [media] saa7134-alsa: Only frees registered sound cards Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 58/67] ahci: Intel DNV device IDs SATA Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 07/67] cdc-acm:exclude Samsung phone 04e8:685d Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 05/67] itimers: Handle relative timers with CONFIG_TIME_LOW_RES proper Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
[PATCH 3.2 46/67] scsi_dh_rdac: always retry MODE SELECT on command lock violation Ben Hutchings <ben@decadent.org.uk> - 2016-02-23 23:10 +0100
Re: [PATCH 3.2 00/67] 3.2.78-rc1 review Guenter Roeck <linux@roeck-us.net> - 2016-02-24 03:50 +0100
Re: [PATCH 3.2 00/67] 3.2.78-rc1 review Ben Hutchings <ben@decadent.org.uk> - 2016-02-24 15:50 +0100
csiph-web