sb_mixer.c/save_mixer() awfully buggy?

sb_mixer.c/save_mixer() awfully buggy?

[Andreas Mohr]

Hi all,

with my ALS4000, I'm hitting this upon resume:

ALSA sound/isa/sb/sb_mixer.c:936: BUG? (num_regs > (sizeof(chip->saved_regs) / sizeof(
(chip->saved_regs)[0]) + (sizeof(char[1 - 2 * !!(__builtin_types_compatible_p(typeof(c
hip->saved_regs), typeof(&chip->saved_regs[0])))]) - 1)))
Pid: 4498, comm: my_sleep Not tainted 2.6.26-rc3 #1
 [<e097ac8c>] restore_mixer+0x6c/0x70 [snd_sb_common]
 [<e097ad07>] snd_sbmixer_resume+0x77/0x80 [snd_sb_common]
 [<e098030a>] snd_als4000_resume+0x5a/0xd0 [snd_als4000]
 [<c0281a72>] pci_device_resume+0x22/0x60
 [<c0303e76>] device_resume+0x86/0x100
 [<c0145bb7>] hibernation_snapshot+0xc7/0x180
 [<c0144f9f>] ? freeze_processes+0x3f/0x80
 [<c0145d4c>] hibernate+0xdc/0x180
 [<c0144980>] ? state_store+0x0/0xd0
 [<c0144a3f>] state_store+0xbf/0xd0
 [<c0144980>] ? state_store+0x0/0xd0
 [<c02743d4>] kobj_attr_store+0x24/0x30
 [<c01aaf5b>] sysfs_write_file+0xbb/0x110
 [<c016ee56>] vfs_write+0x96/0x130
 [<c01aaea0>] ? sysfs_write_file+0x0/0x110
 [<c016f3cd>] sys_write+0x3d/0x70
 [<c0103026>] syscall_call+0x7/0xb
 =======================

triggered by this line:

static void restore_mixer(struct snd_sb *chip, unsigned char *regs, int num_regs)
{
	unsigned char *val = chip->saved_regs;
	snd_assert(num_regs > ARRAY_SIZE(chip->saved_regs), return);
	for (; num_regs; num_regs--)
		snd_sbmixer_write(chip, *regs++, *val++);
}

Uhmm, forgive my utterly dumb state of mind, but isn't this assert
_exactly_ wrong?
We assert that the number of regs to copy is _higher_ than the array
size of the content we want to copy, IOW we want to make _certain_
that we'll hit a nice array overflow or segmentation violation later.
Doesn't make too much - well, exactly zero, zilch - sense to me.

Dito save_mixer().

Forgive me if this is already fixed in ALSA head (still exists in current
2.6.26-rc7).
Sounds like 2.6.26(!) material to me if it is indeed buggy.

Thanks,

Andreas Mohr

Re: sb_mixer.c/save_mixer() awfully buggy?

[Takashi Iwai]

At Mon, 23 Jun 2008 00:46:35 +0200,
Andreas Mohr wrote:
> 
> Hi all,
> 
> with my ALS4000, I'm hitting this upon resume:
> 
> ALSA sound/isa/sb/sb_mixer.c:936: BUG? (num_regs > (sizeof(chip->saved_regs) / sizeof(
> (chip->saved_regs)[0]) + (sizeof(char[1 - 2 * !!(__builtin_types_compatible_p(typeof(c
> hip->saved_regs), typeof(&chip->saved_regs[0])))]) - 1)))
> Pid: 4498, comm: my_sleep Not tainted 2.6.26-rc3 #1
>  [<e097ac8c>] restore_mixer+0x6c/0x70 [snd_sb_common]
>  [<e097ad07>] snd_sbmixer_resume+0x77/0x80 [snd_sb_common]
>  [<e098030a>] snd_als4000_resume+0x5a/0xd0 [snd_als4000]
>  [<c0281a72>] pci_device_resume+0x22/0x60
>  [<c0303e76>] device_resume+0x86/0x100
>  [<c0145bb7>] hibernation_snapshot+0xc7/0x180
>  [<c0144f9f>] ? freeze_processes+0x3f/0x80
>  [<c0145d4c>] hibernate+0xdc/0x180
>  [<c0144980>] ? state_store+0x0/0xd0
>  [<c0144a3f>] state_store+0xbf/0xd0
>  [<c0144980>] ? state_store+0x0/0xd0
>  [<c02743d4>] kobj_attr_store+0x24/0x30
>  [<c01aaf5b>] sysfs_write_file+0xbb/0x110
>  [<c016ee56>] vfs_write+0x96/0x130
>  [<c01aaea0>] ? sysfs_write_file+0x0/0x110
>  [<c016f3cd>] sys_write+0x3d/0x70
>  [<c0103026>] syscall_call+0x7/0xb
>  =======================
> 
> triggered by this line:
> 
> static void restore_mixer(struct snd_sb *chip, unsigned char *regs, int num_regs)
> {
> 	unsigned char *val = chip->saved_regs;
> 	snd_assert(num_regs > ARRAY_SIZE(chip->saved_regs), return);
> 	for (; num_regs; num_regs--)
> 		snd_sbmixer_write(chip, *regs++, *val++);
> }
> 
> Uhmm, forgive my utterly dumb state of mind, but isn't this assert
> _exactly_ wrong?
> We assert that the number of regs to copy is _higher_ than the array
> size of the content we want to copy, IOW we want to make _certain_
> that we'll hit a nice array overflow or segmentation violation later.
> Doesn't make too much - well, exactly zero, zilch - sense to me.
> 
> Dito save_mixer().

Indeed, it's a silly bug.  It just be num_regs <= ARRAY_SIZE().
The codepath wasn't tested with the debug environment.  I fixed it on
my tree now.

> 
> Forgive me if this is already fixed in ALSA head (still exists in current
> 2.6.26-rc7).
> Sounds like 2.6.26(!) material to me if it is indeed buggy.

Yeah, will do a pull request.


thanks,

Takashi