| Message ID | 1328177646-10163-2-git-send-email-apw@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 02.02.2012 11:14, Andy Whitcroft wrote: > From: Dan Rosenberg <drosenberg@vsecurity.com> > > Was: [PATCH] sound/oss/midi_synth: prevent underflow, use of > uninitialized value, and signedness issue > > The offset passed to midi_synth_load_patch() can be essentially > arbitrary. If it's greater than the header length, this will result in > a copy_from_user(dst, src, negative_val). While this will just return > -EFAULT on x86, on other architectures this may cause memory corruption. > Additionally, the length field of the sysex_info structure may not be > initialized prior to its use. Finally, a signed comparison may result > in an unintentionally large loop. > > On suggestion by Takashi Iwai, version two removes the offset argument > from the load_patch callbacks entirely, which also resolves similar > issues in opl3. Compile tested only. > > v3 adjusts comments and hopefully gets copy offsets right. > > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > (cherry picked from commit b769f49463711205d57286e64cf535ed4daf59e9) > CVE-2011-1476 > BugLink: http://bugs.launchpad.net/bugs/925337 > Signed-off-by: Andy Whitcroft <apw@canonical.com> > --- > sound/oss/dev_table.h | 2 +- > sound/oss/midi_synth.c | 30 +++++++++++++----------------- > sound/oss/midi_synth.h | 2 +- > sound/oss/opl3.c | 8 ++------ > sound/oss/sequencer.c | 2 +- > 5 files changed, 18 insertions(+), 26 deletions(-) > > diff --git a/sound/oss/dev_table.h b/sound/oss/dev_table.h > index b7617be..0199a31 100644 > --- a/sound/oss/dev_table.h > +++ b/sound/oss/dev_table.h > @@ -271,7 +271,7 @@ struct synth_operations > void (*reset) (int dev); > void (*hw_control) (int dev, unsigned char *event); > int (*load_patch) (int dev, int format, const char __user *addr, > - int offs, int count, int pmgr_flag); > + int count, int pmgr_flag); > void (*aftertouch) (int dev, int voice, int pressure); > void (*controller) (int dev, int voice, int ctrl_num, int value); > void (*panning) (int dev, int voice, int value); > diff --git a/sound/oss/midi_synth.c b/sound/oss/midi_synth.c > index 9e45098..f83f072 100644 > --- a/sound/oss/midi_synth.c > +++ b/sound/oss/midi_synth.c > @@ -476,7 +476,7 @@ EXPORT_SYMBOL(midi_synth_hw_control); > > int > midi_synth_load_patch(int dev, int format, const char __user *addr, > - int offs, int count, int pmgr_flag) > + int count, int pmgr_flag) > { > int orig_dev = synth_devs[dev]->midi_dev; > > @@ -491,33 +491,29 @@ midi_synth_load_patch(int dev, int format, const char __user *addr, > if (!prefix_cmd(orig_dev, 0xf0)) > return 0; > > + /* Invalid patch format */ > if (format != SYSEX_PATCH) > - { > -/* printk("MIDI Error: Invalid patch format (key) 0x%x\n", format);*/ > return -EINVAL; > - } > + > + /* Patch header too short */ > if (count < hdr_size) > - { > -/* printk("MIDI Error: Patch header too short\n");*/ > return -EINVAL; > - } > + > count -= hdr_size; > > /* > - * Copy the header from user space but ignore the first bytes which have > - * been transferred already. > + * Copy the header from user space > */ > > - if(copy_from_user(&((char *) &sysex)[offs], &(addr)[offs], hdr_size - offs)) > + if (copy_from_user(&sysex, addr, hdr_size)) > return -EFAULT; > - > - if (count < sysex.len) > - { > -/* printk(KERN_WARNING "MIDI Warning: Sysex record too short (%d<%d)\n", count, (int) sysex.len);*/ > + > + /* Sysex record too short */ > + if ((unsigned)count < (unsigned)sysex.len) > sysex.len = count; > - } > - left = sysex.len; > - src_offs = 0; > + > + left = sysex.len; > + src_offs = 0; > > for (i = 0; i < left && !signal_pending(current); i++) > { > diff --git a/sound/oss/midi_synth.h b/sound/oss/midi_synth.h > index 6bc9d00..b64ddd6 100644 > --- a/sound/oss/midi_synth.h > +++ b/sound/oss/midi_synth.h > @@ -8,7 +8,7 @@ int midi_synth_open (int dev, int mode); > void midi_synth_close (int dev); > void midi_synth_hw_control (int dev, unsigned char *event); > int midi_synth_load_patch (int dev, int format, const char __user * addr, > - int offs, int count, int pmgr_flag); > + int count, int pmgr_flag); > void midi_synth_panning (int dev, int channel, int pressure); > void midi_synth_aftertouch (int dev, int channel, int pressure); > void midi_synth_controller (int dev, int channel, int ctrl_num, int value); > diff --git a/sound/oss/opl3.c b/sound/oss/opl3.c > index 7781c13..6b5f54b 100644 > --- a/sound/oss/opl3.c > +++ b/sound/oss/opl3.c > @@ -819,7 +819,7 @@ static void opl3_hw_control(int dev, unsigned char *event) > } > > static int opl3_load_patch(int dev, int format, const char __user *addr, > - int offs, int count, int pmgr_flag) > + int count, int pmgr_flag) > { > struct sbi_instrument ins; > > @@ -829,11 +829,7 @@ static int opl3_load_patch(int dev, int format, const char __user *addr, > return -EINVAL; > } > > - /* > - * What the fuck is going on here? We leave junk in the beginning > - * of ins and then check the field pretty close to that beginning? > - */ > - if(copy_from_user(&((char *) &ins)[offs], addr + offs, sizeof(ins) - offs)) > + if (copy_from_user(&ins, addr, sizeof(ins))) > return -EFAULT; > > if (ins.channel < 0 || ins.channel >= SBFM_MAXINSTR) > diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c > index 5c215f7..7027b8d 100644 > --- a/sound/oss/sequencer.c > +++ b/sound/oss/sequencer.c > @@ -242,7 +242,7 @@ int sequencer_write(int dev, struct file *file, const char __user *buf, int coun > return -ENXIO; > > fmt = (*(short *) &event_rec[0]) & 0xffff; > - err = synth_devs[dev]->load_patch(dev, fmt, buf, p + 4, c, 0); > + err = synth_devs[dev]->load_patch(dev, fmt, buf + p, c, 0); > if (err < 0) > return err; > Roughly looks sane. Acked-by: Stefan Bader <stefan.bader@canonical.com>
On Thu, Feb 02, 2012 at 10:14:06AM +0000, Andy Whitcroft wrote: > From: Dan Rosenberg <drosenberg@vsecurity.com> > > Was: [PATCH] sound/oss/midi_synth: prevent underflow, use of > uninitialized value, and signedness issue > > The offset passed to midi_synth_load_patch() can be essentially > arbitrary. If it's greater than the header length, this will result in > a copy_from_user(dst, src, negative_val). While this will just return > -EFAULT on x86, on other architectures this may cause memory corruption. > Additionally, the length field of the sysex_info structure may not be > initialized prior to its use. Finally, a signed comparison may result > in an unintentionally large loop. > > On suggestion by Takashi Iwai, version two removes the offset argument > from the load_patch callbacks entirely, which also resolves similar > issues in opl3. Compile tested only. > > v3 adjusts comments and hopefully gets copy offsets right. > > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > (cherry picked from commit b769f49463711205d57286e64cf535ed4daf59e9) > CVE-2011-1476 > BugLink: http://bugs.launchpad.net/bugs/925337 > Signed-off-by: Andy Whitcroft <apw@canonical.com> > --- > sound/oss/dev_table.h | 2 +- > sound/oss/midi_synth.c | 30 +++++++++++++----------------- > sound/oss/midi_synth.h | 2 +- > sound/oss/opl3.c | 8 ++------ > sound/oss/sequencer.c | 2 +- > 5 files changed, 18 insertions(+), 26 deletions(-) > > diff --git a/sound/oss/dev_table.h b/sound/oss/dev_table.h > index b7617be..0199a31 100644 > --- a/sound/oss/dev_table.h > +++ b/sound/oss/dev_table.h > @@ -271,7 +271,7 @@ struct synth_operations > void (*reset) (int dev); > void (*hw_control) (int dev, unsigned char *event); > int (*load_patch) (int dev, int format, const char __user *addr, > - int offs, int count, int pmgr_flag); > + int count, int pmgr_flag); > void (*aftertouch) (int dev, int voice, int pressure); > void (*controller) (int dev, int voice, int ctrl_num, int value); > void (*panning) (int dev, int voice, int value); > diff --git a/sound/oss/midi_synth.c b/sound/oss/midi_synth.c > index 9e45098..f83f072 100644 > --- a/sound/oss/midi_synth.c > +++ b/sound/oss/midi_synth.c > @@ -476,7 +476,7 @@ EXPORT_SYMBOL(midi_synth_hw_control); > > int > midi_synth_load_patch(int dev, int format, const char __user *addr, > - int offs, int count, int pmgr_flag) > + int count, int pmgr_flag) > { > int orig_dev = synth_devs[dev]->midi_dev; > > @@ -491,33 +491,29 @@ midi_synth_load_patch(int dev, int format, const char __user *addr, > if (!prefix_cmd(orig_dev, 0xf0)) > return 0; > > + /* Invalid patch format */ > if (format != SYSEX_PATCH) > - { > -/* printk("MIDI Error: Invalid patch format (key) 0x%x\n", format);*/ > return -EINVAL; > - } > + > + /* Patch header too short */ > if (count < hdr_size) > - { > -/* printk("MIDI Error: Patch header too short\n");*/ > return -EINVAL; > - } > + > count -= hdr_size; > > /* > - * Copy the header from user space but ignore the first bytes which have > - * been transferred already. > + * Copy the header from user space > */ > > - if(copy_from_user(&((char *) &sysex)[offs], &(addr)[offs], hdr_size - offs)) > + if (copy_from_user(&sysex, addr, hdr_size)) > return -EFAULT; > - > - if (count < sysex.len) > - { > -/* printk(KERN_WARNING "MIDI Warning: Sysex record too short (%d<%d)\n", count, (int) sysex.len);*/ > + > + /* Sysex record too short */ > + if ((unsigned)count < (unsigned)sysex.len) > sysex.len = count; > - } > - left = sysex.len; > - src_offs = 0; > + > + left = sysex.len; > + src_offs = 0; > > for (i = 0; i < left && !signal_pending(current); i++) > { > diff --git a/sound/oss/midi_synth.h b/sound/oss/midi_synth.h > index 6bc9d00..b64ddd6 100644 > --- a/sound/oss/midi_synth.h > +++ b/sound/oss/midi_synth.h > @@ -8,7 +8,7 @@ int midi_synth_open (int dev, int mode); > void midi_synth_close (int dev); > void midi_synth_hw_control (int dev, unsigned char *event); > int midi_synth_load_patch (int dev, int format, const char __user * addr, > - int offs, int count, int pmgr_flag); > + int count, int pmgr_flag); > void midi_synth_panning (int dev, int channel, int pressure); > void midi_synth_aftertouch (int dev, int channel, int pressure); > void midi_synth_controller (int dev, int channel, int ctrl_num, int value); > diff --git a/sound/oss/opl3.c b/sound/oss/opl3.c > index 7781c13..6b5f54b 100644 > --- a/sound/oss/opl3.c > +++ b/sound/oss/opl3.c > @@ -819,7 +819,7 @@ static void opl3_hw_control(int dev, unsigned char *event) > } > > static int opl3_load_patch(int dev, int format, const char __user *addr, > - int offs, int count, int pmgr_flag) > + int count, int pmgr_flag) > { > struct sbi_instrument ins; > > @@ -829,11 +829,7 @@ static int opl3_load_patch(int dev, int format, const char __user *addr, > return -EINVAL; > } > > - /* > - * What the fuck is going on here? We leave junk in the beginning > - * of ins and then check the field pretty close to that beginning? > - */ > - if(copy_from_user(&((char *) &ins)[offs], addr + offs, sizeof(ins) - offs)) > + if (copy_from_user(&ins, addr, sizeof(ins))) > return -EFAULT; > > if (ins.channel < 0 || ins.channel >= SBFM_MAXINSTR) > diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c > index 5c215f7..7027b8d 100644 > --- a/sound/oss/sequencer.c > +++ b/sound/oss/sequencer.c > @@ -242,7 +242,7 @@ int sequencer_write(int dev, struct file *file, const char __user *buf, int coun > return -ENXIO; > > fmt = (*(short *) &event_rec[0]) & 0xffff; > - err = synth_devs[dev]->load_patch(dev, fmt, buf, p + 4, c, 0); > + err = synth_devs[dev]->load_patch(dev, fmt, buf + p, c, 0); > if (err < 0) > return err; > > -- > 1.7.8.3 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team >
diff --git a/sound/oss/dev_table.h b/sound/oss/dev_table.h index b7617be..0199a31 100644 --- a/sound/oss/dev_table.h +++ b/sound/oss/dev_table.h @@ -271,7 +271,7 @@ struct synth_operations void (*reset) (int dev); void (*hw_control) (int dev, unsigned char *event); int (*load_patch) (int dev, int format, const char __user *addr, - int offs, int count, int pmgr_flag); + int count, int pmgr_flag); void (*aftertouch) (int dev, int voice, int pressure); void (*controller) (int dev, int voice, int ctrl_num, int value); void (*panning) (int dev, int voice, int value); diff --git a/sound/oss/midi_synth.c b/sound/oss/midi_synth.c index 9e45098..f83f072 100644 --- a/sound/oss/midi_synth.c +++ b/sound/oss/midi_synth.c @@ -476,7 +476,7 @@ EXPORT_SYMBOL(midi_synth_hw_control); int midi_synth_load_patch(int dev, int format, const char __user *addr, - int offs, int count, int pmgr_flag) + int count, int pmgr_flag) { int orig_dev = synth_devs[dev]->midi_dev; @@ -491,33 +491,29 @@ midi_synth_load_patch(int dev, int format, const char __user *addr, if (!prefix_cmd(orig_dev, 0xf0)) return 0; + /* Invalid patch format */ if (format != SYSEX_PATCH) - { -/* printk("MIDI Error: Invalid patch format (key) 0x%x\n", format);*/ return -EINVAL; - } + + /* Patch header too short */ if (count < hdr_size) - { -/* printk("MIDI Error: Patch header too short\n");*/ return -EINVAL; - } + count -= hdr_size; /* - * Copy the header from user space but ignore the first bytes which have - * been transferred already. + * Copy the header from user space */ - if(copy_from_user(&((char *) &sysex)[offs], &(addr)[offs], hdr_size - offs)) + if (copy_from_user(&sysex, addr, hdr_size)) return -EFAULT; - - if (count < sysex.len) - { -/* printk(KERN_WARNING "MIDI Warning: Sysex record too short (%d<%d)\n", count, (int) sysex.len);*/ + + /* Sysex record too short */ + if ((unsigned)count < (unsigned)sysex.len) sysex.len = count; - } - left = sysex.len; - src_offs = 0; + + left = sysex.len; + src_offs = 0; for (i = 0; i < left && !signal_pending(current); i++) { diff --git a/sound/oss/midi_synth.h b/sound/oss/midi_synth.h index 6bc9d00..b64ddd6 100644 --- a/sound/oss/midi_synth.h +++ b/sound/oss/midi_synth.h @@ -8,7 +8,7 @@ int midi_synth_open (int dev, int mode); void midi_synth_close (int dev); void midi_synth_hw_control (int dev, unsigned char *event); int midi_synth_load_patch (int dev, int format, const char __user * addr, - int offs, int count, int pmgr_flag); + int count, int pmgr_flag); void midi_synth_panning (int dev, int channel, int pressure); void midi_synth_aftertouch (int dev, int channel, int pressure); void midi_synth_controller (int dev, int channel, int ctrl_num, int value); diff --git a/sound/oss/opl3.c b/sound/oss/opl3.c index 7781c13..6b5f54b 100644 --- a/sound/oss/opl3.c +++ b/sound/oss/opl3.c @@ -819,7 +819,7 @@ static void opl3_hw_control(int dev, unsigned char *event) } static int opl3_load_patch(int dev, int format, const char __user *addr, - int offs, int count, int pmgr_flag) + int count, int pmgr_flag) { struct sbi_instrument ins; @@ -829,11 +829,7 @@ static int opl3_load_patch(int dev, int format, const char __user *addr, return -EINVAL; } - /* - * What the fuck is going on here? We leave junk in the beginning - * of ins and then check the field pretty close to that beginning? - */ - if(copy_from_user(&((char *) &ins)[offs], addr + offs, sizeof(ins) - offs)) + if (copy_from_user(&ins, addr, sizeof(ins))) return -EFAULT; if (ins.channel < 0 || ins.channel >= SBFM_MAXINSTR) diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c index 5c215f7..7027b8d 100644 --- a/sound/oss/sequencer.c +++ b/sound/oss/sequencer.c @@ -242,7 +242,7 @@ int sequencer_write(int dev, struct file *file, const char __user *buf, int coun return -ENXIO; fmt = (*(short *) &event_rec[0]) & 0xffff; - err = synth_devs[dev]->load_patch(dev, fmt, buf, p + 4, c, 0); + err = synth_devs[dev]->load_patch(dev, fmt, buf + p, c, 0); if (err < 0) return err;