Skip to content

Commit

Permalink
ALSA: pcm: Avoid potential races between OSS ioctls and read/write
Browse files Browse the repository at this point in the history 
Although we apply the params_lock mutex to the whole read and write
operations as well as snd_pcm_oss_change_params(), we may still face
some races.

First off, the params_lock is taken inside the read and write loop.
This is intentional for avoiding the too long locking, but it allows
the in-between parameter change, which might lead to invalid
pointers.  We check the readiness of the stream and set up via
snd_pcm_oss_make_ready() at the beginning of read and write, but it's
called only once, by assuming that it remains ready in the rest.

Second, many ioctls that may change the actual parameters
(i.e. setting runtime->oss.params=1) aren't protected, hence they can
be processed in a half-baked state.

This patch is an attempt to plug these holes.  The stream readiness
check is moved inside the read/write inner loop, so that the stream is
always set up in a proper state before further processing.  Also, each
ioctl that may change the parameter is wrapped with the params_lock
for avoiding the races.

The issues were triggered by syzkaller in a few different scenarios,
particularly the one below appearing as GPF in loopback_pos_update.

Reported-by: syzbot+c4227aec125487ec3efa@syzkaller.appspotmail.com
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
  • Loading branch information
@tiwai
tiwai committed Mar 23, 2018
1 parent f3b906d commit 02a5d69
Showing 1 changed file with 106 additions and 28 deletions.
134 changes: 106 additions & 28 deletions sound/core/oss/pcm_oss.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -823,8 +823,8 @@ static int choose_rate(struct snd_pcm_substream *substream,
return snd_pcm_hw_param_near(substream, params, SNDRV_PCM_HW_PARAM_RATE, best_rate, NULL);
}

static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream,
bool trylock)
/* call with params_lock held */
static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream)
{
struct snd_pcm_runtime *runtime = substream->runtime;
struct snd_pcm_hw_params *params, *sparams;
Expand All @@ -838,11 +838,8 @@ static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream,
const struct snd_mask *sformat_mask;
struct snd_mask mask;

if (trylock) {
if (!(mutex_trylock(&runtime->oss.params_lock)))
return -EAGAIN;
} else if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
if (!runtime->oss.params)
return 0;
sw_params = kzalloc(sizeof(*sw_params), GFP_KERNEL);
params = kmalloc(sizeof(*params), GFP_KERNEL);
sparams = kmalloc(sizeof(*sparams), GFP_KERNEL);
Expand Down Expand Up @@ -1068,6 +1065,23 @@ static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream,
kfree(sw_params);
kfree(params);
kfree(sparams);
return err;
}

/* this one takes the lock by itself */
static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream,
bool trylock)
{
struct snd_pcm_runtime *runtime = substream->runtime;
int err;

if (trylock) {
if (!(mutex_trylock(&runtime->oss.params_lock)))
return -EAGAIN;
} else if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;

err = snd_pcm_oss_change_params_locked(substream);
mutex_unlock(&runtime->oss.params_lock);
return err;
}
Expand Down Expand Up @@ -1096,11 +1110,14 @@ static int snd_pcm_oss_get_active_substream(struct snd_pcm_oss_file *pcm_oss_fil
return 0;
}

/* call with params_lock held */
static int snd_pcm_oss_prepare(struct snd_pcm_substream *substream)
{
int err;
struct snd_pcm_runtime *runtime = substream->runtime;

if (!runtime->oss.prepare)
return 0;
err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_PREPARE, NULL);
if (err < 0) {
pcm_dbg(substream->pcm,
Expand All @@ -1120,14 +1137,35 @@ static int snd_pcm_oss_make_ready(struct snd_pcm_substream *substream)
struct snd_pcm_runtime *runtime;
int err;

if (substream == NULL)
return 0;
runtime = substream->runtime;
if (runtime->oss.params) {
err = snd_pcm_oss_change_params(substream, false);
if (err < 0)
return err;
}
if (runtime->oss.prepare) {
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
err = snd_pcm_oss_prepare(substream);
mutex_unlock(&runtime->oss.params_lock);
if (err < 0)
return err;
}
return 0;
}

/* call with params_lock held */
static int snd_pcm_oss_make_ready_locked(struct snd_pcm_substream *substream)
{
struct snd_pcm_runtime *runtime;
int err;

runtime = substream->runtime;
if (runtime->oss.params) {
err = snd_pcm_oss_change_params_locked(substream);
if (err < 0)
return err;
}
if (runtime->oss.prepare) {
err = snd_pcm_oss_prepare(substream);
if (err < 0)
Expand Down Expand Up @@ -1332,13 +1370,14 @@ static ssize_t snd_pcm_oss_write1(struct snd_pcm_substream *substream, const cha
if (atomic_read(&substream->mmap_count))
return -ENXIO;

if ((tmp = snd_pcm_oss_make_ready(substream)) < 0)
return tmp;
while (bytes > 0) {
if (mutex_lock_interruptible(&runtime->oss.params_lock)) {
tmp = -ERESTARTSYS;
break;
}
tmp = snd_pcm_oss_make_ready_locked(substream);
if (tmp < 0)
goto err;
if (bytes < runtime->oss.period_bytes || runtime->oss.buffer_used > 0) {
tmp = bytes;
if (tmp + runtime->oss.buffer_used > runtime->oss.period_bytes)
Expand Down Expand Up @@ -1439,13 +1478,14 @@ static ssize_t snd_pcm_oss_read1(struct snd_pcm_substream *substream, char __use
if (atomic_read(&substream->mmap_count))
return -ENXIO;

if ((tmp = snd_pcm_oss_make_ready(substream)) < 0)
return tmp;
while (bytes > 0) {
if (mutex_lock_interruptible(&runtime->oss.params_lock)) {
tmp = -ERESTARTSYS;
break;
}
tmp = snd_pcm_oss_make_ready_locked(substream);
if (tmp < 0)
goto err;
if (bytes < runtime->oss.period_bytes || runtime->oss.buffer_used > 0) {
if (runtime->oss.buffer_used == 0) {
tmp = snd_pcm_oss_read2(substream, runtime->oss.buffer, runtime->oss.period_bytes, 1);
Expand Down Expand Up @@ -1501,10 +1541,12 @@ static int snd_pcm_oss_reset(struct snd_pcm_oss_file *pcm_oss_file)
continue;
runtime = substream->runtime;
snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
mutex_lock(&runtime->oss.params_lock);
runtime->oss.prepare = 1;
runtime->oss.buffer_used = 0;
runtime->oss.prev_hw_ptr_period = 0;
runtime->oss.period_ptr = 0;
mutex_unlock(&runtime->oss.params_lock);
}
return 0;
}
Expand Down Expand Up @@ -1590,9 +1632,10 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file)
goto __direct;
if ((err = snd_pcm_oss_make_ready(substream)) < 0)
return err;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
format = snd_pcm_oss_format_from(runtime->oss.format);
width = snd_pcm_format_physical_width(format);
mutex_lock(&runtime->oss.params_lock);
if (runtime->oss.buffer_used > 0) {
#ifdef OSS_DEBUG
pcm_dbg(substream->pcm, "sync: buffer_used\n");
Expand Down Expand Up @@ -1643,7 +1686,9 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file)
substream->f_flags = saved_f_flags;
if (err < 0)
return err;
mutex_lock(&runtime->oss.params_lock);
runtime->oss.prepare = 1;
mutex_unlock(&runtime->oss.params_lock);
}

substream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE];
Expand All @@ -1654,8 +1699,10 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file)
err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
if (err < 0)
return err;
mutex_lock(&runtime->oss.params_lock);
runtime->oss.buffer_used = 0;
runtime->oss.prepare = 1;
mutex_unlock(&runtime->oss.params_lock);
}
return 0;
}
Expand All @@ -1674,10 +1721,13 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate)
rate = 1000;
else if (rate > 192000)
rate = 192000;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
if (runtime->oss.rate != rate) {
runtime->oss.params = 1;
runtime->oss.rate = rate;
}
mutex_unlock(&runtime->oss.params_lock);
}
return snd_pcm_oss_get_rate(pcm_oss_file);
}
Expand Down Expand Up @@ -1705,10 +1755,13 @@ static int snd_pcm_oss_set_channels(struct snd_pcm_oss_file *pcm_oss_file, unsig
if (substream == NULL)
continue;
runtime = substream->runtime;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
if (runtime->oss.channels != channels) {
runtime->oss.params = 1;
runtime->oss.channels = channels;
}
mutex_unlock(&runtime->oss.params_lock);
}
return snd_pcm_oss_get_channels(pcm_oss_file);
}
Expand Down Expand Up @@ -1794,10 +1847,13 @@ static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int for
if (substream == NULL)
continue;
runtime = substream->runtime;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
if (runtime->oss.format != format) {
runtime->oss.params = 1;
runtime->oss.format = format;
}
mutex_unlock(&runtime->oss.params_lock);
}
}
return snd_pcm_oss_get_format(pcm_oss_file);
Expand All @@ -1817,8 +1873,6 @@ static int snd_pcm_oss_set_subdivide1(struct snd_pcm_substream *substream, int s
{
struct snd_pcm_runtime *runtime;

if (substream == NULL)
return 0;
runtime = substream->runtime;
if (subdivide == 0) {
subdivide = runtime->oss.subdivision;
Expand All @@ -1842,9 +1896,16 @@ static int snd_pcm_oss_set_subdivide(struct snd_pcm_oss_file *pcm_oss_file, int

for (idx = 1; idx >= 0; --idx) {
struct snd_pcm_substream *substream = pcm_oss_file->streams[idx];
struct snd_pcm_runtime *runtime;

if (substream == NULL)
continue;
if ((err = snd_pcm_oss_set_subdivide1(substream, subdivide)) < 0)
runtime = substream->runtime;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
err = snd_pcm_oss_set_subdivide1(substream, subdivide);
mutex_unlock(&runtime->oss.params_lock);
if (err < 0)
return err;
}
return err;
Expand All @@ -1854,8 +1915,6 @@ static int snd_pcm_oss_set_fragment1(struct snd_pcm_substream *substream, unsign
{
struct snd_pcm_runtime *runtime;

if (substream == NULL)
return 0;
runtime = substream->runtime;
if (runtime->oss.subdivision || runtime->oss.fragshift)
return -EINVAL;
Expand All @@ -1875,9 +1934,16 @@ static int snd_pcm_oss_set_fragment(struct snd_pcm_oss_file *pcm_oss_file, unsig

for (idx = 1; idx >= 0; --idx) {
struct snd_pcm_substream *substream = pcm_oss_file->streams[idx];
struct snd_pcm_runtime *runtime;

if (substream == NULL)
continue;
if ((err = snd_pcm_oss_set_fragment1(substream, val)) < 0)
runtime = substream->runtime;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
err = snd_pcm_oss_set_fragment1(substream, val);
mutex_unlock(&runtime->oss.params_lock);
if (err < 0)
return err;
}
return err;
Expand Down Expand Up @@ -1961,6 +2027,9 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr
}
if (psubstream) {
runtime = psubstream->runtime;
cmd = 0;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
if (trigger & PCM_ENABLE_OUTPUT) {
if (runtime->oss.trigger)
goto _skip1;
Expand All @@ -1978,13 +2047,19 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr
cmd = SNDRV_PCM_IOCTL_DROP;
runtime->oss.prepare = 1;
}
err = snd_pcm_kernel_ioctl(psubstream, cmd, NULL);
if (err < 0)
return err;
}
_skip1:
mutex_unlock(&runtime->oss.params_lock);
if (cmd) {
err = snd_pcm_kernel_ioctl(psubstream, cmd, NULL);
if (err < 0)
return err;
}
}
if (csubstream) {
runtime = csubstream->runtime;
cmd = 0;
if (mutex_lock_interruptible(&runtime->oss.params_lock))
return -ERESTARTSYS;
if (trigger & PCM_ENABLE_INPUT) {
if (runtime->oss.trigger)
goto _skip2;
Expand All @@ -1999,11 +2074,14 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr
cmd = SNDRV_PCM_IOCTL_DROP;
runtime->oss.prepare = 1;
}
err = snd_pcm_kernel_ioctl(csubstream, cmd, NULL);
if (err < 0)
return err;
}
_skip2:
mutex_unlock(&runtime->oss.params_lock);
if (cmd) {
err = snd_pcm_kernel_ioctl(csubstream, cmd, NULL);
if (err < 0)
return err;
}
}
return 0;
}

Expand Down

0 comments on commit 02a5d69

Please sign in to comment.