This patch doesn't change runtime at all, it's just for kernel hardening.

The "count" here comes from the user and on 32bit systems, it leads to
integer wrapping when we pass it to compute_user_elem_size():

	alloc_size = compute_user_elem_size(private_size, count);

However, the integer over is harmless because later "count" is checked
when we pass it to snd_ctl_new():

	err = snd_ctl_new(&kctl, count, access, file);

These days as part of kernel hardening we're trying to avoid integer
overflows when they affect size_t type.  So to avoid the integer overflow
copy the check from snd_ctl_new() and do it at the start of the
snd_ctl_elem_add() function as well.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
I'm going to write a blog about this which explains the kernel hardening
proposal in more detail.

The problem is that integer overflows are really hard to analyze
because the integer overflow itself is harmless.  The harmful thing comes
later.  Not only are integer overflows harmless, but many of them are
done deliberately.

So what we're doing is we're saying that size_t types should not overflow.
This eliminates many deliberate integer overflows handling time values for
example.  We're also ignoring deliberate idiomatic integer overflows such
as if (a + b < a) {.

We're going to detect these integer overflows using static analysis and at
runtime using UBSan and Syzbot.

The other thing, actually, is the we're planning to only work on 64bit
systems for now so if you want to ignore this patch then that's fine.  There
are a lot more (like 10x more) integer overflows on 32bit systems but most
people are on 64bit.  So it's less work and more impact to focus on 64bit
at first.

 sound/core/control.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 4f55f64c42e1..82b9d14f4ee3 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1641,6 +1641,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	count = info->owner;
 	if (count == 0)
 		count = 1;
+	if (count > MAX_CONTROL_COUNT)
+		return -EINVAL;
 
 	/* Arrange access permissions if needed. */
 	access = info->access;
-- 
2.45.2

On Mon, 30 Sep 2024 09:19:58 +0200,
Dan Carpenter wrote:
> 
> This patch doesn't change runtime at all, it's just for kernel hardening.
> 
> The "count" here comes from the user and on 32bit systems, it leads to
> integer wrapping when we pass it to compute_user_elem_size():
> 
> 	alloc_size = compute_user_elem_size(private_size, count);
> 
> However, the integer over is harmless because later "count" is checked
> when we pass it to snd_ctl_new():
> 
> 	err = snd_ctl_new(&kctl, count, access, file);
> 
> These days as part of kernel hardening we're trying to avoid integer
> overflows when they affect size_t type.  So to avoid the integer overflow
> copy the check from snd_ctl_new() and do it at the start of the
> snd_ctl_elem_add() function as well.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I'm going to write a blog about this which explains the kernel hardening
> proposal in more detail.
> 
> The problem is that integer overflows are really hard to analyze
> because the integer overflow itself is harmless.  The harmful thing comes
> later.  Not only are integer overflows harmless, but many of them are
> done deliberately.
> 
> So what we're doing is we're saying that size_t types should not overflow.
> This eliminates many deliberate integer overflows handling time values for
> example.  We're also ignoring deliberate idiomatic integer overflows such
> as if (a + b < a) {.
> 
> We're going to detect these integer overflows using static analysis and at
> runtime using UBSan and Syzbot.
> 
> The other thing, actually, is the we're planning to only work on 64bit
> systems for now so if you want to ignore this patch then that's fine.  There
> are a lot more (like 10x more) integer overflows on 32bit systems but most
> people are on 64bit.  So it's less work and more impact to focus on 64bit
> at first.

The fix is straightforward and still better to have even for 64bit, so
let's take it.  Now merged to for-linus branch.


thanks,

Takashi

On 30. 09. 24 9:19, Dan Carpenter wrote:
> This patch doesn't change runtime at all, it's just for kernel hardening.
> 
> The "count" here comes from the user and on 32bit systems, it leads to
> integer wrapping when we pass it to compute_user_elem_size():
> 
> 	alloc_size = compute_user_elem_size(private_size, count);
> 
> However, the integer over is harmless because later "count" is checked
> when we pass it to snd_ctl_new():
> 
> 	err = snd_ctl_new(&kctl, count, access, file);
> 
> These days as part of kernel hardening we're trying to avoid integer
> overflows when they affect size_t type.  So to avoid the integer overflow
> copy the check from snd_ctl_new() and do it at the start of the
> snd_ctl_elem_add() function as well.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I'm going to write a blog about this which explains the kernel hardening
> proposal in more detail.
> 
> The problem is that integer overflows are really hard to analyze
> because the integer overflow itself is harmless.  The harmful thing comes
> later.  Not only are integer overflows harmless, but many of them are
> done deliberately.
> 
> So what we're doing is we're saying that size_t types should not overflow.
> This eliminates many deliberate integer overflows handling time values for
> example.  We're also ignoring deliberate idiomatic integer overflows such
> as if (a + b < a) {.
> 
> We're going to detect these integer overflows using static analysis and at
> runtime using UBSan and Syzbot.
> 
> The other thing, actually, is the we're planning to only work on 64bit
> systems for now so if you want to ignore this patch then that's fine.  There
> are a lot more (like 10x more) integer overflows on 32bit systems but most
> people are on 64bit.  So it's less work and more impact to focus on 64bit
> at first.
> 
>   sound/core/control.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 4f55f64c42e1..82b9d14f4ee3 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1641,6 +1641,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>   	count = info->owner;
>   	if (count == 0)
>   		count = 1;
> +	if (count > MAX_CONTROL_COUNT)
> +		return -EINVAL;
>   
>   	/* Arrange access permissions if needed. */
>   	access = info->access;

It looks safe and this check is already in snd_ctl_new(). Perhaps, it may be 
clever to add a direct comment to the code about purpose of this extra 
(double) check.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>


				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.