[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87r09pynn5.wl-tiwai@suse.de>
Date: Thu, 12 Sep 2024 16:03:58 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Takashi Iwai <tiwai@...e.de>,
Takashi Sakamoto <o-takashi@...amocchi.jp>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Mark Brown <broonie@...nel.org>,
linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org,
kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ALSA: control: prevent some integer overflow issues
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.
thanks,
Takashi
Powered by blists - more mailing lists