Commit 8d218dd8 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

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: default avatarDan Carpenter <dan.carpenter@oracle.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent f5e94b4c

sound/core/seq/oss/seq_oss_event.c

+9 −6
Original line number Diff line number Diff line
@@ -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;

sound/core/seq/oss/seq_oss_midi.c

+2 −0
Original line number Diff line number Diff line
@@ -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);

}

sound/core/seq/oss/seq_oss_synth.c

+42 −33
Original line number Diff line number Diff line
@@ -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);

sound/core/seq/oss/seq_oss_synth.h

+2 −1
Original line number Diff line number Diff line
@@ -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);

CRA Git | Maintained and supported by SUSTech CRA and CCSE