lkml.org 
[lkml]   [2018]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.14 45/91] ALSA: seq: oss: Hardening for potential Spectre v1
    Date
    4.14-stable review patch.  If anyone has any objections, please let me know.

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

    From: Takashi Iwai <tiwai@suse.de>

    commit 8d218dd8116695ecda7164f97631c069938aa22e upstream.

    As Smatch recently suggested, a few places in OSS sequencer codes may
    expand the array directly from the user-space value with speculation,
    namely there are a significant amount of references to either
    info->ch[] or dp->synths[] array:

    sound/core/seq/oss/seq_oss_event.c:315 note_on_event() warn: potential spectre issue 'info->ch' (local cap)
    sound/core/seq/oss/seq_oss_event.c:362 note_off_event() warn: potential spectre issue 'info->ch' (local cap)
    sound/core/seq/oss/seq_oss_synth.c:470 snd_seq_oss_synth_load_patch() warn: potential spectre issue 'dp->synths' (local cap)
    sound/core/seq/oss/seq_oss_event.c:293 note_on_event() warn: potential spectre issue 'dp->synths'
    sound/core/seq/oss/seq_oss_event.c:353 note_off_event() warn: potential spectre issue 'dp->synths'
    sound/core/seq/oss/seq_oss_synth.c:506 snd_seq_oss_synth_sysex() warn: potential spectre issue 'dp->synths'
    sound/core/seq/oss/seq_oss_synth.c:580 snd_seq_oss_synth_ioctl() warn: potential spectre issue 'dp->synths'

    Although all these seem doing only the first load without further
    reference, we may want to stay in a safer side, so hardening with
    array_index_nospec() would still make sense.

    We may put array_index_nospec() at each place, but here we take a
    different approach:

    - For dp->synths[], change the helpers to retrieve seq_oss_synthinfo
    pointer directly instead of the array expansion at each place

    - For info->ch[], harden in a normal way, as there are only a couple
    of places

    As a result, the existing helper, snd_seq_oss_synth_is_valid() is
    replaced with snd_seq_oss_synth_info(). Also, we cover MIDI device
    where a similar array expansion is done, too, although it wasn't
    reported by Smatch.

    BugLink: https://marc.info/?l=linux-kernel&m=152411496503418&w=2
    Reported-by: Dan Carpenter <dan.carpenter@oracle.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/seq/oss/seq_oss_event.c | 15 ++++---
    sound/core/seq/oss/seq_oss_midi.c | 2
    sound/core/seq/oss/seq_oss_synth.c | 75 ++++++++++++++++++++-----------------
    sound/core/seq/oss/seq_oss_synth.h | 3 -
    4 files changed, 55 insertions(+), 40 deletions(-)

    --- a/sound/core/seq/oss/seq_oss_event.c
    +++ b/sound/core/seq/oss/seq_oss_event.c
    @@ -26,6 +26,7 @@
    #include <sound/seq_oss_legacy.h>
    #include "seq_oss_readq.h"
    #include "seq_oss_writeq.h"
    +#include <linux/nospec.h>


    /*
    @@ -287,10 +288,10 @@ note_on_event(struct seq_oss_devinfo *dp
    {
    struct seq_oss_synthinfo *info;

    - if (!snd_seq_oss_synth_is_valid(dp, dev))
    + info = snd_seq_oss_synth_info(dp, dev);
    + if (!info)
    return -ENXIO;

    - info = &dp->synths[dev];
    switch (info->arg.event_passing) {
    case SNDRV_SEQ_OSS_PROCESS_EVENTS:
    if (! info->ch || ch < 0 || ch >= info->nr_voices) {
    @@ -298,6 +299,7 @@ note_on_event(struct seq_oss_devinfo *dp
    return set_note_event(dp, dev, SNDRV_SEQ_EVENT_NOTEON, ch, note, vel, ev);
    }

    + ch = array_index_nospec(ch, info->nr_voices);
    if (note == 255 && info->ch[ch].note >= 0) {
    /* volume control */
    int type;
    @@ -347,10 +349,10 @@ note_off_event(struct seq_oss_devinfo *d
    {
    struct seq_oss_synthinfo *info;

    - if (!snd_seq_oss_synth_is_valid(dp, dev))
    + info = snd_seq_oss_synth_info(dp, dev);
    + if (!info)
    return -ENXIO;

    - info = &dp->synths[dev];
    switch (info->arg.event_passing) {
    case SNDRV_SEQ_OSS_PROCESS_EVENTS:
    if (! info->ch || ch < 0 || ch >= info->nr_voices) {
    @@ -358,6 +360,7 @@ note_off_event(struct seq_oss_devinfo *d
    return set_note_event(dp, dev, SNDRV_SEQ_EVENT_NOTEON, ch, note, vel, ev);
    }

    + ch = array_index_nospec(ch, info->nr_voices);
    if (info->ch[ch].note >= 0) {
    note = info->ch[ch].note;
    info->ch[ch].vel = 0;
    @@ -381,7 +384,7 @@ note_off_event(struct seq_oss_devinfo *d
    static int
    set_note_event(struct seq_oss_devinfo *dp, int dev, int type, int ch, int note, int vel, struct snd_seq_event *ev)
    {
    - if (! snd_seq_oss_synth_is_valid(dp, dev))
    + if (!snd_seq_oss_synth_info(dp, dev))
    return -ENXIO;

    ev->type = type;
    @@ -399,7 +402,7 @@ set_note_event(struct seq_oss_devinfo *d
    static int
    set_control_event(struct seq_oss_devinfo *dp, int dev, int type, int ch, int param, int val, struct snd_seq_event *ev)
    {
    - if (! snd_seq_oss_synth_is_valid(dp, dev))
    + if (!snd_seq_oss_synth_info(dp, dev))
    return -ENXIO;

    ev->type = type;
    --- a/sound/core/seq/oss/seq_oss_midi.c
    +++ b/sound/core/seq/oss/seq_oss_midi.c
    @@ -29,6 +29,7 @@
    #include "../seq_lock.h"
    #include <linux/init.h>
    #include <linux/slab.h>
    +#include <linux/nospec.h>


    /*
    @@ -315,6 +316,7 @@ get_mididev(struct seq_oss_devinfo *dp,
    {
    if (dev < 0 || dev >= dp->max_mididev)
    return NULL;
    + dev = array_index_nospec(dev, dp->max_mididev);
    return get_mdev(dev);
    }

    --- a/sound/core/seq/oss/seq_oss_synth.c
    +++ b/sound/core/seq/oss/seq_oss_synth.c
    @@ -26,6 +26,7 @@
    #include <linux/init.h>
    #include <linux/module.h>
    #include <linux/slab.h>
    +#include <linux/nospec.h>

    /*
    * constants
    @@ -339,17 +340,13 @@ snd_seq_oss_synth_cleanup(struct seq_oss
    dp->max_synthdev = 0;
    }

    -/*
    - * check if the specified device is MIDI mapped device
    - */
    -static int
    -is_midi_dev(struct seq_oss_devinfo *dp, int dev)
    +static struct seq_oss_synthinfo *
    +get_synthinfo_nospec(struct seq_oss_devinfo *dp, int dev)
    {
    if (dev < 0 || dev >= dp->max_synthdev)
    - return 0;
    - if (dp->synths[dev].is_midi)
    - return 1;
    - return 0;
    + return NULL;
    + dev = array_index_nospec(dev, SNDRV_SEQ_OSS_MAX_SYNTH_DEVS);
    + return &dp->synths[dev];
    }

    /*
    @@ -359,11 +356,13 @@ static struct seq_oss_synth *
    get_synthdev(struct seq_oss_devinfo *dp, int dev)
    {
    struct seq_oss_synth *rec;
    - if (dev < 0 || dev >= dp->max_synthdev)
    + struct seq_oss_synthinfo *info = get_synthinfo_nospec(dp, dev);
    +
    + if (!info)
    return NULL;
    - if (! dp->synths[dev].opened)
    + if (!info->opened)
    return NULL;
    - if (dp->synths[dev].is_midi) {
    + if (info->is_midi) {
    rec = &midi_synth_dev;
    snd_use_lock_use(&rec->use_lock);
    } else {
    @@ -406,10 +405,8 @@ snd_seq_oss_synth_reset(struct seq_oss_d
    struct seq_oss_synth *rec;
    struct seq_oss_synthinfo *info;

    - if (snd_BUG_ON(dev < 0 || dev >= dp->max_synthdev))
    - return;
    - info = &dp->synths[dev];
    - if (! info->opened)
    + info = get_synthinfo_nospec(dp, dev);
    + if (!info || !info->opened)
    return;
    if (info->sysex)
    info->sysex->len = 0; /* reset sysex */
    @@ -458,12 +455,14 @@ snd_seq_oss_synth_load_patch(struct seq_
    const char __user *buf, int p, int c)
    {
    struct seq_oss_synth *rec;
    + struct seq_oss_synthinfo *info;
    int rc;

    - if (dev < 0 || dev >= dp->max_synthdev)
    + info = get_synthinfo_nospec(dp, dev);
    + if (!info)
    return -ENXIO;

    - if (is_midi_dev(dp, dev))
    + if (info->is_midi)
    return 0;
    if ((rec = get_synthdev(dp, dev)) == NULL)
    return -ENXIO;
    @@ -471,24 +470,25 @@ snd_seq_oss_synth_load_patch(struct seq_
    if (rec->oper.load_patch == NULL)
    rc = -ENXIO;
    else
    - rc = rec->oper.load_patch(&dp->synths[dev].arg, fmt, buf, p, c);
    + rc = rec->oper.load_patch(&info->arg, fmt, buf, p, c);
    snd_use_lock_free(&rec->use_lock);
    return rc;
    }

    /*
    - * check if the device is valid synth device
    + * check if the device is valid synth device and return the synth info
    */
    -int
    -snd_seq_oss_synth_is_valid(struct seq_oss_devinfo *dp, int dev)
    +struct seq_oss_synthinfo *
    +snd_seq_oss_synth_info(struct seq_oss_devinfo *dp, int dev)
    {
    struct seq_oss_synth *rec;
    +
    rec = get_synthdev(dp, dev);
    if (rec) {
    snd_use_lock_free(&rec->use_lock);
    - return 1;
    + return get_synthinfo_nospec(dp, dev);
    }
    - return 0;
    + return NULL;
    }


    @@ -503,16 +503,18 @@ snd_seq_oss_synth_sysex(struct seq_oss_d
    int i, send;
    unsigned char *dest;
    struct seq_oss_synth_sysex *sysex;
    + struct seq_oss_synthinfo *info;

    - if (! snd_seq_oss_synth_is_valid(dp, dev))
    + info = snd_seq_oss_synth_info(dp, dev);
    + if (!info)
    return -ENXIO;

    - sysex = dp->synths[dev].sysex;
    + sysex = info->sysex;
    if (sysex == NULL) {
    sysex = kzalloc(sizeof(*sysex), GFP_KERNEL);
    if (sysex == NULL)
    return -ENOMEM;
    - dp->synths[dev].sysex = sysex;
    + info->sysex = sysex;
    }

    send = 0;
    @@ -557,10 +559,12 @@ snd_seq_oss_synth_sysex(struct seq_oss_d
    int
    snd_seq_oss_synth_addr(struct seq_oss_devinfo *dp, int dev, struct snd_seq_event *ev)
    {
    - if (! snd_seq_oss_synth_is_valid(dp, dev))
    + struct seq_oss_synthinfo *info = snd_seq_oss_synth_info(dp, dev);
    +
    + if (!info)
    return -EINVAL;
    - snd_seq_oss_fill_addr(dp, ev, dp->synths[dev].arg.addr.client,
    - dp->synths[dev].arg.addr.port);
    + snd_seq_oss_fill_addr(dp, ev, info->arg.addr.client,
    + info->arg.addr.port);
    return 0;
    }

    @@ -572,16 +576,18 @@ int
    snd_seq_oss_synth_ioctl(struct seq_oss_devinfo *dp, int dev, unsigned int cmd, unsigned long addr)
    {
    struct seq_oss_synth *rec;
    + struct seq_oss_synthinfo *info;
    int rc;

    - if (is_midi_dev(dp, dev))
    + info = get_synthinfo_nospec(dp, dev);
    + if (!info || info->is_midi)
    return -ENXIO;
    if ((rec = get_synthdev(dp, dev)) == NULL)
    return -ENXIO;
    if (rec->oper.ioctl == NULL)
    rc = -ENXIO;
    else
    - rc = rec->oper.ioctl(&dp->synths[dev].arg, cmd, addr);
    + rc = rec->oper.ioctl(&info->arg, cmd, addr);
    snd_use_lock_free(&rec->use_lock);
    return rc;
    }
    @@ -593,7 +599,10 @@ snd_seq_oss_synth_ioctl(struct seq_oss_d
    int
    snd_seq_oss_synth_raw_event(struct seq_oss_devinfo *dp, int dev, unsigned char *data, struct snd_seq_event *ev)
    {
    - if (! snd_seq_oss_synth_is_valid(dp, dev) || is_midi_dev(dp, dev))
    + struct seq_oss_synthinfo *info;
    +
    + info = snd_seq_oss_synth_info(dp, dev);
    + if (!info || info->is_midi)
    return -ENXIO;
    ev->type = SNDRV_SEQ_EVENT_OSS;
    memcpy(ev->data.raw8.d, data, 8);
    --- a/sound/core/seq/oss/seq_oss_synth.h
    +++ b/sound/core/seq/oss/seq_oss_synth.h
    @@ -37,7 +37,8 @@ void snd_seq_oss_synth_cleanup(struct se
    void snd_seq_oss_synth_reset(struct seq_oss_devinfo *dp, int dev);
    int snd_seq_oss_synth_load_patch(struct seq_oss_devinfo *dp, int dev, int fmt,
    const char __user *buf, int p, int c);
    -int snd_seq_oss_synth_is_valid(struct seq_oss_devinfo *dp, int dev);
    +struct seq_oss_synthinfo *snd_seq_oss_synth_info(struct seq_oss_devinfo *dp,
    + int dev);
    int snd_seq_oss_synth_sysex(struct seq_oss_devinfo *dp, int dev, unsigned char *buf,
    struct snd_seq_event *ev);
    int snd_seq_oss_synth_addr(struct seq_oss_devinfo *dp, int dev, struct snd_seq_event *ev);

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