| Message ID | 0f03d569-9804-4617-a806-f0e5c32399fb@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: control: prevent some integer overflow issues | expand |
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 >
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
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
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
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
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
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)
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(-)