Commit b769f49463711205d57286e64cf535ed4daf59e9
Committed by
Takashi Iwai
Takashi Iwai
1 parent
ce24f58a11
Exists in
master
and in
39 other branches
sound/oss: remove offset from load_patch callbacks
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>
Showing 5 changed files with 18 additions and 26 deletions Side-by-side Diff
sound/oss/dev_table.h
| ... | ... | @@ -271,7 +271,7 @@ |
| 271 | 271 | void (*reset) (int dev); |
| 272 | 272 | void (*hw_control) (int dev, unsigned char *event); |
| 273 | 273 | int (*load_patch) (int dev, int format, const char __user *addr, |
| 274 | - int offs, int count, int pmgr_flag); | |
| 274 | + int count, int pmgr_flag); | |
| 275 | 275 | void (*aftertouch) (int dev, int voice, int pressure); |
| 276 | 276 | void (*controller) (int dev, int voice, int ctrl_num, int value); |
| 277 | 277 | void (*panning) (int dev, int voice, int value); |
sound/oss/midi_synth.c
| ... | ... | @@ -476,7 +476,7 @@ |
| 476 | 476 | |
| 477 | 477 | int |
| 478 | 478 | midi_synth_load_patch(int dev, int format, const char __user *addr, |
| 479 | - int offs, int count, int pmgr_flag) | |
| 479 | + int count, int pmgr_flag) | |
| 480 | 480 | { |
| 481 | 481 | int orig_dev = synth_devs[dev]->midi_dev; |
| 482 | 482 | |
| 483 | 483 | |
| 484 | 484 | |
| 485 | 485 | |
| 486 | 486 | |
| 487 | 487 | |
| 488 | 488 | |
| 489 | 489 | |
| 490 | 490 | |
| ... | ... | @@ -491,33 +491,29 @@ |
| 491 | 491 | if (!prefix_cmd(orig_dev, 0xf0)) |
| 492 | 492 | return 0; |
| 493 | 493 | |
| 494 | + /* Invalid patch format */ | |
| 494 | 495 | if (format != SYSEX_PATCH) |
| 495 | - { | |
| 496 | -/* printk("MIDI Error: Invalid patch format (key) 0x%x\n", format);*/ | |
| 497 | 496 | return -EINVAL; |
| 498 | - } | |
| 497 | + | |
| 498 | + /* Patch header too short */ | |
| 499 | 499 | if (count < hdr_size) |
| 500 | - { | |
| 501 | -/* printk("MIDI Error: Patch header too short\n");*/ | |
| 502 | 500 | return -EINVAL; |
| 503 | - } | |
| 501 | + | |
| 504 | 502 | count -= hdr_size; |
| 505 | 503 | |
| 506 | 504 | /* |
| 507 | - * Copy the header from user space but ignore the first bytes which have | |
| 508 | - * been transferred already. | |
| 505 | + * Copy the header from user space | |
| 509 | 506 | */ |
| 510 | 507 | |
| 511 | - if(copy_from_user(&((char *) &sysex)[offs], &(addr)[offs], hdr_size - offs)) | |
| 508 | + if (copy_from_user(&sysex, addr, hdr_size)) | |
| 512 | 509 | return -EFAULT; |
| 513 | - | |
| 514 | - if (count < sysex.len) | |
| 515 | - { | |
| 516 | -/* printk(KERN_WARNING "MIDI Warning: Sysex record too short (%d<%d)\n", count, (int) sysex.len);*/ | |
| 510 | + | |
| 511 | + /* Sysex record too short */ | |
| 512 | + if ((unsigned)count < (unsigned)sysex.len) | |
| 517 | 513 | sysex.len = count; |
| 518 | - } | |
| 519 | - left = sysex.len; | |
| 520 | - src_offs = 0; | |
| 514 | + | |
| 515 | + left = sysex.len; | |
| 516 | + src_offs = 0; | |
| 521 | 517 | |
| 522 | 518 | for (i = 0; i < left && !signal_pending(current); i++) |
| 523 | 519 | { |
sound/oss/midi_synth.h
| ... | ... | @@ -8,7 +8,7 @@ |
| 8 | 8 | void midi_synth_close (int dev); |
| 9 | 9 | void midi_synth_hw_control (int dev, unsigned char *event); |
| 10 | 10 | int midi_synth_load_patch (int dev, int format, const char __user * addr, |
| 11 | - int offs, int count, int pmgr_flag); | |
| 11 | + int count, int pmgr_flag); | |
| 12 | 12 | void midi_synth_panning (int dev, int channel, int pressure); |
| 13 | 13 | void midi_synth_aftertouch (int dev, int channel, int pressure); |
| 14 | 14 | void midi_synth_controller (int dev, int channel, int ctrl_num, int value); |
sound/oss/opl3.c
| ... | ... | @@ -820,7 +820,7 @@ |
| 820 | 820 | } |
| 821 | 821 | |
| 822 | 822 | static int opl3_load_patch(int dev, int format, const char __user *addr, |
| 823 | - int offs, int count, int pmgr_flag) | |
| 823 | + int count, int pmgr_flag) | |
| 824 | 824 | { |
| 825 | 825 | struct sbi_instrument ins; |
| 826 | 826 | |
| ... | ... | @@ -830,11 +830,7 @@ |
| 830 | 830 | return -EINVAL; |
| 831 | 831 | } |
| 832 | 832 | |
| 833 | - /* | |
| 834 | - * What the fuck is going on here? We leave junk in the beginning | |
| 835 | - * of ins and then check the field pretty close to that beginning? | |
| 836 | - */ | |
| 837 | - if(copy_from_user(&((char *) &ins)[offs], addr + offs, sizeof(ins) - offs)) | |
| 833 | + if (copy_from_user(&ins, addr, sizeof(ins))) | |
| 838 | 834 | return -EFAULT; |
| 839 | 835 | |
| 840 | 836 | if (ins.channel < 0 || ins.channel >= SBFM_MAXINSTR) |
sound/oss/sequencer.c