[PATCH] ALSA: control: prevent some integer overflow issues

[PATCH] ALSA: control: prevent some integer overflow issues

[Dan Carpenter]

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.

  1671
  1672          guard(rwsem_write)(&card->controls_rwsem);
  1673          if (check_user_elem_overflow(card, alloc_size))

The math is check_user_elem_overflow() can also overflow.  Additionally,
large positive values are cast to negative and thus do not exceed
max_user_ctl_alloc_size so they are treated as valid.  It should be the
opposite, where negative sizes are invalid.

  1674                  return -ENOMEM;

Fixes: 2225e79b9b03 ("ALSA: core: reduce stack usage related to snd_ctl_new()")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 sound/core/control.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 4f55f64c42e1..f36af27e68d5 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1397,9 +1397,9 @@ struct user_element {
 };
 
 // check whether the addition (in bytes) of user ctl element may overflow the limit.
-static bool check_user_elem_overflow(struct snd_card *card, ssize_t add)
+static bool check_user_elem_overflow(struct snd_card *card, size_t add)
 {
-	return (ssize_t)card->user_ctl_alloc_size + add > max_user_ctl_alloc_size;
+	return size_add(card->user_ctl_alloc_size, add) > max_user_ctl_alloc_size;
 }
 
 static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
@@ -1593,7 +1593,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 
 static size_t compute_user_elem_size(size_t size, unsigned int count)
 {
-	return sizeof(struct user_element) + size * count;
+	return size_add(sizeof(struct user_element), size_mul(size, count));
 }
 
 static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
-- 
2.45.2

Re: [PATCH] ALSA: control: prevent some integer overflow issues

[Takashi Iwai]

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);


thanks,

Takashi

> 
>   1671
>   1672          guard(rwsem_write)(&card->controls_rwsem);
>   1673          if (check_user_elem_overflow(card, alloc_size))
> 
> The math is check_user_elem_overflow() can also overflow.  Additionally,
> large positive values are cast to negative and thus do not exceed
> max_user_ctl_alloc_size so they are treated as valid.  It should be the
> opposite, where negative sizes are invalid.
> 
>   1674                  return -ENOMEM;
> 
> Fixes: 2225e79b9b03 ("ALSA: core: reduce stack usage related to snd_ctl_new()")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  sound/core/control.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 4f55f64c42e1..f36af27e68d5 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1397,9 +1397,9 @@ struct user_element {
>  };
>  
>  // check whether the addition (in bytes) of user ctl element may overflow the limit.
> -static bool check_user_elem_overflow(struct snd_card *card, ssize_t add)
> +static bool check_user_elem_overflow(struct snd_card *card, size_t add)
>  {
> -	return (ssize_t)card->user_ctl_alloc_size + add > max_user_ctl_alloc_size;
> +	return size_add(card->user_ctl_alloc_size, add) > max_user_ctl_alloc_size;
>  }
>  
>  static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
> @@ -1593,7 +1593,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
>  
>  static size_t compute_user_elem_size(size_t size, unsigned int count)
>  {
> -	return sizeof(struct user_element) + size * count;
> +	return size_add(sizeof(struct user_element), size_mul(size, count));
>  }
>  
>  static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
> -- 
> 2.45.2
> 

Re: [PATCH] ALSA: control: prevent some integer overflow issues

[Dan Carpenter]

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().


regards,
dan carpenter

Re: [PATCH] ALSA: control: prevent some integer overflow issues

[Dan Carpenter]

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

Re: [PATCH] ALSA: control: prevent some integer overflow issues

[Takashi Iwai]

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

Re: [PATCH] ALSA: control: prevent some integer overflow issues

[Dan Carpenter]

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

Re: [PATCH] ALSA: control: prevent some integer overflow issues

[Dan Carpenter]

Actually there is a check in snd_ctl_new() which means that although these
integer overflows do happen, we eventually return -ENOMEM and the whole
thing is harmless.

	if (count == 0 || count > MAX_CONTROL_COUNT)

regards,
dan carpenter