[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <22ea267f-2326-4128-a182-a4e90d4cfb68@perex.cz>
Date: Mon, 30 Sep 2024 09:35:25 +0200
From: Jaroslav Kysela <perex@...ex.cz>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Takashi Iwai <tiwai@...e.com>, Mark Brown <broonie@...nel.org>,
Takashi Sakamoto <o-takashi@...amocchi.jp>, linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ALSA: silence integer wrapping warning
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@...aro.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@...ex.cz>
Jaroslav
--
Jaroslav Kysela <perex@...ex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Powered by blists - more mailing lists