| Message ID | 5457e8c1-01ff-4dd9-b49c-15b817f65ee7@stanley.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a04dae6fa4fc56c6a29cd40e133ef6a77f2c7e4e |
| Headers | show |
| Series | ALSA: silence integer wrapping warning | expand |
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
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
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;
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(+)