On Thu, Sep 12, 2024 at 02:29:58PM +0300, Dan Carpenter wrote: > On Thu, Sep 12, 2024 at 12:05:31PM +0200, Takashi Iwai wrote: > > On Thu, 12 Sep 2024 10:51:14 +0200, > > Dan Carpenter wrote: > > > > > > I believe the this bug affects 64bit systems as well, but analyzing this > > > code is easier if we assume that we're on a 32bit system. The problem is > > > in snd_ctl_elem_add() where we do: > > > > > > sound/core/control.c > > > 1669 private_size = value_sizes[info->type] * info->count; > > > 1670 alloc_size = compute_user_elem_size(private_size, count); > > > ^^^^^ > > > count is info->owner. It's a non-zero u32 that comes from the user via > > > snd_ctl_elem_add_user(). So the math in compute_user_elem_size() could > > > have an integer overflow resulting in a smaller than expected size. > > > > So this should also use the overflow macro, too, in addition to your > > changes? Something like: > > > > --- a/sound/core/control.c > > +++ b/sound/core/control.c > > @@ -1618,7 +1618,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, > > struct snd_kcontrol *kctl; > > unsigned int count; > > unsigned int access; > > - long private_size; > > + size_t private_size; > > size_t alloc_size; > > struct user_element *ue; > > unsigned int offset; > > @@ -1666,7 +1666,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, > > /* user-space control doesn't allow zero-size data */ > > if (info->count < 1) > > return -EINVAL; > > - private_size = value_sizes[info->type] * info->count; > > + private_size = array_size(value_sizes[info->type], info->count); > > alloc_size = compute_user_elem_size(private_size, count); > > > > guard(rwsem_write)(&card->controls_rwsem); > > > > I've reviewed this some more and those changes are harmless but unnecessary. > info->count is checked in snd_ctl_check_elem_info(). > I also considered if I should fix this bug by adding checks to snd_ctl_check_elem_info() but I don't think that's the right approach. I couldn't see how it would work at least. regards, dan carpenter
