ALSA: seq: oss: Hardening for potential Spectre v1

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>

diff --git a/sound/core/seq/oss/seq_oss_event.c b/sound/core/seq/oss/seq_oss_event.c
index c3908862bc8b63932aaa8724a50d7a5ae19a8cf3..86ca584c27b28081663e8493e7b54b5d0009784d 100644 (file)
--- 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, int dev, int ch, int note, int vel, st
 {
        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, int dev, int ch, int note, int vel, st
                        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 *dp, int dev, int ch, int note, int vel, s
 {
        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 *dp, int dev, int ch, int note, int vel, s
                        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 *dp, int dev, int ch, int note, int vel, s
 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 *dp, int dev, int type, int ch, int note,
 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;

diff --git a/sound/core/seq/oss/seq_oss_midi.c b/sound/core/seq/oss/seq_oss_midi.c
index b30b2139e3f033fd71e59fbe10cfa1e25340adc6..9debd1b8fd2880fde1e0fe349a4745bdb443b477 100644 (file)
--- 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, int dev)
 {
        if (dev < 0 || dev >= dp->max_mididev)
                return NULL;
+       dev = array_index_nospec(dev, dp->max_mididev);
        return get_mdev(dev);
 }
 

diff --git a/sound/core/seq/oss/seq_oss_synth.c b/sound/core/seq/oss/seq_oss_synth.c
index 9e2b250ae78059b22eb2e2160abee78eb8ae0ead..278ebb9931225998dd07f0606eeabe289d71aff5 100644 (file)
--- 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_devinfo *dp)
        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_devinfo *dp, int dev)
        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_oss_devinfo *dp, int dev, int fmt,
                            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_oss_devinfo *dp, int dev, int fmt,
        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_devinfo *dp, int dev, unsigned char *buf,
        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_devinfo *dp, int dev, unsigned char *buf,
 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_devinfo *dp, int dev, unsigned int cmd, u
 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);

diff --git a/sound/core/seq/oss/seq_oss_synth.h b/sound/core/seq/oss/seq_oss_synth.h
index 74ac55f166b6517751d197c14a5aaf4b6805ca01..a63f9e22974dfb33ff0f309f0ab309625e0265ea 100644 (file)
--- 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 seq_oss_devinfo *dp);
 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);