lkml.org 
[lkml]   [2018]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 3.18 33/52] ALSA: pcm: Avoid potential races between OSS ioctls and read/write
    Date
    3.18-stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Takashi Iwai <tiwai@suse.de>

    commit 02a5d6925cd34c3b774bdb8eefb057c40a30e870 upstream.

    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>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    sound/core/oss/pcm_oss.c | 134 +++++++++++++++++++++++++++++++++++++----------
    1 file changed, 106 insertions(+), 28 deletions(-)

    --- a/sound/core/oss/pcm_oss.c
    +++ b/sound/core/oss/pcm_oss.c
    @@ -833,8 +833,8 @@ static int choose_rate(struct snd_pcm_su
    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;
    @@ -848,11 +848,8 @@ static int snd_pcm_oss_change_params(str
    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);
    @@ -1079,6 +1076,23 @@ failure:
    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;
    }
    @@ -1107,11 +1121,14 @@ static int snd_pcm_oss_get_active_substr
    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,
    @@ -1131,8 +1148,6 @@ static int snd_pcm_oss_make_ready(struct
    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);
    @@ -1140,6 +1155,29 @@ static int snd_pcm_oss_make_ready(struct
    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)
    return err;
    @@ -1367,13 +1405,14 @@ static ssize_t snd_pcm_oss_write1(struct
    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)
    @@ -1474,13 +1513,14 @@ static ssize_t snd_pcm_oss_read1(struct
    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);
    @@ -1536,10 +1576,12 @@ static int snd_pcm_oss_reset(struct snd_
    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;
    }
    @@ -1625,9 +1667,10 @@ static int snd_pcm_oss_sync(struct snd_p
    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");
    @@ -1695,7 +1738,9 @@ static int snd_pcm_oss_sync(struct snd_p
    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];
    @@ -1706,8 +1751,10 @@ static int snd_pcm_oss_sync(struct snd_p
    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;
    }
    @@ -1726,10 +1773,13 @@ static int snd_pcm_oss_set_rate(struct s
    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);
    }
    @@ -1757,10 +1807,13 @@ static int snd_pcm_oss_set_channels(stru
    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);
    }
    @@ -1846,10 +1899,13 @@ static int snd_pcm_oss_set_format(struct
    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);
    @@ -1869,8 +1925,6 @@ static int snd_pcm_oss_set_subdivide1(st
    {
    struct snd_pcm_runtime *runtime;

    - if (substream == NULL)
    - return 0;
    runtime = substream->runtime;
    if (subdivide == 0) {
    subdivide = runtime->oss.subdivision;
    @@ -1894,9 +1948,16 @@ static int snd_pcm_oss_set_subdivide(str

    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;
    @@ -1906,8 +1967,6 @@ static int snd_pcm_oss_set_fragment1(str
    {
    struct snd_pcm_runtime *runtime;

    - if (substream == NULL)
    - return 0;
    runtime = substream->runtime;
    if (runtime->oss.subdivision || runtime->oss.fragshift)
    return -EINVAL;
    @@ -1927,9 +1986,16 @@ static int snd_pcm_oss_set_fragment(stru

    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;
    @@ -2013,6 +2079,9 @@ static int snd_pcm_oss_set_trigger(struc
    }
    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;
    @@ -2030,13 +2099,19 @@ static int snd_pcm_oss_set_trigger(struc
    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;
    @@ -2051,11 +2126,14 @@ static int snd_pcm_oss_set_trigger(struc
    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;
    }


    \
     
     \ /
      Last update: 2018-04-22 16:28    [W:4.217 / U:1.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site