From 7982e7a46543d924673edff37e1fe119a5ec5219 Mon Sep 17 00:00:00 2001 From: Ariff Abdullah Date: Thu, 16 Mar 2006 04:12:49 +0000 Subject: Fix severe 8bit integer overflow during channel creation and destruction, especially for vchans. It turns out that channel numbering always depend on d->devcount counter (which keep increasing), while PCMMKMINOR() truncate everything to 8bit length. At some point the truncation cause the newly created character device overlapped with the existence one, causing erratic overall system behaviour and panic. Easily reproduce with something like: (Luckily, only root can reproduce this) while : ; do sysctl hw.snd.pcm0.vchans=200 sysctl hw.snd.pcm0.vchans=100 done - Enforce channel/chardev numbering within 8bit boundary. Return E2BIG if necessary. - Traverse d->channels SLIST and try to reclaim "free" counter during channel creation. Don't rely on d->devcount at all. - Destroy vchans in reverse order. Anyway, this is not the fault of vchans. It is just that vchans are so cute and begging to be abused ;) . Don't blame her. Old, hidden bugs.. sigh.. MFC after: 3 days --- sys/dev/sound/pcm/sound.c | 105 ++++++++++++++++++++++++++++++++-------------- sys/dev/sound/pcm/sound.h | 17 +++++--- sys/dev/sound/pcm/vchan.c | 21 +++++++--- 3 files changed, 99 insertions(+), 44 deletions(-) diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c index ec27a3887940..c984bc52d46b 100644 --- a/sys/dev/sound/pcm/sound.c +++ b/sys/dev/sound/pcm/sound.c @@ -181,23 +181,23 @@ pcm_chnalloc(struct snddev_info *d, int direction, pid_t pid, int chnum) } /* no channel available */ - if (direction == PCMDIR_PLAY) { - if ((d->vchancount > 0) && (d->vchancount < snd_maxautovchans)) { - /* try to create a vchan */ - SLIST_FOREACH(sce, &d->channels, link) { - c = sce->channel; - CHN_LOCK(c); - if ((c->flags & CHN_F_HAS_VCHAN) && - !SLIST_EMPTY(&c->children)) { - err = vchan_create(c); - CHN_UNLOCK(c); - if (!err) - return pcm_chnalloc(d, direction, pid, -1); - else - device_printf(d->dev, "vchan_create(%s) == %d\n", c->name, err); - } else - CHN_UNLOCK(c); - } + if (direction == PCMDIR_PLAY && d->vchancount > 0 && + d->vchancount < snd_maxautovchans && + d->devcount <= PCMMAXCHAN) { + /* try to create a vchan */ + SLIST_FOREACH(sce, &d->channels, link) { + c = sce->channel; + CHN_LOCK(c); + if ((c->flags & CHN_F_HAS_VCHAN) && + !SLIST_EMPTY(&c->children)) { + err = vchan_create(c); + CHN_UNLOCK(c); + if (!err) + return pcm_chnalloc(d, direction, pid, -1); + else + device_printf(d->dev, "vchan_create(%s) == %d\n", c->name, err); + } else + CHN_UNLOCK(c); } } @@ -338,7 +338,7 @@ sysctl_hw_snd_maxautovchans(SYSCTL_HANDLER_ARGS) v = snd_maxautovchans; error = sysctl_handle_int(oidp, &v, sizeof(v), req); if (error == 0 && req->newptr != NULL) { - if (v < 0 || v >= SND_MAXVCHANS || pcm_devclass == NULL) + if (v < 0 || v > (PCMMAXCHAN + 1) || pcm_devclass == NULL) return EINVAL; if (v != snd_maxautovchans) { for (i = 0; i < devclass_get_maxunit(pcm_devclass); i++) { @@ -467,10 +467,31 @@ pcm_chn_add(struct snddev_info *d, struct pcm_channel *ch) snd_mtxlock(d->lock); sce->channel = ch; - sce->chan_num= d->devcount++; if (SLIST_EMPTY(&d->channels)) { SLIST_INSERT_HEAD(&d->channels, sce, link); + sce->chan_num = 0; } else { + sce->chan_num = 0; +retry_search: + SLIST_FOREACH(tmp, &d->channels, link) { + if (tmp == NULL) + continue; + if (sce->chan_num == tmp->chan_num) { + sce->chan_num++; + goto retry_search; + } + } + /* + * Don't overflow PCMMKMINOR / PCMMAXCHAN. + */ + if (sce->chan_num > PCMMAXCHAN) { + snd_mtxunlock(d->lock); + device_printf(d->dev, + "%s: WARNING: sce->chan_num overflow! (%d)\n", + __func__, sce->chan_num); + free(sce, M_DEVBUF); + return E2BIG; + } /* * Micro optimization, channel ordering: * hw,hw,hw,vch,vch,vch,rec @@ -503,6 +524,7 @@ pcm_chn_add(struct snddev_info *d, struct pcm_channel *ch) SLIST_INSERT_AFTER(after, sce, link); } } + d->devcount++; snd_mtxunlock(d->lock); sce->dsp_devt= make_dev(&dsp_cdevsw, PCMMKMINOR(device, SND_DEV_DSP, sce->chan_num), @@ -788,14 +810,23 @@ pcm_unregister(device_t dev) } SLIST_FOREACH(sce, &d->channels, link) { - if (sce->dsp_devt) + if (sce->dsp_devt) { destroy_dev(sce->dsp_devt); - if (sce->dspW_devt) + sce->dsp_devt = NULL; + } + if (sce->dspW_devt) { destroy_dev(sce->dspW_devt); - if (sce->audio_devt) + sce->dspW_devt = NULL; + } + if (sce->audio_devt) { destroy_dev(sce->audio_devt); - if (sce->dspr_devt) + sce->audio_devt = NULL; + } + if (sce->dspr_devt) { destroy_dev(sce->dspr_devt); + sce->dspr_devt = NULL; + } + d->devcount--; } #ifdef SND_DYNSYSCTL @@ -968,7 +999,8 @@ sysctl_hw_snd_vchans(SYSCTL_HANDLER_ARGS) if (err == 0 && req->newptr != NULL) { - if (newcnt < 0 || newcnt > SND_MAXVCHANS) + if (newcnt < 0 || newcnt > (PCMMAXCHAN + 1) || + (d->playcount + d->reccount + newcnt) > (PCMMAXCHAN + 1)) return E2BIG; if (pcm_inprog(d, 1) != 1) { @@ -1008,28 +1040,37 @@ next: addok: c->flags |= CHN_F_BUSY; while (err == 0 && newcnt > cnt) { + if (d->devcount > PCMMAXCHAN) { + device_printf(d->dev, "%s: Maximum channel reached.\n", __func__); + break; + } err = vchan_create(c); if (err == 0) cnt++; + if (newcnt > cnt && err == E2BIG) { + device_printf(d->dev, "%s: err=%d Maximum channel reached.\n", __func__, err); + err = 0; + break; + } } CHN_UNLOCK(c); } else if (newcnt < cnt) { snd_mtxlock(d->lock); while (err == 0 && newcnt < cnt) { + c = NULL; SLIST_FOREACH(sce, &d->channels, link) { - c = sce->channel; - CHN_LOCK(c); - if (c->direction == PCMDIR_PLAY && - (c->flags & (CHN_F_BUSY | CHN_F_VIRTUAL)) == CHN_F_VIRTUAL) - goto remok; - - CHN_UNLOCK(c); + CHN_LOCK(sce->channel); + if (sce->channel->direction == PCMDIR_PLAY && + (sce->channel->flags & CHN_F_VIRTUAL)) + c = sce->channel; + CHN_UNLOCK(sce->channel); } + if (c != NULL) + goto remok; snd_mtxunlock(d->lock); pcm_inprog(d, -1); return EINVAL; remok: - CHN_UNLOCK(c); err = vchan_destroy(c); if (err == 0) cnt--; diff --git a/sys/dev/sound/pcm/sound.h b/sys/dev/sound/pcm/sound.h index af539a66bf0b..2bf490c3675f 100644 --- a/sys/dev/sound/pcm/sound.h +++ b/sys/dev/sound/pcm/sound.h @@ -123,11 +123,15 @@ nomenclature: [etc.] */ -#define PCMMINOR(x) (minor(x)) -#define PCMCHAN(x) ((PCMMINOR(x) & 0x00ff0000) >> 16) -#define PCMUNIT(x) ((PCMMINOR(x) & 0x000000f0) >> 4) -#define PCMDEV(x) (PCMMINOR(x) & 0x0000000f) -#define PCMMKMINOR(u, d, c) ((((c) & 0xff) << 16) | (((u) & 0x0f) << 4) | ((d) & 0x0f)) +#define PCMMAXCHAN 0xff +#define PCMMAXDEV 0x0f +#define PCMMAXUNIT 0x0f +#define PCMMINOR(x) (minor(x)) +#define PCMCHAN(x) ((PCMMINOR(x) >> 16) & PCMMAXCHAN) +#define PCMUNIT(x) ((PCMMINOR(x) >> 4) & PCMMAXUNIT) +#define PCMDEV(x) (PCMMINOR(x) & PCMMAXDEV) +#define PCMMKMINOR(u, d, c) ((((c) & PCMMAXCHAN) << 16) | \ + (((u) & PCMMAXUNIT) << 4) | ((d) & PCMMAXDEV)) #define SD_F_SIMPLEX 0x00000001 #define SD_F_AUTOVCHAN 0x00000002 @@ -156,7 +160,8 @@ nomenclature: struct pcm_channel *fkchan_setup(device_t dev); int fkchan_kill(struct pcm_channel *c); -#define SND_MAXVCHANS 255 +/* XXX Flawed definition. I'll fix it someday. */ +#define SND_MAXVCHANS PCMMAXCHAN /* * Minor numbers for the sound driver. diff --git a/sys/dev/sound/pcm/vchan.c b/sys/dev/sound/pcm/vchan.c index 308addcdec4f..0627be606eb2 100644 --- a/sys/dev/sound/pcm/vchan.c +++ b/sys/dev/sound/pcm/vchan.c @@ -505,8 +505,8 @@ vchan_create(struct pcm_channel *parent) parent->flags &= ~CHN_F_HAS_VCHAN; CHN_UNLOCK(parent); free(pce, M_DEVBUF); - pcm_chn_remove(d, child); - pcm_chn_destroy(child); + if (pcm_chn_remove(d, child) == 0) + pcm_chn_destroy(child); CHN_LOCK(parent); return err; } @@ -544,14 +544,23 @@ vchan_destroy(struct pcm_channel *c) gotch: SLIST_FOREACH(sce, &d->channels, link) { if (sce->channel == c) { - if (sce->dsp_devt) + if (sce->dsp_devt) { destroy_dev(sce->dsp_devt); - if (sce->dspW_devt) + sce->dsp_devt = NULL; + } + if (sce->dspW_devt) { destroy_dev(sce->dspW_devt); - if (sce->audio_devt) + sce->dspW_devt = NULL; + } + if (sce->audio_devt) { destroy_dev(sce->audio_devt); - if (sce->dspr_devt) + sce->audio_devt = NULL; + } + if (sce->dspr_devt) { destroy_dev(sce->dspr_devt); + sce->dspr_devt = NULL; + } + d->devcount--; break; } } -- cgit v1.2.3