On Thu, Sep 12, 2024 at 04:03:58PM +0200, Takashi Iwai wrote:
> On Thu, 12 Sep 2024 14:44:30 +0200,
> Dan Carpenter wrote:
> >
> > 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.
>
> OK, so it doesn't seem affected in the end.
> The input values have been checked, and they can't overflow.
>
Ugh... I need to send a v2.
The bug is real on 32bit systems, but reviewing it more, I don't think it affects
64bit systems. And I made a mistake. We do need to change the types in
check_user_elem_overflow() but the negative values were intentional in
replace_user_tlv().
if (check_user_elem_overflow(ue->card, (ssize_t)(size - ue->tlv_data_size)))
The size variable is the new size and the ue->tlv_data_size is the previous
size. So making the buffer smaller is fine but going over the user limit is
not. So I need to re-write this as:
if (size > ue->tlv_data_size &&
check_user_elem_overflow(ue->card, size - ue->tlv_data_size))
return -ENOMEM;
regards,
dan carpenter
